Re: [PATCH v4 2/8] xen/arm: Introduce a generic way to describe device

[Manish Jaggi <mjaggi@caviumnetworks.com>]

On Thursday 26 February 2015 12:22 AM, Julien Grall wrote:
> Currently, Xen is supporting PCI and Platform device (based on Device Tree).
>
> While Xen only supports Platform device on ARM, Xen will gain support of
> PCI soon.
>
> Some drivers, such as IOMMU drivers, may handle PCI and platform device in
> the same way. Only few lines of code differs.
>
> Rather than requesting to provide 2 set of functions (one for PCI and
> one for platform device), introduce a generic structure "device" which
> is embedded in each specialized device.
>
> As x86 only supports PCI, introduce a new type device_t which will be an
> alias to pci_dev for this architecture. It will avoid to add a new field
> for this place.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>
> ---
>      Changes in v4:
>          - Add Jan and Stefano's ack
>          - Typoes
>
>      Changes in v3:
>          - Drop common/device.c as it wasn't used after v1
>
>      Changes in v2:
>          - As x86 will only support PCI, only introduce the generic
>          device on ARM
>          - Introduce a typedef device_t to be used in common code
>          - Drop the PCI code for ARM for now. It will be reintroduced
>          when PCI support will be added
>          - s#asm/device.h#xen/device.h# is not anymore needed
> ---
>   xen/common/device_tree.c      |  3 +++
>   xen/include/asm-arm/device.h  | 26 ++++++++++++++++++++++++++
>   xen/include/asm-x86/device.h  | 25 +++++++++++++++++++++++++
>   xen/include/xen/device_tree.h | 13 +++++++++++++
>   xen/include/xen/iommu.h       |  1 +
>   xen/include/xen/pci.h         |  1 +
>   6 files changed, 69 insertions(+)
>   create mode 100644 xen/include/asm-x86/device.h
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 34a1b9e..d1c716f 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1454,6 +1454,9 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>               ((char *)pp->value)[sz - 1] = 0;
>               dt_dprintk("fixed up name for %s -> %s\n", pathp,
>                          (char *)pp->value);
> +            /* Generic device initialization */
> +            np->dev.type = DEV_DT;
> +            np->dev.of_node = np;
>           }
>       }
>       if ( allnextpp )
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index b6b32bc..1cedd49 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -2,8 +2,34 @@
>   #define __ASM_ARM_DEVICE_H
>   
>   #include <xen/init.h>
> +
> +enum device_type
> +{
> +    DEV_DT,
> +};
> +
> +struct dev_archdata {
> +    void *iommu;    /* IOMMU private data */
> +};
> +
> +/* struct device - The basic device structure */
> +struct device
> +{
> +    enum device_type type;
> +#ifdef HAS_DEVICE_TREE
> +    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> +#endif
> +    struct dev_archdata archdata;
> +};
> +
> +typedef struct device device_t;
> +
>   #include <xen/device_tree.h>
>   
> +/* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
> +#define dev_is_pci(dev) ((void)(dev), 0)
> +#define dev_is_dt(dev)  ((dev->type == DEV_DT)
> +
>   enum device_class
>   {
>       DEVICE_SERIAL,
> diff --git a/xen/include/asm-x86/device.h b/xen/include/asm-x86/device.h
> new file mode 100644
> index 0000000..f2acc7e
> --- /dev/null
> +++ b/xen/include/asm-x86/device.h
> @@ -0,0 +1,25 @@
> +#ifndef __ASM_X86_DEVICE_H
> +#define __ASM_X86_DEVICE_H
> +
> +#include <xen/pci.h>
> +
> +/*
> + * x86 only supports PCI. Therefore it's possible to directly use
> + * pci_dev to avoid adding new field.
> + */
> +
> +typedef struct pci_dev device_t;
> +
> +#define dev_is_pci(dev) ((void)(dev), 1)
> +#define pci_to_dev(pci) (pci)
When HAS_PCI and HAS_PASSTHROUGH is enabled for ARM, pci_to_dev is 
undefined for ARM and compilation fails.
How do you propose to fix this?

Should I add a device as a member in pci_dev, as you mentioned in the 
commit "... embedded in each specialized device"
> +
> +#endif /* __ASM_X86_DEVICE_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 6502369..c8a0375 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -11,7 +11,9 @@
>   #define __XEN_DEVICE_TREE_H__
>   
>   #include <asm/byteorder.h>
> +#include <asm/device.h>
>   #include <public/xen.h>
> +#include <xen/kernel.h>
>   #include <xen/init.h>
>   #include <xen/string.h>
>   #include <xen/types.h>
> @@ -80,8 +82,19 @@ struct dt_device_node {
>       /* IOMMU specific fields */
>       bool is_protected;
>       struct list_head domain_list;
> +
> +    struct device dev;
>   };
>   
> +#define dt_to_dev(dt_node)  (&(dt_node)->dev)
> +
> +static inline struct dt_device_node *dev_to_dt(struct device *dev)
> +{
> +    ASSERT(dev->type == DEV_DT);
> +
> +    return container_of(dev, struct dt_device_node, dev);
> +}
> +
>   #define MAX_PHANDLE_ARGS 16
>   struct dt_phandle_args {
>       struct dt_device_node *np;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 8eb764a..ecb2627 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -25,6 +25,7 @@
>   #include <xen/pci.h>
>   #include <public/hvm/ioreq.h>
>   #include <public/domctl.h>
> +#include <asm/device.h>
>   #include <asm/iommu.h>
>   
>   extern bool_t iommu_enable, iommu_enabled;
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 5f295f3..3988ee68 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -13,6 +13,7 @@
>   #include <xen/irq.h>
>   #include <xen/pci_regs.h>
>   #include <xen/pfn.h>
> +#include <asm/device.h>
>   #include <asm/pci.h>
>   
>   /*