| Message ID | 20200804134209.8717-13-paul@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU cleanup | expand |
On 04.08.2020 15:42, Paul Durrant wrote: > --- a/xen/drivers/passthrough/vtd/iommu.h > +++ b/xen/drivers/passthrough/vtd/iommu.h > @@ -184,21 +184,28 @@ > #define dma_frcd_source_id(c) (c & 0xffff) > #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */ > > -/* > - * 0: Present > - * 1-11: Reserved > - * 12-63: Context Ptr (12 - (haw-1)) > - * 64-127: Reserved > - */ > struct root_entry { > - u64 val; > - u64 rsvd1; > + union { > + __uint128_t val; I couldn't find a use of this field, and I also can't foresee any. Could it be left out? > + struct { uint64_t lo, hi; }; > + struct { > + /* 0 - 63 */ > + uint64_t p:1; bool? > + uint64_t reserved0:11; > + uint64_t ctp:52; > + > + /* 64 - 127 */ > + uint64_t reserved1; > + }; > + }; > }; > -#define root_present(root) ((root).val & 1) > -#define set_root_present(root) do {(root).val |= 1;} while(0) > -#define get_context_addr(root) ((root).val & PAGE_MASK_4K) > -#define set_root_value(root, value) \ > - do {(root).val |= ((value) & PAGE_MASK_4K);} while(0) > + > +#define root_present(r) (r).p > +#define set_root_present(r) do { (r).p = 1; } while (0) And then "true" here? > +#define root_ctp(r) ((r).ctp << PAGE_SHIFT_4K) > +#define set_root_ctp(r, val) \ > + do { (r).ctp = ((val) >> PAGE_SHIFT_4K); } while (0) For documentation purposes, can the 2nd macro param be named maddr or some such? > --- a/xen/drivers/passthrough/vtd/x86/ats.c > +++ b/xen/drivers/passthrough/vtd/x86/ats.c > @@ -74,8 +74,8 @@ int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd) > static bool device_in_domain(const struct vtd_iommu *iommu, > const struct pci_dev *pdev, uint16_t did) > { > - struct root_entry *root_entry; > - struct context_entry *ctxt_entry = NULL; > + struct root_entry *root_entry, *root_entries = NULL; > + struct context_entry *context_entry, *context_entries = NULL; Just like root_entry, root_entries doesn't look to need an initializer. I'm unconvinced anyway that you now need two variables each: unmap_vtd_domain_page() does quite fine with the low 12 bits not all being zero, afaict. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 06 August 2020 13:34 > To: Paul Durrant <paul@xen.org> > Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Kevin Tian > <kevin.tian@intel.com> > Subject: RE: [EXTERNAL] [PATCH v4 12/14] vtd: use a bit field for root_entry > > CAUTION: This email originated from outside of the organization. Do not click links or open > attachments unless you can confirm the sender and know the content is safe. > > > > On 04.08.2020 15:42, Paul Durrant wrote: > > --- a/xen/drivers/passthrough/vtd/iommu.h > > +++ b/xen/drivers/passthrough/vtd/iommu.h > > @@ -184,21 +184,28 @@ > > #define dma_frcd_source_id(c) (c & 0xffff) > > #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */ > > > > -/* > > - * 0: Present > > - * 1-11: Reserved > > - * 12-63: Context Ptr (12 - (haw-1)) > > - * 64-127: Reserved > > - */ > > struct root_entry { > > - u64 val; > > - u64 rsvd1; > > + union { > > + __uint128_t val; > > I couldn't find a use of this field, and I also can't foresee any. > Could it be left out? Yes, probably. > > > + struct { uint64_t lo, hi; }; > > + struct { > > + /* 0 - 63 */ > > + uint64_t p:1; > > bool? > I'd prefer not to. One of the points of using a bit field (at least from my PoV) is that it makes referring back to the spec. much easier, by using uint64_t types consistently and hence using bit widths that can be straightforwardly summed to give the bit offsets stated in the spec. > > + uint64_t reserved0:11; > > + uint64_t ctp:52; > > + > > + /* 64 - 127 */ > > + uint64_t reserved1; > > + }; > > + }; > > }; > > -#define root_present(root) ((root).val & 1) > > -#define set_root_present(root) do {(root).val |= 1;} while(0) > > -#define get_context_addr(root) ((root).val & PAGE_MASK_4K) > > -#define set_root_value(root, value) \ > > - do {(root).val |= ((value) & PAGE_MASK_4K);} while(0) > > + > > +#define root_present(r) (r).p > > +#define set_root_present(r) do { (r).p = 1; } while (0) > > And then "true" here? > > > +#define root_ctp(r) ((r).ctp << PAGE_SHIFT_4K) > > +#define set_root_ctp(r, val) \ > > + do { (r).ctp = ((val) >> PAGE_SHIFT_4K); } while (0) > > For documentation purposes, can the 2nd macro param be named maddr > or some such? > Sure. > > --- a/xen/drivers/passthrough/vtd/x86/ats.c > > +++ b/xen/drivers/passthrough/vtd/x86/ats.c > > @@ -74,8 +74,8 @@ int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd) > > static bool device_in_domain(const struct vtd_iommu *iommu, > > const struct pci_dev *pdev, uint16_t did) > > { > > - struct root_entry *root_entry; > > - struct context_entry *ctxt_entry = NULL; > > + struct root_entry *root_entry, *root_entries = NULL; > > + struct context_entry *context_entry, *context_entries = NULL; > > Just like root_entry, root_entries doesn't look to need an initializer. > I'm unconvinced anyway that you now need two variables each: > unmap_vtd_domain_page() does quite fine with the low 12 bits not all > being zero, afaict. Not passing a page aligned address into something that unmaps a page seems a little bit fragile though, e.g. if someone happened to add a check in future. I'll see if I can drop the initializer though. Paul > > Jan
> From: Paul Durrant <paul@xen.org> > Sent: Tuesday, August 4, 2020 9:42 PM > > From: Paul Durrant <pdurrant@amazon.com> > > This makes the code a little easier to read and also makes it more consistent > with iremap_entry. I feel the original readability is slightly better, as ctp is less obvious than set_root_value, get_context_addr, etc. Thanks Kevin > > Also take the opportunity to tidy up the implementation of > device_in_domain(). > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Kevin Tian <kevin.tian@intel.com> > > v4: > - New in v4 > --- > xen/drivers/passthrough/vtd/iommu.c | 4 ++-- > xen/drivers/passthrough/vtd/iommu.h | 33 ++++++++++++++++----------- > xen/drivers/passthrough/vtd/utils.c | 4 ++-- > xen/drivers/passthrough/vtd/x86/ats.c | 27 ++++++++++++---------- > 4 files changed, 39 insertions(+), 29 deletions(-) > > diff --git a/xen/drivers/passthrough/vtd/iommu.c > b/xen/drivers/passthrough/vtd/iommu.c > index f8da4fe0e7..76025f6ccd 100644 > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -245,11 +245,11 @@ static u64 bus_to_context_maddr(struct > vtd_iommu *iommu, u8 bus) > unmap_vtd_domain_page(root_entries); > return 0; > } > - set_root_value(*root, maddr); > + set_root_ctp(*root, maddr); > set_root_present(*root); > iommu_sync_cache(root, sizeof(struct root_entry)); > } > - maddr = (u64) get_context_addr(*root); > + maddr = root_ctp(*root); > unmap_vtd_domain_page(root_entries); > return maddr; > } > diff --git a/xen/drivers/passthrough/vtd/iommu.h > b/xen/drivers/passthrough/vtd/iommu.h > index 216791b3d6..031ac5f66c 100644 > --- a/xen/drivers/passthrough/vtd/iommu.h > +++ b/xen/drivers/passthrough/vtd/iommu.h > @@ -184,21 +184,28 @@ > #define dma_frcd_source_id(c) (c & 0xffff) > #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */ > > -/* > - * 0: Present > - * 1-11: Reserved > - * 12-63: Context Ptr (12 - (haw-1)) > - * 64-127: Reserved > - */ > struct root_entry { > - u64 val; > - u64 rsvd1; > + union { > + __uint128_t val; > + struct { uint64_t lo, hi; }; > + struct { > + /* 0 - 63 */ > + uint64_t p:1; > + uint64_t reserved0:11; > + uint64_t ctp:52; > + > + /* 64 - 127 */ > + uint64_t reserved1; > + }; > + }; > }; > -#define root_present(root) ((root).val & 1) > -#define set_root_present(root) do {(root).val |= 1;} while(0) > -#define get_context_addr(root) ((root).val & PAGE_MASK_4K) > -#define set_root_value(root, value) \ > - do {(root).val |= ((value) & PAGE_MASK_4K);} while(0) > + > +#define root_present(r) (r).p > +#define set_root_present(r) do { (r).p = 1; } while (0) > + > +#define root_ctp(r) ((r).ctp << PAGE_SHIFT_4K) > +#define set_root_ctp(r, val) \ > + do { (r).ctp = ((val) >> PAGE_SHIFT_4K); } while (0) > > struct context_entry { > u64 lo; > diff --git a/xen/drivers/passthrough/vtd/utils.c > b/xen/drivers/passthrough/vtd/utils.c > index 4febcf506d..4c85242894 100644 > --- a/xen/drivers/passthrough/vtd/utils.c > +++ b/xen/drivers/passthrough/vtd/utils.c > @@ -112,7 +112,7 @@ void print_vtd_entries(struct vtd_iommu *iommu, int > bus, int devfn, u64 gmfn) > return; > } > > - printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].val); > + printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].lo); > if ( !root_present(root_entry[bus]) ) > { > unmap_vtd_domain_page(root_entry); > @@ -120,7 +120,7 @@ void print_vtd_entries(struct vtd_iommu *iommu, int > bus, int devfn, u64 gmfn) > return; > } > > - val = root_entry[bus].val; > + val = root_ctp(root_entry[bus]); > unmap_vtd_domain_page(root_entry); > ctxt_entry = map_vtd_domain_page(val); > if ( ctxt_entry == NULL ) > diff --git a/xen/drivers/passthrough/vtd/x86/ats.c > b/xen/drivers/passthrough/vtd/x86/ats.c > index 04d702b1d6..8369415dcc 100644 > --- a/xen/drivers/passthrough/vtd/x86/ats.c > +++ b/xen/drivers/passthrough/vtd/x86/ats.c > @@ -74,8 +74,8 @@ int ats_device(const struct pci_dev *pdev, const struct > acpi_drhd_unit *drhd) > static bool device_in_domain(const struct vtd_iommu *iommu, > const struct pci_dev *pdev, uint16_t did) > { > - struct root_entry *root_entry; > - struct context_entry *ctxt_entry = NULL; > + struct root_entry *root_entry, *root_entries = NULL; > + struct context_entry *context_entry, *context_entries = NULL; > unsigned int tt; > bool found = false; > > @@ -85,25 +85,28 @@ static bool device_in_domain(const struct > vtd_iommu *iommu, > return false; > } > > - root_entry = map_vtd_domain_page(iommu->root_maddr); > - if ( !root_present(root_entry[pdev->bus]) ) > + root_entries = (struct root_entry *)map_vtd_domain_page(iommu- > >root_maddr); > + root_entry = &root_entries[pdev->bus]; > + if ( !root_present(*root_entry) ) > goto out; > > - ctxt_entry = map_vtd_domain_page(root_entry[pdev->bus].val); > - if ( context_domain_id(ctxt_entry[pdev->devfn]) != did ) > + context_entries = map_vtd_domain_page(root_ctp(*root_entry)); > + context_entry = &context_entries[pdev->devfn]; > + if ( context_domain_id(*context_entry) != did ) > goto out; > > - tt = context_translation_type(ctxt_entry[pdev->devfn]); > + tt = context_translation_type(*context_entry); > if ( tt != CONTEXT_TT_DEV_IOTLB ) > goto out; > > found = true; > -out: > - if ( root_entry ) > - unmap_vtd_domain_page(root_entry); > > - if ( ctxt_entry ) > - unmap_vtd_domain_page(ctxt_entry); > + out: > + if ( root_entries ) > + unmap_vtd_domain_page(root_entries); > + > + if ( context_entries ) > + unmap_vtd_domain_page(context_entries); > > return found; > } > -- > 2.20.1
On 12.08.2020 15:13, Durrant, Paul wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 06 August 2020 13:34 >> >> On 04.08.2020 15:42, Paul Durrant wrote: >>> --- a/xen/drivers/passthrough/vtd/iommu.h >>> +++ b/xen/drivers/passthrough/vtd/iommu.h >>> @@ -184,21 +184,28 @@ >>> #define dma_frcd_source_id(c) (c & 0xffff) >>> #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */ >>> >>> -/* >>> - * 0: Present >>> - * 1-11: Reserved >>> - * 12-63: Context Ptr (12 - (haw-1)) >>> - * 64-127: Reserved >>> - */ >>> struct root_entry { >>> - u64 val; >>> - u64 rsvd1; >>> + union { >>> + __uint128_t val; >> >> I couldn't find a use of this field, and I also can't foresee any. >> Could it be left out? > > Yes, probably. > >> >>> + struct { uint64_t lo, hi; }; >>> + struct { >>> + /* 0 - 63 */ >>> + uint64_t p:1; >> >> bool? >> > > I'd prefer not to. One of the points of using a bit field (at least from my PoV) is that it makes referring back to the spec. much easier, by using uint64_t types consistently and hence using bit widths that can be straightforwardly summed to give the bit offsets stated in the spec. We've gone the suggested route for earlier struct conversions on the AMD side, so I think we should follow suit here. See e.g. struct amd_iommu_dte or union amd_iommu_control. >>> --- a/xen/drivers/passthrough/vtd/x86/ats.c >>> +++ b/xen/drivers/passthrough/vtd/x86/ats.c >>> @@ -74,8 +74,8 @@ int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd) >>> static bool device_in_domain(const struct vtd_iommu *iommu, >>> const struct pci_dev *pdev, uint16_t did) >>> { >>> - struct root_entry *root_entry; >>> - struct context_entry *ctxt_entry = NULL; >>> + struct root_entry *root_entry, *root_entries = NULL; >>> + struct context_entry *context_entry, *context_entries = NULL; >> >> Just like root_entry, root_entries doesn't look to need an initializer. >> I'm unconvinced anyway that you now need two variables each: >> unmap_vtd_domain_page() does quite fine with the low 12 bits not all >> being zero, afaict. > > Not passing a page aligned address into something that unmaps a page seems a little bit fragile though, e.g. if someone happened to add a check in future. There are quite a few existing callers passing a not-page-aligned address into unmap_domain_page(). I don't see why having one more instance would cause any kind of issue. Jan
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index f8da4fe0e7..76025f6ccd 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -245,11 +245,11 @@ static u64 bus_to_context_maddr(struct vtd_iommu *iommu, u8 bus) unmap_vtd_domain_page(root_entries); return 0; } - set_root_value(*root, maddr); + set_root_ctp(*root, maddr); set_root_present(*root); iommu_sync_cache(root, sizeof(struct root_entry)); } - maddr = (u64) get_context_addr(*root); + maddr = root_ctp(*root); unmap_vtd_domain_page(root_entries); return maddr; } diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h index 216791b3d6..031ac5f66c 100644 --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -184,21 +184,28 @@ #define dma_frcd_source_id(c) (c & 0xffff) #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */ -/* - * 0: Present - * 1-11: Reserved - * 12-63: Context Ptr (12 - (haw-1)) - * 64-127: Reserved - */ struct root_entry { - u64 val; - u64 rsvd1; + union { + __uint128_t val; + struct { uint64_t lo, hi; }; + struct { + /* 0 - 63 */ + uint64_t p:1; + uint64_t reserved0:11; + uint64_t ctp:52; + + /* 64 - 127 */ + uint64_t reserved1; + }; + }; }; -#define root_present(root) ((root).val & 1) -#define set_root_present(root) do {(root).val |= 1;} while(0) -#define get_context_addr(root) ((root).val & PAGE_MASK_4K) -#define set_root_value(root, value) \ - do {(root).val |= ((value) & PAGE_MASK_4K);} while(0) + +#define root_present(r) (r).p +#define set_root_present(r) do { (r).p = 1; } while (0) + +#define root_ctp(r) ((r).ctp << PAGE_SHIFT_4K) +#define set_root_ctp(r, val) \ + do { (r).ctp = ((val) >> PAGE_SHIFT_4K); } while (0) struct context_entry { u64 lo; diff --git a/xen/drivers/passthrough/vtd/utils.c b/xen/drivers/passthrough/vtd/utils.c index 4febcf506d..4c85242894 100644 --- a/xen/drivers/passthrough/vtd/utils.c +++ b/xen/drivers/passthrough/vtd/utils.c @@ -112,7 +112,7 @@ void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64 gmfn) return; } - printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].val); + printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].lo); if ( !root_present(root_entry[bus]) ) { unmap_vtd_domain_page(root_entry); @@ -120,7 +120,7 @@ void print_vtd_entries(struct vtd_iommu *iommu, int bus, int devfn, u64 gmfn) return; } - val = root_entry[bus].val; + val = root_ctp(root_entry[bus]); unmap_vtd_domain_page(root_entry); ctxt_entry = map_vtd_domain_page(val); if ( ctxt_entry == NULL ) diff --git a/xen/drivers/passthrough/vtd/x86/ats.c b/xen/drivers/passthrough/vtd/x86/ats.c index 04d702b1d6..8369415dcc 100644 --- a/xen/drivers/passthrough/vtd/x86/ats.c +++ b/xen/drivers/passthrough/vtd/x86/ats.c @@ -74,8 +74,8 @@ int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd) static bool device_in_domain(const struct vtd_iommu *iommu, const struct pci_dev *pdev, uint16_t did) { - struct root_entry *root_entry; - struct context_entry *ctxt_entry = NULL; + struct root_entry *root_entry, *root_entries = NULL; + struct context_entry *context_entry, *context_entries = NULL; unsigned int tt; bool found = false; @@ -85,25 +85,28 @@ static bool device_in_domain(const struct vtd_iommu *iommu, return false; } - root_entry = map_vtd_domain_page(iommu->root_maddr); - if ( !root_present(root_entry[pdev->bus]) ) + root_entries = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr); + root_entry = &root_entries[pdev->bus]; + if ( !root_present(*root_entry) ) goto out; - ctxt_entry = map_vtd_domain_page(root_entry[pdev->bus].val); - if ( context_domain_id(ctxt_entry[pdev->devfn]) != did ) + context_entries = map_vtd_domain_page(root_ctp(*root_entry)); + context_entry = &context_entries[pdev->devfn]; + if ( context_domain_id(*context_entry) != did ) goto out; - tt = context_translation_type(ctxt_entry[pdev->devfn]); + tt = context_translation_type(*context_entry); if ( tt != CONTEXT_TT_DEV_IOTLB ) goto out; found = true; -out: - if ( root_entry ) - unmap_vtd_domain_page(root_entry); - if ( ctxt_entry ) - unmap_vtd_domain_page(ctxt_entry); + out: + if ( root_entries ) + unmap_vtd_domain_page(root_entries); + + if ( context_entries ) + unmap_vtd_domain_page(context_entries); return found; }