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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 13 September 2018 13:46
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v8 1/7] iommu: introduce the concept of DFN...
> 
> >>> On 13.09.18 at 12:31, <paul.durrant@xxxxxxxxxx> wrote:
> > ...meaning 'device DMA frame number' i.e. a frame number mapped in the
> IOMMU
> > (rather than the MMU) and hence used for DMA address translation.
> >
> > This patch is a largely cosmetic change that substitutes the terms 'gfn'
> > and 'gaddr' for 'dfn' and 'daddr' in all the places where the frame
> number
> > or address relate to a device rather than the CPU.
> >
> > The parts that are not purely cosmetic are:
> >
> >  - the introduction of a type-safe declaration of dfn_t and definition
> of
> >    INVALID_DFN to make the substitution of gfn_x(INVALID_GFN)
> mechanical.
> >  - the introduction of __dfn_to_daddr and __daddr_to_dfn (and type-safe
> >    variants without the leading __) with some use of the former.
> 
> The latter is rather unfortunate, but presumably unavoidable,
> both because of the name space issue and the fact that the other
> non-type-safe variants are intended to go away as well).
> 
> > --- 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);
> > +#define PRI_dfn     PRIx64
> > +#define INVALID_DFN _dfn(~0ULL)
> > +
> > +#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
> 
> Is this correct for ARM in all cases?

Looks like there's no fixed page size so these may actually be better off in an 
x86 header. They just got pulled here when I made the code common.

> 
> > +#define IOMMU_PAGE_SIZE  (1 << IOMMU_PAGE_SHIFT)
> > +#define IOMMU_PAGE_MASK  (~(IOMMU_PAGE_SIZE - 1))
> > +
> > +typedef uint64_t daddr_t;
> > +
> > +#define __dfn_to_daddr(dfn) ((daddr_t)(dfn) << IOMMU_PAGE_SHIFT)
> > +#define __daddr_to_dfn(daddr) ((uint64_t)(daddr >> IOMMU_PAGE_SHIFT))
> 
> Please could at least the latter of the two casts be omitted? Also
> daddr needed parenthesizing.
> 

Ok.

> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -26,6 +26,11 @@
> >   *   A linear idea of a guest physical address space. For an auto-
> translated
> >   *   guest, pfn == gfn while for a non-translated guest, pfn != gfn.
> >   *
> > + * dfn: Device DMA Frame Number (definitions in include/xen/iommu.h)
> > + *   The linear frame numbers of IOMMU address space. All initiators
> for (i.e.
> > + *   all devices assigned to) a guest share a single IOMMU address
> space and,
> > + *   by default, Xen will ensure dfn == pfn.
> 
> While the names have been changed, the test still talks about "IOMMU
> address space". In particular for the IOMMU there very clear are two
> address spaces (incoming and outgoing), so this needs disambiguation.
> 
> With these minor issues taken care of
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 

Thanks. Do you want me to move the page size definitions and re-submit with the 
other things fixed up?

  Paul

> Jan
> 


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