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 > >>> guest_physmap_remove_page? > >> > >> 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. 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 obscure. Thanks, Roger.
