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

Re: [PATCH for-4.16 v4] gnttab: allow setting max version per-domain


  • To: Julien Grall <julien@xxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 29 Oct 2021 13:04:23 +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:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=xkmFxWAVPJ5VUqgBkfUGg8iQeZPlKe9heBr3gRjkk5I=; b=lfWDLr4VuVhFBuXarQqC5qnNEFQP3htlr1k9PYwvoEjYxd/os28QqsMfEshMNUqPb0TYNsv+YwOfz9FFCwEDCLAEgjiSzBXHoMFuswzATfalz+BSAVoPM1XqK9Xyz+c3XRrY5TOW5Gzxr7t5hwHw3vabwqe3MJeX2vlakP9JWGOCwo9mINEIvH4++rhCy4JgTtRyypiUHwxzE6kWUUY74+n+TqY/05IMqAXNIc63+r8ETqavPuc9M6UbhigdwlwXIW1hcJhzRjFTKNO5WJP/ygdSn1vP4Agy7ltX+X3WFb750tGw46tQr/eQ5mjF6qFtAOIyf6bXRnOjGFWx+8avhQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Z3XdZ7sIXQPHZl6hDDL37RO9ZNyLYsR19IhofmM7GMN1U9Zwv8pDBeWm7oJESi/fwfkRIEpAZBWMnd9kj5l7aeFyYDKH5KWQO/0rFp7ZsmS6Nj29Z7NVwP41uSeIv8exAbwZSaDwfXMdvrNBF/acGM9EKblB73sNSUXlOg35vnPs18LJEdk0H5mpPX5CWB1UK5+jO3Etk06/nEUGwJ+zru6rfgka03vzIc3+RcooC2xwVIylp/aWLtf8r/VW7/XVkbs954QH4SAESOzOeurNyrrX/eS5gfChYv4dcnAgttF9mxvtUCK0f8IE5ftcQByU3EUVktk7kpQPQpIYF0P2jA==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Christian Lindig <christian.lindig@xxxxxxxxxx>, David Scott <dave@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Fri, 29 Oct 2021 11:05:00 +0000
  • Ironport-data: A9a23:t0DtQKJTA+R1kof9FE+RjZMlxSXFcZb7ZxGr2PjKsXjdYENS0DZUz zAcDzzQbvmON2fye48iPNywpx8Cu8fdz4MwTVZlqX01Q3x08seUXt7xwmUcns+xwm8vaGo9s q3yv/GZdJhcokcxIn5BC5C5xZVG/fjgqoHUVaiUZUideSc+EH140Eo5yrZg6mJVqYPR7z2l6 IuaT/L3YDdJ6xYsWo7Dw/vewP/HlK2aVAIw5jTSV9gS1LPtvyB94KYkDbOwNxPFrrx8RYZWc QphIIaRpQs19z91Yj+sfy2SnkciGtY+NiDW4pZatjTLbrGvaUXe345iXMfwZ3u7hB2ogOJ0+ olPlqWUUAd2G6DUt+0RahVXRnQW0a1uoNcrIFC6uM2XiUbHb2Ht07NlC0Re0Y8wo7gtRzsUr LpBdW5LPkvra+GemdpXTsF2gcsuNo/zNZ43sXB81zDJS/0hRPgvRo2Xu4YIjGxq1qiiG97FX 9E6YhxkXC/iegJwME4sL8hvsL+R0yyXnzpw9wvO+PtfD3Lo5AB4zrXFKtfefd2OA8JPkS6wp G3c+H/iKgoHL9HZwj2Amlqtme3njS79QJgVFrCz6rhtmlL77lIUDBoaRF6qu86Tg0S1W89cA 0EM8y9opq83nGSwVcX0VRC8pH+CvzYfVsBWHul87xuCooLE7gDcCmUaQzppbN09qNRwVTEsz kWOnd7iGXpoqrL9YXCA8raZqxuiNC5TKnUNDQcGRwYY59jooKkokwnCCN1kFcadkdndCTz2h TeQo0ADa6471JBRkf/hpBae3mzq9sOhohMJChv/f32X6zElZq2cQ5Wotn3W9dlJIL+VQQzU1 JQboPS24OcLBJCLsSWCRuQRAb2kj8q43C3gbU1HRMZ5qWz8k5K3VcUJumsmfRY2WioRUWaxO Be7hO9H2HNE0JJGh4dMaIWtF99i86HkEdn0Phw/RosTOsYvHONrEScHWKJx44wPuBRz+U3cE c3CGSpJMZr8If46pNZRb7xEuYLHPghkmQvuqWnTlnxLK4a2an+PUqsiO1CTdO0/567siFyLq IsDa5XXlkUGCLeWjszrHWg7dw1iwZ8TXsmeliCqXrTbfloO9J8JUqe5LUwdl3xNwP0Oy7agE oCVUU5E0lvv7UAr2i3RAk2PnIjHBM4lxVpiZHREFQ/xhxALPNb+hI9CJsBfVeR2q4ReIQtcE qBtlzOoWa8UFFwqOl01MPHAkWCVXE/621PRZnD1MGRXklwJb1Whx+IItzDHrUEmJiG2qdE/s /un0AbaSoAEXANsEIDdb/fH8r97lSF1dDtaUxSaL99NVl/r9YQ2eSX9guVue5MHKAnZxyvc3 AGTWE9Kqe7Iqo4z0d/ImaHb8Nv5T7ogRhJXTzvB8LK7FSjG5W7/k4VOZ/mFIGLGX2Tu9aT8O egMl6PgMOcKlUphupZnF+o51ro34tbi/ucIzgltEHjRQU6sD7dsfiuP0cVV7/Ufzb5FowqmH EmI/4ACa7mOPcrkFn8XJRYkMbvfha1FxGGK4K1sckvg5SJx8L6WamloPkGB2H5HMb94EII52 uN96sQY3BOy10gxOdGcgyEKq2nVdi4cU78qv40xCZPwjlZ50UlLZJHRB3Ok4JyLbNkQYEAmL iXN2fjHjrVYgEHDb2AyBT7G2u8E3cYCvxVDzVkjIVWVm4Wa2q9rjUMJqTlnHB5Iyhhn0v5oP jk5PkJ4EqyC4jN0iZURRGurAQxAWEWU90GZJ4HlT4EFo51EjlDwEVA=
  • Ironport-hdrordr: A9a23:AyBKCKANHsyHsUzlHeg2sceALOsnbusQ8zAXPh9KJiC9I/b1qy nxppkmPH/P6Qr4WBkb6Le90Y27MAnhHPlOkPQs1NaZLXLbUQ6TQr2KgrGSoQEIdxeOk9K1kJ 0QD5SWa+eAfGSS7/yKmTVQeuxIqLLskNHK9JfjJjVWPHlXgslbnnlE422gYytLrWd9dP4E/M 323Ls5m9PsQwVcUu2LQl0+G8TTrdzCk5zrJTYAGh4c8QGLyRel8qTzHRS01goXF2on+8ZvzU H11yjCoomzufCyzRHRk0fV8pRtgdPkjv9OHtaFhMQ5IijlziyoeINicbufuy1dmpDj1H8a1P 335zswNcV67H3cOkmzvBvWwgHllA0j7nfzoGXoyEfLkIjcfnYXGsBBjYVWfl/y8Ew7puxx16 pNwiawq4dXJQmoplW92/H4EzVR0makq3srluAey1ZFV5EFVbNXpYsDuGtIDZY7Gj7g4oxPKp ghMCjl3ocUTbqmVQGagoE2q+bcG0jbXy32DXTqg/blkwS/xxtCvg8lLM92pAZ3yHtycegC2w 3+CNUbqFh5dL5gUUtMPpZzfSKJMB25ffvtChPbHb21LtBNB5ryw6SHlIndotvaPqA18A==
  • Ironport-sdr: Ose0NfwTPiRvjnyOJZM0BHFppAlkQfbtbXHRBOwuMOvvgVslMwrA8uC1zrxNzMC3Cp8qWBi4Iy wD22FaOuSXfFsxfcvx0ZZxaLdqIQrf+iNA9QnoSztYZAbrlUj5pO41kyFk8rI3vtC+0ZOsgFpi CqIMMg4Fca8aMcxZdOvAM6yyRRMfi+PL3Czqt8Oq3q/hU3hXjmNJn/oV7j245/hLmS497RlO0B zNMUwfMVk3miyWzsjWdWJJZboFPDj2UyH/No349/n04ivxdGr4Lcxcgbwrpq5r6CHJlcJ6pJW6 PjXuzS2sUfGKX77piruTeJpL
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hello,

On Fri, Oct 29, 2021 at 11:01:29AM +0100, Julien Grall wrote:
> 
> 
> On 29/10/2021 10:41, Roger Pau Monné wrote:
> > On Fri, Oct 29, 2021 at 09:58:55AM +0100, Julien Grall wrote:
> > > Hi Roger,
> Hi Roger,
> 
> > > On 29/10/2021 08:59, Roger Pau Monne wrote:
> > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > > > index e510395d08..f94f0f272c 100644
> > > > --- a/xen/common/grant_table.c
> > > > +++ b/xen/common/grant_table.c
> > > > @@ -53,6 +53,7 @@ struct grant_table {
> > > >        percpu_rwlock_t       lock;
> > > >        /* Lock protecting the maptrack limit */
> > > >        spinlock_t            maptrack_lock;
> > > > +    unsigned int          max_version;
> > > >        /*
> > > >         * Defaults to v1.  May be changed with GNTTABOP_set_version.  
> > > > All other
> > > >         * values are invalid.
> > > > @@ -1917,11 +1918,33 @@ active_alloc_failed:
> > > >    }
> > > >    int grant_table_init(struct domain *d, int max_grant_frames,
> > > > -                     int max_maptrack_frames)
> > > > +                     int max_maptrack_frames, unsigned int options)
> > > >    {
> > > >        struct grant_table *gt;
> > > > +    unsigned int max_grant_version = options & 
> > > > XEN_DOMCTL_GRANT_version_mask;
> > > >        int ret = -ENOMEM;
> > > > +    if ( max_grant_version == XEN_DOMCTL_GRANT_version_default )
> > > > +        max_grant_version = opt_gnttab_max_version;
> > > > +    if ( !max_grant_version )
> > > > +    {
> > > > +        dprintk(XENLOG_INFO, "%pd: invalid grant table version 0 
> > > > requested\n",
> > > > +                d);
> > > > +        return -EINVAL;
> > > > +    }
> > > > +    if ( max_grant_version > opt_gnttab_max_version )
> > > > +    {
> > > > +        dprintk(XENLOG_INFO,
> > > > +                "%pd: requested grant version (%u) greater than 
> > > > supported (%u)\n",
> > > > +                d, max_grant_version, opt_gnttab_max_version);
> > > > +        return -EINVAL;
> > > > +    }
> > > > +    if ( unlikely(max_page >= PFN_DOWN(TB(16))) && is_pv_domain(d) &&
> > > 
> > >  From my understanding, the limit for the grant table v1 is based on the 
> > > page
> > > granularity used and the size of the fields.
> > > 
> > > So the limit you add is valid for 4KB but not 16KB/64KB. Therefore, I 
> > > think
> > > it would be better to use:
> > > 
> > > 'max_page >= (1U << 32)'
> > 
> > I'm slightly confused. Isn't Xen always using a 4KB page granularity,
> 
> Yes. We only support 4KB today. But most of Xen is agnostic to the page
> granularity. I have actually started to look to allow 64KB/16KB page
> granularity for Xen on Arm in my spare time.
> 
> > and that also applies to the grant table entries?
> The page granularity for the hypercall interface is whatever the page
> granularity Xen is using. So...

I've somehow assumed that the current hypercall ABI was strictly tied
to 4KB pages, as that's for example already hardcoded in Linux
as XEN_PAGE_SIZE.

> > 
> > I don't think it's possible to use correctly use a 16KB or 64KB page
> > as an entry for the grant table, as Xen assumes those to always be 4KB
> > based.
> 
> ... if you build Xen with 16KB, then the grant table entries will be using
> 16KB.
> 
> So I would like to avoid making the assumption that we are always using 4KB.
> That said, the worse that can happen is a spurious message. So this is more
> to get an accurate check.

I don't have strong objections to using max_page >> 32, it might even
be clearer than checking against TB(16).

It's just that the check would be wrong if we allow Xen itself to use
a different page size than the one used by the grant table interface
to the guest.

> > 
> > > Furthermore, it would add a comment explaining where this limit comes 
> > > from.
> > > 
> > > Lastly, did you check the compiler wouldn't throw an error on arm32?
> > 
> > I've tested a previous version (v2), but not this one. I assume it
> > doesn't build?
> 
> I haven't tried. But I remember in the past seen report for always
> true/false check. Maybe that was just on coverity?

Hm, possibly. It seems like debian-unstable arm32 gcc is building OK,
but I've got no idea if different compiler versions could complain.

Thanks, Roger.



 


Rackspace

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