[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame()


On 22/09/2021 14:42, Jan Beulich wrote:
On 22.09.2021 11:26, Roger Pau Monné wrote:
On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
On 21.09.2021 10:32, Roger Pau Monné wrote:
On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
On 20.09.2021 12:20, Roger Pau Monné wrote:
On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote:
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
+        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \

I'm slightly confused by this checks, don't you need to check for
gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call

Why? It's ogfn which gets passed to the function. And it indeed is the
prior GFN's mapping that we want to remove here.

Or assuming that ogfn is not invalid can be used to imply a removal?

That implication can be (and on x86 is) used for the incoming argument,
i.e. "gfn". I don't think "ogfn" can serve this purpose.

I guess I'm confused due to the ogfn checks done on the Arm side that
are not performed on x86. So on Arm you always need to explicitly
unhook the previous GFN before attempting to setup a new mapping,
while on x86 you only need to do this when it's a removal in order to
clear the entry?

The difference isn't with guest_physmap_add_entry() (both x86 and
Arm only insert a new mapping there), but with
xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
mappings. And gnttab_map_frame() gets called only from there. This
is effectively what the first paragraph of the description is about.

OK, sorry, it wasn't clear to me from the description. Could you
explicitly mention in the description that the removal is moved into
gnttab_set_frame_gfn on Arm in order to cope with the fact that
xenmem_add_to_physmap_one doesn't perform it.

Well, it's not really "in order to cope" - that's true for the placement
prior to this change as well, so not a justification for the change.
Nevertheless I've tried to make this more clear by changing the 1st
paragraph to:

"Without holding appropriate locks, attempting to remove a prior mapping
  of the underlying page is pointless, as the same (or another) mapping
  could be re-established by a parallel request on another vCPU. Move the
  code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since
  xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of
  course this new placement doesn't improve things in any way as far as
  the security of grant status frame mappings goes (see XSA-379). Proper
  locking would be needed here to allow status frames to be mapped

TBH I think it would be in our best interest to try to make
xenmem_add_to_physmap_one behave as close as possible between arches.
This discrepancy between x86 and Arm regarding page removal is just
going to bring more trouble in the long term, and hiding the
differences inside gnttab_set_frame_gfn just makes it even more

Stefano, Julien?

This would be ideal as I don't particular like the approach taken in this patch. But AFAICT, this would require us to implement an M2P. Or is there another way to do it?

In another context, I saw the suggestion to bring an M2P on Arm. But I am still somewhat split whether this is really worth it for the current use.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.