[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 15/18] IOMMU/x86: prefill newly allocate page tables
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Date: Tue, 14 Dec 2021 16:15:16 +0100
- 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=YN9lW1TI4ATp0d0WrCH5IjPExl0iV6e2d5QMJ/yvlEc=; b=FiHfsyCW1X1CIMMLB05rkCvFOIwdDle9zG8VLoA0Kgnii4cmsIyoHYcRjUeT+SlseQO4YCPJp/gq5odkSUAI97yZd4b9rE+/FkHOGfaDZ2vmEroeR3EGEsopChpCFZ+PIXtjvb1FzAsJea0MsY/piARm+pCDEjqsXyXN3qaO4OwJjgAIZmRnRqm2MbQ1KZHUbcrIjE1PIryWUbAfA079VUTzskFZbsY/rEdZ5sH3HZ42KMT66LO441Yfdd2MPPZwn6whZMt6cM1UMM40I7to0GtzZzLGD4UbDQHw81ZbsgZso1nTCgAk2fBP5XLktfb06sFyzzlrdeeCtUw49NJhKw==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=WzcfIc6HY1ys+7UT95Ov6+DrvxC3HDt/g4MDfUTfuwLm364q4ZOT0HCTM8OteXswRfwXopZ6ZbLriiNVSe7jQhZ58ttSyNDmTt3dfr8UDx1f/US/QrWT8PjKuhPai32BJaa45SNLt2FtXM4yM72FD2SmMBfcevSjBhnDVVFmoOpgkfVE3TvFTsLajTcROtPHNmBpqKrEQEpCpdFpwO3yKCq45R/nIETCjKkarjAXc7iNRgd+D3V1faxCT3t+QnBnSa8dOgXalrshuBc7gp7WvxpCYm6hduFA+bFUS7cVB5+odTgXrrP1EmUd6H2t5a7osjEwkzH1Pl+dtVXLuA8yKw==
- Authentication-results: esa5.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>, Paul Durrant <paul@xxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>
- Delivery-date: Tue, 14 Dec 2021 15:15:33 +0000
- Ironport-data: A9a23:u+rty64MCuCRTK0ikoBhJQxRtM/AchMFZxGqfqrLsTDasY5as4F+v mofXm2COPuDY2qjfdh2aIvi9kIBuMDcy98wTwY//CwxHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuV3zyIQUBUjclkfJKlYAL/En03FV8MpBsJ00o5wrdj29Iw27BVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Z4 +pA7ZeKc0AQHaTWssswSzNyOChsMvgTkFPHCSDXXc27ykTHdz3nwul0DVFwNoodkgp1KTgQr 7pCcmlLN03dwbLtqF64YrAEasALNs7kMZlZonh95TrYEewnUdbIRKCiCdpwgWZs15wXRqy2i 8wxUWJCQgvOUkF1GAklF4AlxvmzqWL6SmgNwL6SjfVuuDWCpOBr65D1OcfRUsyHQ4NShEnwj mHL4WX/RA0bPdq3yDyZ/3bqjejK9Qv5Uo8PELyz9tZxnUaegGcUDXU+RVa95PW0lEO6c9ZeM FAPvDojq7Ao806mRcW7WAe3yENopTZFBYAWSbdjrljQlOyEuG51G1ToUBZqV9F+v/UMAgUJ3 0WjsP7xLxZEua+aHCf1GqivkRu+Pi0cLGknbCACTBcY79SLnLzfni4jXf44Tvfr04Sd9SXYh mnT8XNg3+l7Ydsjjv3jpTj6bySQSo8lp+LfziHeRSqb4wxwf+ZJjKT4uAGAvZ6swGt0J2RtX UToeeDCvIji7rnXzURhpdnh+pnzuJ5p1xWG3jZS82EJrWjFxpJaVdk4DMtCDEloKN0YXjTif VXevwhcjLcKYiD7MfUuO9/vVJV6pUQFKTgDfqqLBjapSsIuHDJrAQk0PRLAt4wTuBZEfV4D1 WezLp/3UCdy5VVPxzuqXeYNuYLHNQhlrV4/savTlkz9uZLHPSb9Ye5cbDOmM7FhhIvZ8V692 4sOaKO3J+B3DbSWjt//qtVIczjn7BETWPjLliCgXrLZf1c9Rjh+U6S5LHFIU9UNopm5X9zgp xmVckRZ1ED+lTvALwCLYWpkc7ThQdB0qndTAMDmFQ/AN6ELbdn94aEBWYEweLV7puVvweQtF 6sOetmaA+QJQTPComxPYZ74pY1kVRKqmQPRYHb1PGlhJ8ZtF17T59vpXgrz7y1SXCC5gtQz/ u+73QTBTJtdGwk7VJTKaOiixk+atGQGnL4gRFPBJ9ReIR2+8IVjJyHroOUwJsUAdUfKyjeAj l7EChYEv+jd5YQy9YCR16yDqo6oFcp4H1ZbQDaHverna3GC8zP6k4FaUeuOcTTMb0/O+f2vN bdP0vXxEPwbh1IW4YByJKlmkPAl7Nz1qr4Ekgk9RCfXb06mA69LK2Wd2ZUdrbVEw7JUtFfkW k+L/dUGa7yFNNm8TQwULQshKO+CyesVin/Z6vFseBf24yp+/bymV0ROPkbT1HwBfeUtaI51k /08vMM26hCkjkt4O9mLuSlY6mCQIyFSSK4grJwbXNfmhwdDJouuunAA5vsaOK2yVug=
- Ironport-hdrordr: A9a23:dsj8l6EJrlpGin0NpLqE7MeALOsnbusQ8zAXPidKOHtom62j5q STdZEgviMc5wx8ZJhNo7+90cq7IU80l6Qa3WB5B97LNmTbUQCTTb1K3M/PxCDhBj271sM179 YET0GmMqySMbGtt7eZ3DWF
- Ironport-sdr: ohebrPkhOuPel6yu0druO4yDrXy+hgXfnUlj7as8eJHBdq2xS3iCPV6DWnQRJb9U4kDx99/gG5 AZYAwDrhKie6tsx6hO6k37QE51d24VxOycVrmK59gDJQdrPkq6McrbnpshMPWgcC+jMS0yxiZK uvCkUagMkTntLuW7xuh8EPv6f+LCkyuDcNH0UC5GnQRUfG2haon/F788js6zCFz57af97YEHKp UhqpbYOEs0x2hsTjOTYZK6W+8XFxTVRCQ6cBpy+nJZktLXMUnAWe07y2DHHP8zuTvtu7s+Go/U yrGvcgYLarz/d5nXQvIvozjV
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On Tue, Dec 14, 2021 at 04:05:27PM +0100, Jan Beulich wrote:
> On 14.12.2021 15:50, Roger Pau Monné wrote:
> > On Fri, Sep 24, 2021 at 11:54:58AM +0200, Jan Beulich wrote:
> >> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> >> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> >> @@ -445,6 +445,8 @@ union amd_iommu_x2apic_control {
> >> #define IOMMU_PAGE_TABLE_U32_PER_ENTRY (IOMMU_PAGE_TABLE_ENTRY_SIZE /
> >> 4)
> >> #define IOMMU_PAGE_TABLE_ALIGNMENT 4096
> >>
> >> +#define IOMMU_PTE_CONTIG_MASK 0x1e /* The ign0 field below. */
> >
> > Should you rename ign0 to contig_mask or some such now?
>
> Not sure. I guess I should attach a comment linking here.
>
> > Same would apply to the comment next to dma_pte for VT-d, where bits
> > 52:62 are ignored (the comments seems to be missing this already) and
> > we will be using bits 52:55 to store the contiguous mask for the
> > entry.
>
> Same here then.
I would prefer that.
> >> --- a/xen/drivers/passthrough/amd/iommu_map.c
> >> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> >> @@ -116,7 +116,19 @@ static void set_iommu_ptes_present(unsig
> >>
> >> while ( nr_ptes-- )
> >> {
> >> - set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> >> + ASSERT(!pde->next_level);
> >> + ASSERT(!pde->u);
> >> +
> >> + if ( pde > table )
> >> + ASSERT(pde->ign0 == find_first_set_bit(pde - table));
> >> + else
> >> + ASSERT(pde->ign0 == PAGE_SHIFT - 3);
> >
> > You could even special case (pde - table) % 2 != 0, but this is debug
> > only code, and it's possible a mod is more costly than
> > find_first_set_bit.
>
> Not sure why I would want to special case anything that doesn't need
> special casing. The pde == table case needs special care because there
> find_first_set_bit() cannot be called.
Well in iommu_alloc_pgtable you already special case odd entries by
explicitly setting the mask to 0 instead of using find_first_set_bit.
> >> @@ -450,7 +450,28 @@ struct page_info *iommu_alloc_pgtable(st
> >> return NULL;
> >>
> >> p = __map_domain_page(pg);
> >> - clear_page(p);
> >> +
> >> + if ( contig_mask )
> >> + {
> >> + unsigned int i, shift = find_first_set_bit(contig_mask);
> >> +
> >> + ASSERT(((PAGE_SHIFT - 3) & (contig_mask >> shift)) == PAGE_SHIFT
> >> - 3);
> >> +
> >> + p[0] = (PAGE_SHIFT - 3ull) << shift;
> >> + p[1] = 0;
> >> + p[2] = 1ull << shift;
> >> + p[3] = 0;
> >> +
> >> + for ( i = 4; i < PAGE_SIZE / 8; i += 4 )
> >> + {
> >> + p[i + 0] = (find_first_set_bit(i) + 0ull) << shift;
> >> + p[i + 1] = 0;
> >> + p[i + 2] = 1ull << shift;
> >> + p[i + 3] = 0;
> >> + }
> >
> > You could likely do:
> >
> > for ( i = 0; i < PAGE_SIZE / 8; i += 4 )
> > {
> > p[i + 0] = i ? ((find_first_set_bit(i) + 0ull) << shift)
> > : ((PAGE_SHIFT - 3ull) << shift);
> > p[i + 1] = 0;
> > p[i + 2] = 1ull << shift;
> > p[i + 3] = 0;
> > }
> >
> > To avoid having to open code the first loop iteration.
>
> I could, but I explicitly didn't want to. I consider conditionals
> inside a loop which special case just the first (or last) iteration
> quite odd (unless they really save a lot of duplication).
>
> > The ternary
> > operator could also be nested before the shift, but I find that
> > harder to read.
>
> If I was to make the change, then that alternative way, as it would
> allow to avoid the addition of 0ull:
>
> p[i + 0] = (i ? find_first_set_bit(i)
> : PAGE_SHIFT - 3ull) << shift;
>
> I could be talked into going that route (but not the intermediate
> one you've suggested).
If you prefer to open code the first iteration I'm also fine with
that.
Thanks, Roger.
|