[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: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 22 Sep 2021 11:26:37 +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=9x5Q87cIOuFleBoYcvIT6cs7a8IkDoF0kihHsHDYnhA=; b=oVT+kVI63H8cAQR4fJNchh75CmCO8wx886JqguVj6EClTg1zzbEsqK9EoGmcmYcwLl0kuqo8bbogUAS8/l7LOoHi3PKWlC36mViKSgRDcSBGyVA58uNrEP831TBYiI7Nowoq0eMOXukbv3HGn/OtV6lyJNeHjA7ZOcV3FHep1iCgB2+cERYwRyq2fmMReIpmToSJ7ZvvfGBMMvkyBejZ0XNMSI43aIfYvTIA1DFg6dP0/3TfAppCLWcWwwkop6GktNNr4HfsTdfEiW1G0Capx70DpUtdkLeeRqY4jFQlv4diAnnM2xoilv/XDva+t1pEaMB+BAs/4300T/3AYOwZNA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZnUKcw5p9CezGq4cTIM2QhRR3dkuXDx2H6nZEd2jQkQG1rQUZA3ACOHI+AuCuavnsnZ0lxiTVMUGfafqXpE/DNCNFl9dDVFk0+u1QWwBstW/jaYMMygES+AvsuLHhwB8sVxi53xQCVpmzGv7ZmkeYfXIbgUNNmOSDJ4YvOcq8FPT3Op56OWtFU0W8DMiNnLrZpeafIlGq7a7G4jo4hBkh48GJ0FmYYsVBjEf9QMpwN4XCIpAzJeWs1z7HnJbpxqm/EyTrHQLOMwWuBwBdK7N2B0Ha2WUrDHUpnwBnNyDPWzGecO9okTZImpzz5//wQLzpBvcPDmJqMQWXVTNz/7e0w==
  • 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: Wed, 22 Sep 2021 09:26:52 +0000
  • Ironport-data: A9a23:tZtqPqjOr1eulYqM9kpV19+UX161shcKZh0ujC45NGQN5FlHY01je htvXGCFPaveNDD2ctp1PI22oU9TuJOBnIVgSgNp+ykyQSMb9cadCdqndUqhZCn6wu8v7a5EA 2fyTvGacajYm1eF/k/F3oAMKRCQ7InQLlbGILes1htZGEk0FU/NtTo5w7Rg2t8y24Dja++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /1W5LK8Ui07ZJTUldgMTR1lK2JDJqRZreqvzXiX6aR/zmXDenrohf5vEFs3LcsT/eMf7WNmr KJCbmpXN1ba2rzwkOnTpupE36zPKOHxO4wSoDd4xCzxBvc6W5HTBa7N4Le02R9t25kWQK6CO qL1bxJhZh3xXQcMPms2K5AUzNuqpkjeKQdx/Qf9Sa0fvDGIkV0ZPKLWGNjIft2HQ+1Fk0Deo XjJl0ziGQ0TPtGbzTuD81qvi/XJkCe9X5gdfJWn8tZ6jVvVwXYcYDUUX1ampfiyimalRslSb UcT/0IGvaU0sUCmUNT5dxm5u2Kf+A4RXcJKFO834x3LzbDbiy6YGWosXjNHcMYhtsI9WXotz FDhoj/yLWUx6vvPEyvbr+rK62PpUcQIEYMcTTRVYwceycu5mZk+sgPRSutbU5Ouh8KgTFkc3 Au2QDgCa6Q71JBQjfzrowyf2lpAtbCSEVVkvVy/snaNq1ojPd/7PdTABU3zsK4YRLt1WGVtq 5TtdyK21+kIEZjFvyiEWuxl8FqBtqvdbWG0bbKCGfAcG9WRF5yLJts4DNJWfh4B3iM4ldjBO hS7hO+pzMUPVEZGlIcuC25LNyjP8ZUM6Py/DqyEBjaxXnSBXFDep3w/DaJh90vsjFItgckCB HtvSu71VSxyIf0+lFKeHr5BuZd2lnFW7T6CHvjTkkX4uYdykVbIEN/pxnPVNbtnhE5FyS2Im +ti2zyikUQHDLKmPXmJrub+7zkidBAGOHw/kOQOHsarKQt6AmAxTfjXxLIqYYt+mKpJ0OzP+ xmAtoVwljITXFXLdleHbG5NcrTqUcotpH43J3V0b12px2IiccCk66JGL8k7erwu9epCy/9oT qZaJ5XcU6oXEjmXqS4AaZTdrZB5cEj5jwy5ICf4MiM0eIRtRlKV94a8LBfv7iQHEgG+qdA6/ ++7zgreTJdaH1ZiAc/aZeiB1VS0uXRByut+U1GReotYeVn28ZgsICv016dlL8YJIBTF5z2by wfJXktI+biT+9c4qYCbi7qFooGlF/pFMnBbR2SLv6yrMST6/3a4xdMSWui/Yj2ABnj//7+vZ LsJwqikYuEHhltDr6F1D61vkfAl/9LqqrJXklZkEXHMYwj5A79sOCDbj8xGt6kLzb5FowqmH EmI/4ACa7mOPcrkFn8XJRYkMbvfha1FxGGK4KRnOlj+6Q924KGDABdbMBS7gSBAKKd4bdE+y uA7tc9KswGyh3LG6DpdYvy4I4hUEkE9bg==
  • Ironport-hdrordr: A9a23:P1MK6qmGaUI/v282B9tPH6jO14bpDfO/imdD5ihNYBxZY6Wkfp +V8sjzhCWatN9OYh0dcLC7WJVpQRvnhPhICK0qTMqftW7dyReVxeBZnPHfKljbehEWmdQtsJ uIH5IObOEYSGIK8voSgzPIY+rIouP3iJxA7N22pxwGIHAIGsMQnDuRSDzraXGeLDM2dKbRf6 Dsn/avyQDQHkj+Oa+Adwc4tqX41pH2vaOjRSRDKw8s6QGIgz/twLnmEyKA1hNbdz9U278t/U XMjgS8v8yYwr+G4y6Z81WWw4VdmdPnxNcGLMuQivINIjGprgqzfoxuV5CLoThwiuCy71QBls XKvn4bTotOwkKUWlvwjQrm2gHm3jprw3j+yWWAiX+mmsD9TCJSMbsLuatpNj/ir2YwttB116 xGm0iDsYBMMB/GlCPho/DVShBDjCOP0DkfuN9Wq0YafZoVabdXo4Ba1lhSCo08ECXz751iOP VyDfvb+O1dfTqhHjDkV1FUsZmRt0kIb1O7qhBogL3T79EWpgE586Ig/r1cop9an6hNDaWt5I z/Q+xVff91P5YrhQ8UPpZ3fSKNMB25ffv7ChPaHb3WLtB0B5vzke+C3FwU3pDhRHVa9up+pH z+OGkow1LaPXieUfGz4A==
  • Ironport-sdr: oogck7G+RwvXlYQTkvzOhp1dp/dr9RLR5nLwk+UPdHu/aslLE3wCSV0Zv+NTlH+hun38NxLJPd LRRv3lHPcMxWS0GnEisTu6iqpJllawky+dgLIURD7K+7yQLN9PXXxpgjCNIecI0Uz/Jy6m9Dkc xr4yqDo/1Y+TTvCbmoLuPvlAlZREyYR+0Shc4vBZZDXtI4nTZL6xK7T+mlq3MUYOE5gNLbApJn minvaPh2GNmHI1m8TL9HBfa/vTUCpW+KlBKHvgtRFqg+wsnYI9sTrloe5Dxe0essszZIJSuqbp +bAOGYUxq92CN0pU82trFyR3
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote:
> On 21.09.2021 10:32, Roger Pau Monné wrote:
> > On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote:
> >> 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
> >>>> +        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.
> > 
> > I guess I'm confused due to the ogfn checks done on the Arm side that
> > are not performed on x86. So on Arm you always need to explicitly
> > unhook the previous GFN before attempting to setup a new mapping,
> > while on x86 you only need to do this when it's a removal in order to
> > clear the entry?
> 
> The difference isn't with guest_physmap_add_entry() (both x86 and
> Arm only insert a new mapping there), but with
> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior
> mappings. And gnttab_map_frame() gets called only from there. This
> is effectively what the first paragraph of the description is about.

OK, sorry, it wasn't clear to me from the description. Could you
explicitly mention in the description that the removal is moved into
gnttab_set_frame_gfn on Arm in order to cope with the fact that
xenmem_add_to_physmap_one doesn't perform it.

TBH I think it would be in our best interest to try to make
xenmem_add_to_physmap_one behave as close as possible between arches.
This discrepancy between x86 and Arm regarding page removal is just
going to bring more trouble in the long term, and hiding the
differences inside gnttab_set_frame_gfn just makes it even more
obscure.

Thanks, Roger.



 


Rackspace

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