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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 12 September 2018 12:49
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: 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 12/09/18 12:30, 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);
> > +#define PRI_dfn     PRIx64
> > +#define INVALID_DFN _dfn(~0UL)
> 
> Presumably the use uint64_t as opposed to unsigned long is for ARM32's
> benefit?

I've lost track as I've chopped and changed this series so many times now. 
That's probably reason.

> 
> If so, INVALID_DFN needs to be ~0ULL to work as intended.
> 

Ok.

> > +
> > +#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)
> 
> "1, L", but this shouldn't need any _AC() at all.  1 << 12 is signed and
> fits within an int so should be fine on its own.
> 

Ok. I'll drop is. The lack of space was just from precedent in page.h.

> > +#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))
> > +
> > +#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;
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> > index 24654e8e22..288dc77b85 100644
> > --- 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.
> 
> ITYM gfn here?  PV-IOMMU is what a PV guest can use to program the
> IOMMU
> with dfn = pfn, but that isn't the default in Xen.
> 

It is the default. For a PV guest Xen will ensure there is a 1:1 mfn map. For a 
PV guest pfn = mfn (unless I'm misinterpreting the comment just above this one) 
so dfn == pfn. Simalarly, for an HVM guest Xen will ensure a 1:1 gfn map. For 
an HVM guest pfn = gfn so, again, dfn = pfn.

  Paul

> ~Andrew

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