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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 21 Sep 2021 11:20:28 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=Mw9V19GYeYg80+2iPoM1iFhfs5y4YO6kDw5nFI2ANSE=; b=No+3L/sNjSdimnLZDlpJeTmoSWfOm+hkaCYqR+4Ww4K7osQcW8iJAv+DC/XtA1N6HNgGR7+fONd98pdvQS9OfWeRq+kZjkRrv0sFdPETkIn0nwMy5Fj8ptu46zgqCsUfW2AUBMSuAzDtA7XhbIbBgRyEchexECjlkdvnTMOC16hChXECv133in+KTpDagj5XYARcxadYDYD63l7i4I7l60v8iOY2j5h7gZcRgO9HJLjsxRAqX69eBzRpwjbf9PhXrQO2v4a9oq8EV3PO+4oCvvqkCMf+WpofY+ZMJMRQde5IBJJW+ENu/sOj0d5dgLu4SwUc9hB0CXp1dm39Vdbyeg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=L2GYZiNqZzwKRKQITxpa7xdO/P3pive+DVQf5is9vL5ud2nUj/rDIHxvRgP5OIXiEnJRupNHUOICh7j3G2/NYZrT/CKPQBKIr9rMB+MBhA/DXxaiJcHxw1A11FhAQd6tjgQ1LqEvxKxVVcQ3yA+cossEQsKYLCYpGeuVseSNkVhBMVVxLSpUCgMHik5h+X4klTlD3l8RhNh3tptbWX+gSkjxu9/LSbtqmKbH5hOo2dNV7bNJNP48uE9y93krCzyjVve6FI0vMSNK/q4Lso5Q01rdMgUJpQ48BNfRFJmhRqphCmImTT4AxtZadU4fTsIq1JJhiPCo+G5HmL6fZCy+dg==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.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: Tue, 21 Sep 2021 09:20:53 +0000
  • Ironport-data: A9a23:PKlmnq9+P5hMspkFoe7bDrUDbnmTJUtcMsCJ2f8bNWPcYEJGY0x3x mFLD23Ub/nZM2akLt0ia4zn9RkHupODyoViSQc/rS48E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si9AttENlFEkvU2ybuOU5NXsZ2YhGGeIdA970Ug6w79j29Yx6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPhqm MsVlLKUTjwQEYjQp9oYQhh7NyNxaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcFgGZh2psTRJ4yY eInRxVVVxribyZgP2g3Eb4hgde2hlPwJmgwRFW9+vNsvjm7IBZK+LrwNNvYfPSaSMMTmVyXz krd5HjwCBweMN2ZyBKG/2iqi+uJmjn0MKoNEJWo+/gsh0ecrkQRAhALUVqwodGil1WzHdlYL iQ86ico6KQ/6kGvZt38RAGj5m6JuAYGXNhdGPF87xuCooLV/ASxFmUCViRGatEtqIkxXzNC6 7OSt4q3X3o16uTTEC/DsOfPxd+vBcQLBXRSe3clYy8C2ODMhZ0MsBfeT/VHEqHg27UZBgrML yC2QDkW3utI1J5QhvTjpzgrkBr3+cOYFVddChH/Gzv/t1InPtbNi5mAtACDhcusOrp1WbVoU JIsoMGY8OlGJpWEjiXlrA4lTezxuqrt3NExhzdS83gdG9aFoCXLkWN4umgWyKJV3iEsI2SBX aMrkVkNjKK/xVPzBUONX25UNyjN5fO6fekJq9iONoYeCnSPXFbfoUmCmnJ8L0iyyRNxwMnTy L+wcNq2DGZyNEiU5GPtHI8gPUsQ7nlmnwv7HMmjpzz+iOb2TCPFGN8tbQrVBshkvfzsnekg2 4sGXyd8404EC7OWj+i+2dN7EG3m2lBhVMiq9JQIJrDcSuekcUl4Y8LsLXoaU9UNt4xel/vS/ 2H7XUldyVHlgmbAJxnMYXdmAI4Dl74lxZ7iFSBzb1uuxVY5ZoOjsPUWe5ctJOF1/+1/1/9kC fICfpzYUPhITz3G/RUbbIX889M+JEj621rWMnr3eiU7cr5hWxfNpo3ucDzw+XRcFSGwr8Y// eGtj1uJXZoZSg1+J8/Kc/bznUiptH0QlbsqDUvFK9VeYmv2941uJ3Cjh/M7OZhUex7C2iGbx 0CdBhJB/bvBpIo88d/og6GYrtj2T7siTxQCR2SCtOS4LyjX+Gan0LRsaufQcGCPTn7w9YWje f5Rk6P2PsoYkQsYqIF7Cbtqk/4zvoO9u79Aww14N3zXdFD3WKh4K3yL0MQT5K1AwrhV5Vm/V k6Vo4QIPLyIPIXuEUILJRpjZeOGjKlGlj7X5PUzAUP7+C4oo+bXDRQMZ0GB2H5HMb94EII52 uNw6scZ5ju2hgcuLtvb3Dtf8H6BLyBYXqgq3n3A7FQHVub/Jol+XKHh
  • Ironport-hdrordr: A9a23:gt9ZBqvNCa9X2umRZCbHrxmX7skC54Mji2hC6mlwRA09TyXGra +TdaUguSMc1gx9ZJhBo7G90KnpewK6yXdQ2/hqAV7EZniahILIFvAY0WKG+VPd8kLFh4xgPM tbAs1D4ZjLfCRHZKXBkXiF+rQbsaC6GcmT7I+0pRcdLnAbV0gj1XYANu/yKDwJeOAsP+teKH Pz3Lsim9L2Ek5nEfhTS0N1EtTrlpnurtbLcBQGDxko5E2nii6p0qfzF1y90g0FWz1C7L8++S yd+jaJqZmLgrWe8FvxxmXT55NZlJ/IzcZCPtWFjowwJi/3ggilSYx9U/mpvSwzosuo9FE2+e O87ysIDoBW0Tf8b2u1qRzi103J1ysv0WbrzRuijX7qsaXCNXoHIvsEobgcXgrS6kImst05+r lMxXilu51eCg6FtDjh5vDTPisa1nackD4Hq6o+nnZfWYwRZPt6tooE5n5YF58GAWbT9J0nKu 9zF8vRjcwmMG9yV0qp/FWH/ebcG0jaRny9Mww/U42uonZrdUlCvgglLJd1pAZGyHo/I6M0rt gsfJ4Y0o2n46ctHNZA7dw6MLmK41r2MGTx2VKpUCfa/Z48SgfwQr7Mkf4IDbKRCdY1JKVbou W3bLofjx9qR37T
  • Ironport-sdr: /qXoLdOR4W/wHl2Rb1URGAWa7o5FsW6irWPgfOKMTKpDv8fyrHOMs/aA/ZYmyb6lx9tETI27E4 w5ssWk9Qhv/7aBZm0xQ4PrkaaxUL4S6uPB3LcC6PUNttaJI3I6VorTToZoOj4aSEHmezXE+OlO QVpONEyhTilBpE9BnQGrMqxBHkfLnBU4jTrFuEEgz8Ckgvs3PPLDhm6MeaGW0mtGojwFF2qSlr sy6IhFC5b8UHqVQQxG/D/LHrio3XSuJywa6RAULYMhIXLhEpltKravnTIuEVAmPXz+nc0NcpC8 luBe/JoX+NyoWNXEMGQYIdNf
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Sep 01, 2021 at 06:06:37PM +0200, 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>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just a couple of comments, as we now handle errors in some placs where
we didn't before.

> ---
> 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).
> 
> --- 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);

Is it fine to set the frame even if updating the physmap failed?

>          }
> @@ -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,

Would it be worth it setting the mfn on the guest output to
INVALID_MFN or some such if the physmap addition failed?

Thanks, Roger.



 


Rackspace

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