Re: [Xen-devel] [PATCH v7 1/6] iommu: introduce the concept of DFN...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 12 September 2018 13:15
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Kevin Tian <kevin.tian@xxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v7 1/6] iommu: introduce the concept of
> DFN...
> 
> On Wed, Sep 12, 2018 at 12:30:23PM +0100, Paul Durrant wrote:
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index e35d941f3c..19d5d55d79 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -23,11 +23,37 @@
> >  #include <xen/page-defs.h>
> >  #include <xen/spinlock.h>
> >  #include <xen/pci.h>
> > +#include <xen/typesafe.h>
> >  #include <public/hvm/ioreq.h>
> >  #include <public/domctl.h>
> >  #include <asm/device.h>
> >  #include <asm/iommu.h>
> >
> > +TYPE_SAFE(uint64_t, dfn);
> 
> Shouldn't this be unsigned long instead of uint64_t? (like gfn or mfn)
> 

Also, there is no reason why a 32-bit domain can't use h/w capable of 64-bit 
DMA.

> > +#define PRI_dfn     PRIx64
> > +#define INVALID_DFN _dfn(~0UL)
> 
> Or if there's a reason for the above to be uint64_t, then INVALID_DFN
> should use ULL.
> 

Yes, I missed that.

> > +
> > +#ifndef dfn_t
> > +#define dfn_t /* Grep fodder: dfn_t, _dfn() and dfn_x() are defined
> above */
> > +#define _dfn
> > +#define dfn_x
> > +#undef dfn_t
> > +#undef _dfn
> > +#undef dfn_x
> > +#endif
> > +
> > +#define IOMMU_PAGE_SHIFT 12
> > +#define IOMMU_PAGE_SIZE  (_AC(1,L) << IOMMU_PAGE_SHIFT)
> 
> There's no need to use _AC?
> 

Indeed.

> > +#define IOMMU_PAGE_MASK  (~(IOMMU_PAGE_SIZE - 1))
> > +
> > +typedef uint64_t daddr_t;
> 
> Use paddr_t instead of uint64_t directly?
> 

No, I don't think paddr_t is the correct thing to use.

> > +
> > +#define __dfn_to_daddr(dfn) ((daddr_t)(dfn) << IOMMU_PAGE_SHIFT)
> > +#define __daddr_to_dfn(daddr) ((uint64_t)(daddr >>
> IOMMU_PAGE_SHIFT))
> > +
> > +#define dfn_to_daddr(dfn) __dfn_to_daddr(dfn_x(dfn))
> > +#define daddr_to_dfn(daddr) _dfn(__daddr_to_dfn(daddr))
> > +
> >  extern bool_t iommu_enable, iommu_enabled;
> >  extern bool_t force_iommu, iommu_dom0_strict, iommu_verbose;
> >  extern bool_t iommu_workaround_bios_bug, iommu_igfx,
> iommu_passthrough;
> > @@ -60,9 +86,9 @@ void iommu_teardown(struct domain *d);
> >  #define IOMMUF_readable  (1u<<_IOMMUF_readable)
> >  #define _IOMMUF_writable 1
> >  #define IOMMUF_writable  (1u<<_IOMMUF_writable)
> > -int __must_check iommu_map_page(struct domain *d, unsigned long
> gfn,
> > +int __must_check iommu_map_page(struct domain *d, unsigned long
> dfn,
> >                                  unsigned long mfn, unsigned int flags);
> > -int __must_check iommu_unmap_page(struct domain *d, unsigned long
> gfn);
> > +int __must_check iommu_unmap_page(struct domain *d, unsigned long
> dfn);
> >
> >  enum iommu_feature
> >  {
> > @@ -150,9 +176,9 @@ struct iommu_ops {
> >  #endif /* HAS_PCI */
> >
> >      void (*teardown)(struct domain *d);
> > -    int __must_check (*map_page)(struct domain *d, unsigned long gfn,
> > +    int __must_check (*map_page)(struct domain *d, unsigned long dfn,
> >                                   unsigned long mfn, unsigned int flags);
> > -    int __must_check (*unmap_page)(struct domain *d, unsigned long
> gfn);
> > +    int __must_check (*unmap_page)(struct domain *d, unsigned long
> dfn);
> >      void (*free_page_table)(struct page_info *);
> >  #ifdef CONFIG_X86
> >      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg,
> unsigned int value);
> > @@ -163,7 +189,7 @@ struct iommu_ops {
> >      void (*resume)(void);
> >      void (*share_p2m)(struct domain *d);
> >      void (*crash_shutdown)(void);
> > -    int __must_check (*iotlb_flush)(struct domain *d, unsigned long gfn,
> > +    int __must_check (*iotlb_flush)(struct domain *d, unsigned long dfn,
> >                                      unsigned int page_count);
> >      int __must_check (*iotlb_flush_all)(struct domain *d);
> >      int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
> > @@ -185,7 +211,7 @@ int iommu_do_pci_domctl(struct xen_domctl *,
> struct domain *d,
> >  int iommu_do_domctl(struct xen_domctl *, struct domain *d,
> >                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
> >
> > -int __must_check iommu_iotlb_flush(struct domain *d, unsigned long
> gfn,
> > +int __must_check iommu_iotlb_flush(struct domain *d, unsigned long
> dfn,
> 
> While doing this wholesale gfn to dfn replacement, it might be a good
> idea to also change unsigned long for dfn at the same time?
> 

Not a lot of point. I'm going to replace these occurrences with type-safe dfn_t 
in a later patch.

  Paul

> Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel