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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +#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.

> --- 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>

Jan



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