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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
>> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned
>>          XFREE((gt)->arch.status_gfn);                                    \
>>      } while ( 0 )
>>  
>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn)                           \
>> -    do {                                                                 \
>> -        ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] =    \
>> -            (gfn);                                                       \
>> -    } while ( 0 )
>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
>> +    ({                                                                   \
>> +        int rc_ = 0;                                                     \
>> +        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
> 
> Newline maybe? Not sure whether we decided that macros should also
> follow coding style regarding variable definition followed by newline.

To be honest in macros I'm always on the fence: A long line of all blanks
and just a trailing backslash is about as ugly to me as the missing
separator. I think normally I opt for the way chosen above, but I'm not
going to claim to know I'm consistent with this.

>> +        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.

> Also the check for ogfn == gfn is only used on Arm, while I would
> assume a similar one would be needed on x86 to guarantee the same
> behavior?

Things are sufficiently different on Arm and x86. As said above, on
x86 I'm indeed using "gfn" being INVALID_GFN as an indication that a
removal is requested. This is simply transforming the behavior from
prior to this change, with the function invocation moved into the
per-arch macros. It may be relevant to note that
gnttab_unpopulate_status_frames() calls gnttab_set_frame_gfn() with
INVALID_GFN only when gnttab_get_frame_gfn() didn't return
INVALID_GFN, i.e. the gnttab_get_frame_gfn() used in
gnttab_set_frame_gfn() won't return this value either (we've not
dropped any locks in between). And the only other caller of
gnttab_set_frame_gfn() guarantees "gfn" not to be INVALID_GFN. A
little fragile (towards hypothetical further callers of the macro/
function), yes, but I couldn't think of anything substantially
better.

Jan