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

Ping: [PATCH] common: guest_physmap_add_page()'s return value needs checking


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 21 Sep 2021 10:02:52 +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=4S774r1OPPy/t2Aaam3xGobazJwl30XB1bp7FcSWO9k=; b=LggVY+WtFfa5xOBQ/H0vhvo0BiSgO+gmeRI9TolTFQ+iGu78GfsUFu/qWdBEj9t6klXfkGySt1ZtjtDawcSKGRiV1A2lu+8n+0ZZz62sZ6tYNGt9l2J0PYh3Uug/r5lXmXAQSvFbVvSBlqI+GDGeB0oE+V2jkPyviLQuCfwv6AsSHOpQLRFlMvLKhb7nMpJn43ind8rsFLxITuznyrGooyQ7hkPpbZwYn5fyyOtfGe58fPyKMS4OlW76pUYAH3d8EHLBO41stP5mA0rF8A8dq1YqLntO+d62lkZnIY2+sw/dnI7tSq06v5U3xHIdzTXGkI9YX8zKVDda9IbLKJHk0A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kQOXRyWyi/xLvof80RMXlcD1/8mKNKlUvevDU6tm4A04NZR8epolyqb617Ia1V9yKZGK3TiFQhjPQf4943MSKl/h6NPBbcspreKtjVNcZrDUBk7ZubU6BcIVHO8KZeB+w8T4yD5Mx0ALyhGBkxGp8hvovfeQoCgpvxhoCLDYCsCCdWYrmE5N+Zz4kWgi3nh9faaIh/qe19tQOn7SmYSGe+Hs/YNQH+TFkKpOFl0hPhld5yUjyzXKDRkq/SRaP61r/Ab/HoEk4VY+Iueflwgt8T8OcjbNu4T0x61j1JhcTJ85QUgLKyUx1TGQHM3UxNcjmDhTUSOqeOQTgu6S8KXD+Q==
  • Authentication-results: citrix.com; dkim=none (message not signed) header.d=none;citrix.com; dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 21 Sep 2021 08:03:01 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 01.09.2021 18:06, Jan Beulich wrote:
> The function may fail; it is not correct to indicate "success" in this
> case up the call stack. Mark the function must-check to prove all
> cases have been caught (and no new ones will get introduced).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> In the grant-transfer case it is not really clear to me whether we can
> stick to setting GTF_transfer_completed in the error case. Since a guest
> may spin-wait for the flag to become set, simply not setting the flag is
> not an option either. I was wondering whether we may want to slightly
> alter (extend) the ABI and allow for a GTF_transfer_committed ->
> GTF_transfer_completed transition (i.e. clearing GTF_transfer_committed
> at the same time as setting GTF_transfer_completed).

While I did get a reply from Andrew on this additional remark, the code
change itself has remained un-acked and un-responded to, despite imo
being pretty straightforward. May I please as for an ack or otherwise
an indication of what needs to be changed?

Jan

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2394,7 +2394,7 @@ gnttab_transfer(
>          {
>              grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, 
> gop.ref);
>  
> -            guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
> +            rc = guest_physmap_add_page(e, _gfn(sha->frame), mfn, 0);
>              if ( !paging_mode_translate(e) )
>                  sha->frame = mfn_x(mfn);
>          }
> @@ -2402,7 +2402,7 @@ gnttab_transfer(
>          {
>              grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, 
> gop.ref);
>  
> -            guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 0);
> +            rc = guest_physmap_add_page(e, _gfn(sha->full_page.frame), mfn, 
> 0);
>              if ( !paging_mode_translate(e) )
>                  sha->full_page.frame = mfn_x(mfn);
>          }
> @@ -2415,7 +2415,7 @@ gnttab_transfer(
>  
>          rcu_unlock_domain(e);
>  
> -        gop.status = GNTST_okay;
> +        gop.status = rc ? GNTST_general_error : GNTST_okay;
>  
>      copyback:
>          if ( unlikely(__copy_field_to_guest(uop, &gop, status)) )
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -268,7 +268,8 @@ static void populate_physmap(struct memo
>                  mfn = page_to_mfn(page);
>              }
>  
> -            guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order);
> +            if ( guest_physmap_add_page(d, _gfn(gpfn), mfn, a->extent_order) 
> )
> +                goto out;
>  
>              if ( !paging_mode_translate(d) &&
>                   /* Inform the domain of the new page's machine address. */
> @@ -765,8 +766,8 @@ static long memory_exchange(XEN_GUEST_HA
>              }
>  
>              mfn = page_to_mfn(page);
> -            guest_physmap_add_page(d, _gfn(gpfn), mfn,
> -                                   exch.out.extent_order);
> +            rc = guest_physmap_add_page(d, _gfn(gpfn), mfn,
> +                                        exch.out.extent_order) ?: rc;
>  
>              if ( !paging_mode_translate(d) &&
>                   __copy_mfn_to_guest_offset(exch.out.extent_start,
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -307,10 +307,9 @@ int guest_physmap_add_entry(struct domai
>                              p2m_type_t t);
>  
>  /* Untyped version for RAM only, for compatibility */
> -static inline int guest_physmap_add_page(struct domain *d,
> -                                         gfn_t gfn,
> -                                         mfn_t mfn,
> -                                         unsigned int page_order)
> +static inline int __must_check
> +guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn,
> +                       unsigned int page_order)
>  {
>      return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
>  }
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -583,8 +583,8 @@ int guest_physmap_add_entry(struct domain *d, gfn_t gfn,
>                              p2m_type_t t);
>  
>  /* Untyped version for RAM only, for compatibility and PV. */
> -int guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn,
> -                           unsigned int page_order);
> +int __must_check guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t 
> mfn,
> +                                        unsigned int page_order);
>  
>  /* Set a p2m range as populate-on-demand */
>  int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long 
> gfn,
> 
> 




 


Rackspace

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