| Message ID | 20200804134209.8717-15-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.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1772,13 +1772,14 @@ static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn, > old = *pte; > > dma_set_pte_addr(new, mfn_to_maddr(mfn)); > - dma_set_pte_prot(new, > - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | > - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); > + if ( flags & IOMMUF_readable ) > + dma_set_pte_readable(new); > + if ( flags & IOMMUF_writable ) > + dma_set_pte_writable(new); > > /* Set the SNP on leaf page table if Snoop Control available */ > if ( iommu_snoop ) > - dma_set_pte_snp(new); > + dma_set_pte_snoop(new); Perhaps simply use an initializer: new = (struct dma_ptr){ .r = flags & IOMMUF_readable, .w = flags & IOMMUF_writable, .snp = iommu_snoop, .addr = mfn_x(mfn), }; ? This also points out that the "addr" field isn't really an address, and hence may want renaming. Again comments on the two earlier patch apply here respectively (or else part of the suggestion above isn't going to work as is). Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 06 August 2020 13:54 > To: Paul Durrant <paul@xen.org> > Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Kevin Tian > <kevin.tian@intel.com> > Subject: Re: [PATCH v4 14/14] vtd: use a bit field for dma_pte > > On 04.08.2020 15:42, Paul Durrant wrote: > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -1772,13 +1772,14 @@ static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn, > > old = *pte; > > > > dma_set_pte_addr(new, mfn_to_maddr(mfn)); > > - dma_set_pte_prot(new, > > - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | > > - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); > > + if ( flags & IOMMUF_readable ) > > + dma_set_pte_readable(new); > > + if ( flags & IOMMUF_writable ) > > + dma_set_pte_writable(new); > > > > /* Set the SNP on leaf page table if Snoop Control available */ > > if ( iommu_snoop ) > > - dma_set_pte_snp(new); > > + dma_set_pte_snoop(new); > > Perhaps simply use an initializer: > > new = (struct dma_ptr){ > .r = flags & IOMMUF_readable, > .w = flags & IOMMUF_writable, > .snp = iommu_snoop, > .addr = mfn_x(mfn), > }; > > ? This also points out that the "addr" field isn't really an address, > and hence may want renaming. If I am getting rid of macros then this makes more sense. Paul > > Again comments on the two earlier patch apply here respectively (or > else part of the suggestion above isn't going to work as is). > > Jan
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 766d33058e..2d60cebe67 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1772,13 +1772,14 @@ static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn, old = *pte; dma_set_pte_addr(new, mfn_to_maddr(mfn)); - dma_set_pte_prot(new, - ((flags & IOMMUF_readable) ? DMA_PTE_READ : 0) | - ((flags & IOMMUF_writable) ? DMA_PTE_WRITE : 0)); + if ( flags & IOMMUF_readable ) + dma_set_pte_readable(new); + if ( flags & IOMMUF_writable ) + dma_set_pte_writable(new); /* Set the SNP on leaf page table if Snoop Control available */ if ( iommu_snoop ) - dma_set_pte_snp(new); + dma_set_pte_snoop(new); if ( old.val == new.val ) { diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h index 509d13918a..017286b0e1 100644 --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -283,29 +283,40 @@ struct context_entry { * 12-63: Host physcial address */ struct dma_pte { - u64 val; + union { + uint64_t val; + struct { + uint64_t r:1; + uint64_t w:1; + uint64_t reserved0:1; + uint64_t ignored0:4; + uint64_t ps:1; + uint64_t ignored1:3; + uint64_t snp:1; + uint64_t addr:52; + }; + }; }; -#define DMA_PTE_READ (1) -#define DMA_PTE_WRITE (2) -#define DMA_PTE_PROT (DMA_PTE_READ | DMA_PTE_WRITE) -#define DMA_PTE_SP (1 << 7) -#define DMA_PTE_SNP (1 << 11) -#define dma_clear_pte(p) do {(p).val = 0;} while(0) -#define dma_set_pte_readable(p) do {(p).val |= DMA_PTE_READ;} while(0) -#define dma_set_pte_writable(p) do {(p).val |= DMA_PTE_WRITE;} while(0) -#define dma_set_pte_superpage(p) do {(p).val |= DMA_PTE_SP;} while(0) -#define dma_set_pte_snp(p) do {(p).val |= DMA_PTE_SNP;} while(0) -#define dma_set_pte_prot(p, prot) do { \ - (p).val = ((p).val & ~DMA_PTE_PROT) | ((prot) & DMA_PTE_PROT); \ - } while (0) -#define dma_pte_prot(p) ((p).val & DMA_PTE_PROT) -#define dma_pte_read(p) (dma_pte_prot(p) & DMA_PTE_READ) -#define dma_pte_write(p) (dma_pte_prot(p) & DMA_PTE_WRITE) -#define dma_pte_addr(p) ((p).val & PADDR_MASK & PAGE_MASK_4K) -#define dma_set_pte_addr(p, addr) do {\ - (p).val |= ((addr) & PAGE_MASK_4K); } while (0) -#define dma_pte_present(p) (((p).val & DMA_PTE_PROT) != 0) -#define dma_pte_superpage(p) (((p).val & DMA_PTE_SP) != 0) + +#define dma_pte_read(p) ((p).r) +#define dma_set_pte_readable(p) do { (p).r = 1; } while (0) + +#define dma_pte_write(p) ((p).w) +#define dma_set_pte_writable(p) do { (p).w = 1; } while (0) + +#define dma_pte_addr(p) ((p).addr << PAGE_SHIFT_4K) +#define dma_set_pte_addr(p, val) \ + do { (p).addr = (val) >> PAGE_SHIFT_4K; } while (0) + +#define dma_pte_present(p) ((p).r || (p).w) + +#define dma_pte_superpage(p) ((p).ps) +#define dma_set_pte_superpage(p) do { (p).ps = 1; } while (0) + +#define dma_pte_snoop(p) ((p).snp) +#define dma_set_pte_snoop(p) do { (p).snp = 1; } while (0) + +#define dma_clear_pte(p) do { (p).val = 0; } while (0) /* interrupt remap entry */ struct iremap_entry {