[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()

  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 20 Sep 2021 17:27:17 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=9xHMM1ePrv4Vr/9K70ZCf92wFZaFxdeMKR/JT3xeIxM=; b=FDTHpZ4IhBcTZhCGh7KrgfjI5sOiUm2/POrrX+A+B6Z6ln8Fe6t36zxazFilwAuwoZtO8EZqxrm1XKODOV3bbNxXeueiJnLcBNyqEb0XTQBd+7xA9sx6P0VqNszOa39IelUxSt9nja6NOYqTiD0shMBtvqB5p5SAAA0UoWHRk2mqSVxSMgg7q6VpXR3QgB4dCnxKZ18gb9KyjZOgmMqxXRtNXH/pR4n6v5ESgF4pIIEOMdzmVz9m6epXmdN+78E6+VkBUE2NGoGhsoegv83wAA/kDCBJXEjZHWE/0MS3+qMLE0oAvuLeqey9eguSjfWSmZSn28yE0KTW0dydFGv2yA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=E25hpfpD9fizYHQHS5010nJggpjTf47Oogy1teD56Bo7GHKIMX0I23VbZ5hEB8APr+3Ft7XDto1HLakymC3EIfLDI2s/QnQ188sg/rJbKqsnZLSAP67WG5xmZcMAZ+K2zLUahWQHNFd/mLd2jv2U4aF1K1H9/FvOEmMVdXmOX8fP30J8axPDllCkjP8dP0ytT0j4+lFcNPvxfpuc/oZSWD8P9Us77eTovZ/K3+5ICGt7SZl35SOUHRWmNb2NqH6q4IJt2aCR0t7OELS0hYbFXqYoOFrLQXV/0ZKZgibp78XAdN/rwbOEdoL2XuyeJGQ9sZPFkHASS6K4ROS+whp89Q==
  • Authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Mon, 20 Sep 2021 15:27:30 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




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