RE: [PATCH v4 14/14] vtd: use a bit field for dma_pte

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 06 August 2020 13:54
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; Kevin 
> Tian
> <kevin.tian@xxxxxxxxx>
> 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