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
