[PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Manish Jaggi]

Add ARM PCI Support
---------------
a) Place holder functions are added for pci_conf_read/write calls.
b) Macros dev_is_pci, pci_to_dev are implemented in
drivers/passthrough/pci/arm code

Signed-off-by: Manish Jaggi <manish.jaggi@caviumnetworks.com>
---
  xen/arch/arm/Makefile                |    1 +
  xen/arch/arm/pci.c                   |   60 +++++++++++++++++++++++
  xen/drivers/passthrough/arm/Makefile |    1 +
  xen/drivers/passthrough/arm/pci.c    |   88 ++++++++++++++++++++++++++++++++++
  xen/drivers/passthrough/arm/smmu.c   |    1 -
  xen/drivers/passthrough/pci.c        |    9 ++--
  xen/include/asm-arm/device.h         |   33 +++++++++----
  xen/include/asm-arm/domain.h         |    3 ++
  xen/include/asm-arm/pci.h            |    7 +--
  9 files changed, 186 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 935999e..aaf5d88 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -39,6 +39,7 @@ obj-y += device.o
  obj-y += decode.o
  obj-y += processor.o
  obj-y += smc.o
+obj-$(HAS_PCI) += pci.o
  
  #obj-bin-y += ....o
  
diff --git a/xen/arch/arm/pci.c b/xen/arch/arm/pci.c
new file mode 100644
index 0000000..9438f41
--- /dev/null
+++ b/xen/arch/arm/pci.c
@@ -0,0 +1,60 @@
+#include <xen/pci.h>
+
+void _pci_conf_write(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg, uint32_t data, int bytes)
+{
+}
+
+uint32_t _pci_conf_read(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg, int bytes)
+{
+    return 0;
+}
+
+uint8_t pci_conf_read8(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg)
+{
+    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 1);
+}
+
+uint16_t pci_conf_read16(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg)
+{
+    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 2);
+}
+
+uint32_t pci_conf_read32(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg)
+{
+    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 4);
+}
+
+void pci_conf_write8(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg, uint8_t data)
+{
+     _pci_conf_write(seg, bus, dev, func, reg, data, 1);
+}
+
+void pci_conf_write16(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg, uint16_t data)
+{
+     _pci_conf_write(seg, bus, dev, func, reg, data, 2);
+}
+
+void pci_conf_write32(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg, uint32_t data)
+{
+     _pci_conf_write(seg, bus, dev, func, reg, data, 4);
+}
+
+void arch_pci_ro_device(int seg, int bdf)
+{
+}
diff --git a/xen/drivers/passthrough/arm/Makefile b/xen/drivers/passthrough/arm/Makefile
index f4cd26e..1a41549 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,3 @@
  obj-y += iommu.o
  obj-y += smmu.o
+obj-$(HAS_PCI) += pci.o
diff --git a/xen/drivers/passthrough/arm/pci.c b/xen/drivers/passthrough/arm/pci.c
new file mode 100644
index 0000000..07a5a78
--- /dev/null
+++ b/xen/drivers/passthrough/arm/pci.c
@@ -0,0 +1,88 @@
+/*
+ * PCI helper functions for arm for passthrough support.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) 2015 Cavium Networks
+ *
+ * Author: Manish Jaggi <manish.jaggi@cavium.com>
+ */
+#include <xen/pci.h>
+#include <asm/device.h>
+#include <xen/sched.h>
+
+int _dump_pci_devices(struct pci_seg *pseg, void *arg)
+{
+    struct pci_dev *pdev;
+
+    printk("==== segment %04x ====\n", pseg->nr);
+
+    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+    {
+        printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ",
+               pseg->nr, pdev->bus,
+               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
+               pdev->domain ? pdev->domain->domain_id : -1,
+               (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
+        printk(">\n");
+    }
+
+    return 0;
+}
+
+struct device* pci_to_dev(struct pci_dev *pci)
+{
+    if(!pci->arch.dev)
+    {
+        device_t *dev;
+        dev = xzalloc (struct device);
+        pci->arch.dev = dev;
+        dev->class = DEVICE_PCI;
+        dev->type = DEV_ENUMERATED;
+    }
+    return pci->arch.dev;
+}
+
+struct pci_dev* to_pci_dev(struct device *dev)
+{
+    if(dev->class == DEVICE_PCI)
+        return dev->pci_dev;
+    return NULL;
+}
+
+int dev_is_pci(struct device *dev)
+{
+    if(dev->class == DEVICE_PCI)
+        return 1;
+    return 0;
+}
+
+
+int pci_clean_dpci_irqs(struct domain *d)
+{
+    /* This is a placeholder function */
+    return 0;
+}
+
+struct pci_dev* pci_alloc_msix(struct pci_dev *pdev, struct pci_seg  *pseg,
+                               u8 bus, u8 devfn)
+{
+    /* This is a placeholder function. It is implemented only on x86 */
+    return 0;
+}
+
+void pci_cleanup_msi(struct pci_dev *pdev)
+{
+    /* This is a placeholder function. It is implemented only on x86 */
+}
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8a9b58b..1406261 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -183,7 +183,6 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
   * Xen: PCI functions
   * TODO: It should be implemented when PCI will be supported
   */
-#define to_pci_dev(dev)	(NULL)
  static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
  					 int (*fn) (struct pci_dev *pdev,
  						    u16 alias, void *data),
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 004aba9..68c292b 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1252,9 +1252,12 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
      /* Prevent device assign if mem paging or mem sharing have been
       * enabled for this domain */
      if ( unlikely(!need_iommu(d) &&
-            (d->arch.hvm_domain.mem_sharing_enabled ||
-             d->mem_event->paging.ring_page ||
-             p2m_get_hostp2m(d)->global_logdirty)) )
+            (d->mem_event->paging.ring_page
+#ifdef CONFIG_X86
+             || d->arch.hvm_domain.mem_sharing_enabled ||
+             p2m_get_hostp2m(d)->global_logdirty
+#endif
+            )) )
          return -EXDEV;
  
      if ( !spin_trylock(&pcidevs_lock) )
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index a72f7c9..009ff0a 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -6,6 +6,17 @@
  enum device_type
  {
      DEV_DT,
+    DEV_ENUMERATED,
+};
+
+enum device_class
+{
+    DEVICE_SERIAL,
+    DEVICE_IOMMU,
+    DEVICE_GIC,
+    DEVICE_PCI,
+    /* Use for error */
+    DEVICE_UNKNOWN,
  };
  
  struct dev_archdata {
@@ -16,28 +27,30 @@ struct dev_archdata {
  struct device
  {
      enum device_type type;
+    enum device_class class;
  #ifdef HAS_DEVICE_TREE
      struct dt_device_node *of_node; /* Used by drivers imported from Linux */
  #endif
      struct dev_archdata archdata;
+#ifdef HAS_PCI
+    void *pci_dev;
+#endif
  };
  
  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,
-    DEVICE_IOMMU,
-    DEVICE_GIC,
-    /* Use for error */
-    DEVICE_UNKNOWN,
-};
+struct pci_dev;
+device_t* pci_to_dev(struct pci_dev *pci);
+#ifndef HAS_PCI
+#define to_pci_dev(dev) (NULL)
+#else
+struct pci_dev* to_pci_dev(device_t *dev);
+#endif
+int dev_is_pci(device_t *dev);
  
  struct device_desc {
      /* Device name */
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 9e0419e..41ae847 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -42,6 +42,8 @@ struct vtimer {
          uint64_t cval;
  };
  
+#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
+
  struct arch_domain
  {
  #ifdef CONFIG_ARM_64
@@ -56,6 +58,7 @@ struct arch_domain
      xen_pfn_t *grant_table_gpfn;
  
      struct io_handler io_handlers;
+    struct list_head pdev_list;
      /* Continuable domain_relinquish_resources(). */
      enum {
          RELMEM_not_started,
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index de13359..b8ec882 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -1,7 +1,8 @@
-#ifndef __X86_PCI_H__
-#define __X86_PCI_H__
+#ifndef __ARM_PCI_H__
+#define __ARM_PCI_H__
  
  struct arch_pci_dev {
+    void *dev;
  };
  
-#endif /* __X86_PCI_H__ */
+#endif /* __ARM_PCI_H__ */
-- 
1.7.9.5

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Stefano Stabellini]

On Mon, 13 Apr 2015, Manish Jaggi wrote:
> Add ARM PCI Support
> ---------------
> a) Place holder functions are added for pci_conf_read/write calls.
> b) Macros dev_is_pci, pci_to_dev are implemented in
> drivers/passthrough/pci/arm code
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@caviumnetworks.com>
> ---
>  xen/arch/arm/Makefile                |    1 +
>  xen/arch/arm/pci.c                   |   60 +++++++++++++++++++++++
>  xen/drivers/passthrough/arm/Makefile |    1 +
>  xen/drivers/passthrough/arm/pci.c    |   88
> ++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/arm/smmu.c   |    1 -
>  xen/drivers/passthrough/pci.c        |    9 ++--
>  xen/include/asm-arm/device.h         |   33 +++++++++----
>  xen/include/asm-arm/domain.h         |    3 ++
>  xen/include/asm-arm/pci.h            |    7 +--
>  9 files changed, 186 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 935999e..aaf5d88 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,6 +39,7 @@ obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
>  obj-y += smc.o
> +obj-$(HAS_PCI) += pci.o
>   #obj-bin-y += ....o
>  diff --git a/xen/arch/arm/pci.c b/xen/arch/arm/pci.c
> new file mode 100644
> index 0000000..9438f41
> --- /dev/null
> +++ b/xen/arch/arm/pci.c
> @@ -0,0 +1,60 @@
> +#include <xen/pci.h>
> +
> +void _pci_conf_write(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, uint32_t data, int bytes)
> +{
> +}
> +
> +uint32_t _pci_conf_read(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, int bytes)
> +{
> +    return 0;
> +}

I guess that they are going to be implemented with platform specific
code? Although I thought that the mmcfg stuff is somewhat standard across
architectures.


> +uint8_t pci_conf_read8(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)
> +{
> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 1);
> +}
> +
> +uint16_t pci_conf_read16(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)
> +{
> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 2);
> +}
> +
> +uint32_t pci_conf_read32(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)
> +{
> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 4);
> +}
> +
> +void pci_conf_write8(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, uint8_t data)
> +{
> +     _pci_conf_write(seg, bus, dev, func, reg, data, 1);
> +}
> +
> +void pci_conf_write16(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, uint16_t data)
> +{
> +     _pci_conf_write(seg, bus, dev, func, reg, data, 2);
> +}
> +
> +void pci_conf_write32(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, uint32_t data)
> +{
> +     _pci_conf_write(seg, bus, dev, func, reg, data, 4);
> +}
> +
> +void arch_pci_ro_device(int seg, int bdf)
> +{
> +}

This is also missing an implementation. Maybe you should add /* TODO */
here too



> diff --git a/xen/drivers/passthrough/arm/Makefile
> b/xen/drivers/passthrough/arm/Makefile
> index f4cd26e..1a41549 100644
> --- a/xen/drivers/passthrough/arm/Makefile
> +++ b/xen/drivers/passthrough/arm/Makefile
> @@ -1,2 +1,3 @@
>  obj-y += iommu.o
>  obj-y += smmu.o
> +obj-$(HAS_PCI) += pci.o
> diff --git a/xen/drivers/passthrough/arm/pci.c
> b/xen/drivers/passthrough/arm/pci.c
> new file mode 100644
> index 0000000..07a5a78
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/pci.c
> @@ -0,0 +1,88 @@
> +/*
> + * PCI helper functions for arm for passthrough support.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
> USA.
> + *
> + * Copyright (C) 2015 Cavium Networks
> + *
> + * Author: Manish Jaggi <manish.jaggi@cavium.com>
> + */
> +#include <xen/pci.h>
> +#include <asm/device.h>
> +#include <xen/sched.h>
> +
> +int _dump_pci_devices(struct pci_seg *pseg, void *arg)
> +{
> +    struct pci_dev *pdev;
> +
> +    printk("==== segment %04x ====\n", pseg->nr);
> +
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +    {
> +        printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ",
> +               pseg->nr, pdev->bus,
> +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> +               pdev->domain ? pdev->domain->domain_id : -1,
> +               (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
> +        printk(">\n");
> +    }
> +
> +    return 0;
> +}
> +
> +struct device* pci_to_dev(struct pci_dev *pci)
> +{
> +    if(!pci->arch.dev)
> +    {
> +        device_t *dev;
> +        dev = xzalloc (struct device);
> +        pci->arch.dev = dev;
> +        dev->class = DEVICE_PCI;
> +        dev->type = DEV_ENUMERATED;
> +    }
> +    return pci->arch.dev;
> +}

There must be a better place to initialize pci->arch.dev than here

    
> +struct pci_dev* to_pci_dev(struct device *dev)
> +{
> +    if(dev->class == DEVICE_PCI)
> +        return dev->pci_dev;
> +    return NULL;
> +}
> +
> +int dev_is_pci(struct device *dev)
> +{
> +    if(dev->class == DEVICE_PCI)
> +        return 1;
> +    return 0;
> +}
> +
> +
> +int pci_clean_dpci_irqs(struct domain *d)
> +{
> +    /* This is a placeholder function */
> +    return 0;
> +}
> +
> +struct pci_dev* pci_alloc_msix(struct pci_dev *pdev, struct pci_seg  *pseg,
> +                               u8 bus, u8 devfn)
> +{
> +    /* This is a placeholder function. It is implemented only on x86 */
> +    return 0;
> +}
> +
> +void pci_cleanup_msi(struct pci_dev *pdev)
> +{
> +    /* This is a placeholder function. It is implemented only on x86 */
> +}

Although they are just placeholders for now, you are planning to
implement them on ARM, right?


> diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> index 8a9b58b..1406261 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -183,7 +183,6 @@ static void __iomem *devm_ioremap_resource(struct device
> *dev,
>   * Xen: PCI functions
>   * TODO: It should be implemented when PCI will be supported
>   */
> -#define to_pci_dev(dev)	(NULL)
>  static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
>  					 int (*fn) (struct pci_dev *pdev,
>  						    u16 alias, void *data),
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 004aba9..68c292b 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1252,9 +1252,12 @@ static int assign_device(struct domain *d, u16 seg, u8
> bus, u8 devfn)
>      /* Prevent device assign if mem paging or mem sharing have been
>       * enabled for this domain */
>      if ( unlikely(!need_iommu(d) &&
> -            (d->arch.hvm_domain.mem_sharing_enabled ||
> -             d->mem_event->paging.ring_page ||
> -             p2m_get_hostp2m(d)->global_logdirty)) )
> +            (d->mem_event->paging.ring_page
> +#ifdef CONFIG_X86
> +             || d->arch.hvm_domain.mem_sharing_enabled ||
> +             p2m_get_hostp2m(d)->global_logdirty
> +#endif
> +            )) )
>          return -EXDEV;
>       if ( !spin_trylock(&pcidevs_lock) )
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index a72f7c9..009ff0a 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -6,6 +6,17 @@
>  enum device_type
>  {
>      DEV_DT,
> +    DEV_ENUMERATED,
> +};
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_GIC,
> +    DEVICE_PCI,
> +    /* Use for error */
> +    DEVICE_UNKNOWN,
>  };
>   struct dev_archdata {
> @@ -16,28 +27,30 @@ struct dev_archdata {
>  struct device
>  {
>      enum device_type type;
> +    enum device_class class;
>  #ifdef HAS_DEVICE_TREE
>      struct dt_device_node *of_node; /* Used by drivers imported from Linux
> */
>  #endif
>      struct dev_archdata archdata;
> +#ifdef HAS_PCI
> +    void *pci_dev;
> +#endif
>  };
>   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,
> -    DEVICE_IOMMU,
> -    DEVICE_GIC,
> -    /* Use for error */
> -    DEVICE_UNKNOWN,
> -};
> +struct pci_dev;
> +device_t* pci_to_dev(struct pci_dev *pci);
> +#ifndef HAS_PCI
> +#define to_pci_dev(dev) (NULL)
> +#else
> +struct pci_dev* to_pci_dev(device_t *dev);
> +#endif
> +int dev_is_pci(device_t *dev);
>   struct device_desc {
>      /* Device name */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 9e0419e..41ae847 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -42,6 +42,8 @@ struct vtimer {
>          uint64_t cval;
>  };
>  +#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
> +
>  struct arch_domain
>  {
>  #ifdef CONFIG_ARM_64
> @@ -56,6 +58,7 @@ struct arch_domain
>      xen_pfn_t *grant_table_gpfn;
>       struct io_handler io_handlers;
> +    struct list_head pdev_list;
>      /* Continuable domain_relinquish_resources(). */
>      enum {
>          RELMEM_not_started,
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index de13359..b8ec882 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -1,7 +1,8 @@
> -#ifndef __X86_PCI_H__
> -#define __X86_PCI_H__
> +#ifndef __ARM_PCI_H__
> +#define __ARM_PCI_H__
>   struct arch_pci_dev {
> +    void *dev;
>  };
>  -#endif /* __X86_PCI_H__ */
> +#endif /* __ARM_PCI_H__ */
> -- 
> 1.7.9.5
> 

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Manish Jaggi]

On Monday 13 April 2015 03:44 PM, Stefano Stabellini wrote:
> On Mon, 13 Apr 2015, Manish Jaggi wrote:
>> Add ARM PCI Support
>> ---------------
>> a) Place holder functions are added for pci_conf_read/write calls.
>> b) Macros dev_is_pci, pci_to_dev are implemented in
>> drivers/passthrough/pci/arm code
>>
>> Signed-off-by: Manish Jaggi <manish.jaggi@caviumnetworks.com>
>> ---
>>   xen/arch/arm/Makefile                |    1 +
>>   xen/arch/arm/pci.c                   |   60 +++++++++++++++++++++++
>>   xen/drivers/passthrough/arm/Makefile |    1 +
>>   xen/drivers/passthrough/arm/pci.c    |   88
>> ++++++++++++++++++++++++++++++++++
>>   xen/drivers/passthrough/arm/smmu.c   |    1 -
>>   xen/drivers/passthrough/pci.c        |    9 ++--
>>   xen/include/asm-arm/device.h         |   33 +++++++++----
>>   xen/include/asm-arm/domain.h         |    3 ++
>>   xen/include/asm-arm/pci.h            |    7 +--
>>   9 files changed, 186 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 935999e..aaf5d88 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -39,6 +39,7 @@ obj-y += device.o
>>   obj-y += decode.o
>>   obj-y += processor.o
>>   obj-y += smc.o
>> +obj-$(HAS_PCI) += pci.o
>>    #obj-bin-y += ....o
>>   diff --git a/xen/arch/arm/pci.c b/xen/arch/arm/pci.c
>> new file mode 100644
>> index 0000000..9438f41
>> --- /dev/null
>> +++ b/xen/arch/arm/pci.c
>> @@ -0,0 +1,60 @@
>> +#include <xen/pci.h>
>> +
>> +void _pci_conf_write(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg, uint32_t data, int bytes)
>> +{
>> +}
>> +
>> +uint32_t _pci_conf_read(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg, int bytes)
>> +{
>> +    return 0;
>> +}
> I guess that they are going to be implemented with platform specific
> code? Although I thought that the mmcfg stuff is somewhat standard across
> architectures.
>
yes.
_pci_conf_read/write will call pcihb_conf_read/write (pcihb = 
pci_host_bridge).
ref: http://lists.xen.org/archives/html/xen-devel/2015-02/msg02717.html
>> +uint8_t pci_conf_read8(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg)
>> +{
>> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 1);
>> +}
>> +
>> +uint16_t pci_conf_read16(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg)
>> +{
>> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 2);
>> +}
>> +
>> +uint32_t pci_conf_read32(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg)
>> +{
>> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 4);
>> +}
>> +
>> +void pci_conf_write8(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg, uint8_t data)
>> +{
>> +     _pci_conf_write(seg, bus, dev, func, reg, data, 1);
>> +}
>> +
>> +void pci_conf_write16(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg, uint16_t data)
>> +{
>> +     _pci_conf_write(seg, bus, dev, func, reg, data, 2);
>> +}
>> +
>> +void pci_conf_write32(
>> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
>> +    uint32_t reg, uint32_t data)
>> +{
>> +     _pci_conf_write(seg, bus, dev, func, reg, data, 4);
>> +}
>> +
>> +void arch_pci_ro_device(int seg, int bdf)
>> +{
>> +}
> This is also missing an implementation. Maybe you should add /* TODO */
> here too
ok.
>
>
>> diff --git a/xen/drivers/passthrough/arm/Makefile
>> b/xen/drivers/passthrough/arm/Makefile
>> index f4cd26e..1a41549 100644
>> --- a/xen/drivers/passthrough/arm/Makefile
>> +++ b/xen/drivers/passthrough/arm/Makefile
>> @@ -1,2 +1,3 @@
>>   obj-y += iommu.o
>>   obj-y += smmu.o
>> +obj-$(HAS_PCI) += pci.o
>> diff --git a/xen/drivers/passthrough/arm/pci.c
>> b/xen/drivers/passthrough/arm/pci.c
>> new file mode 100644
>> index 0000000..07a5a78
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/arm/pci.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * PCI helper functions for arm for passthrough support.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
>> USA.
>> + *
>> + * Copyright (C) 2015 Cavium Networks
>> + *
>> + * Author: Manish Jaggi <manish.jaggi@cavium.com>
>> + */
>> +#include <xen/pci.h>
>> +#include <asm/device.h>
>> +#include <xen/sched.h>
>> +
>> +int _dump_pci_devices(struct pci_seg *pseg, void *arg)
>> +{
>> +    struct pci_dev *pdev;
>> +
>> +    printk("==== segment %04x ====\n", pseg->nr);
>> +
>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> +    {
>> +        printk("%04x:%02x:%02x.%u - dom %-3d - node %-3d - MSIs < ",
>> +               pseg->nr, pdev->bus,
>> +               PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
>> +               pdev->domain ? pdev->domain->domain_id : -1,
>> +               (pdev->node != NUMA_NO_NODE) ? pdev->node : -1);
>> +        printk(">\n");
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +struct device* pci_to_dev(struct pci_dev *pci)
>> +{
>> +    if(!pci->arch.dev)
>> +    {
>> +        device_t *dev;
>> +        dev = xzalloc (struct device);
>> +        pci->arch.dev = dev;
>> +        dev->class = DEVICE_PCI;
>> +        dev->type = DEV_ENUMERATED;
>> +    }
>> +    return pci->arch.dev;
>> +}
> There must be a better place to initialize pci->arch.dev than here
ok will check.
>      
>> +struct pci_dev* to_pci_dev(struct device *dev)
>> +{
>> +    if(dev->class == DEVICE_PCI)
>> +        return dev->pci_dev;
>> +    return NULL;
>> +}
>> +
>> +int dev_is_pci(struct device *dev)
>> +{
>> +    if(dev->class == DEVICE_PCI)
>> +        return 1;
>> +    return 0;
>> +}
>> +
>> +
>> +int pci_clean_dpci_irqs(struct domain *d)
>> +{
>> +    /* This is a placeholder function */
>> +    return 0;
>> +}
>> +
>> +struct pci_dev* pci_alloc_msix(struct pci_dev *pdev, struct pci_seg  *pseg,
>> +                               u8 bus, u8 devfn)
>> +{
>> +    /* This is a placeholder function. It is implemented only on x86 */
>> +    return 0;
>> +}
>> +
>> +void pci_cleanup_msi(struct pci_dev *pdev)
>> +{
>> +    /* This is a placeholder function. It is implemented only on x86 */
>> +}
> Although they are just placeholders for now, you are planning to
> implement them on ARM, right?
>
>
No. As MSIs are handled by guest directly in ARM. There are added to 
make compilation clean.
>> diff --git a/xen/drivers/passthrough/arm/smmu.c
>> b/xen/drivers/passthrough/arm/smmu.c
>> index 8a9b58b..1406261 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -183,7 +183,6 @@ static void __iomem *devm_ioremap_resource(struct device
>> *dev,
>>    * Xen: PCI functions
>>    * TODO: It should be implemented when PCI will be supported
>>    */
>> -#define to_pci_dev(dev)	(NULL)
>>   static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
>>   					 int (*fn) (struct pci_dev *pdev,
>>   						    u16 alias, void *data),
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 004aba9..68c292b 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1252,9 +1252,12 @@ static int assign_device(struct domain *d, u16 seg, u8
>> bus, u8 devfn)
>>       /* Prevent device assign if mem paging or mem sharing have been
>>        * enabled for this domain */
>>       if ( unlikely(!need_iommu(d) &&
>> -            (d->arch.hvm_domain.mem_sharing_enabled ||
>> -             d->mem_event->paging.ring_page ||
>> -             p2m_get_hostp2m(d)->global_logdirty)) )
>> +            (d->mem_event->paging.ring_page
>> +#ifdef CONFIG_X86
>> +             || d->arch.hvm_domain.mem_sharing_enabled ||
>> +             p2m_get_hostp2m(d)->global_logdirty
>> +#endif
>> +            )) )
>>           return -EXDEV;
>>        if ( !spin_trylock(&pcidevs_lock) )
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index a72f7c9..009ff0a 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -6,6 +6,17 @@
>>   enum device_type
>>   {
>>       DEV_DT,
>> +    DEV_ENUMERATED,
>> +};
>> +
>> +enum device_class
>> +{
>> +    DEVICE_SERIAL,
>> +    DEVICE_IOMMU,
>> +    DEVICE_GIC,
>> +    DEVICE_PCI,
>> +    /* Use for error */
>> +    DEVICE_UNKNOWN,
>>   };
>>    struct dev_archdata {
>> @@ -16,28 +27,30 @@ struct dev_archdata {
>>   struct device
>>   {
>>       enum device_type type;
>> +    enum device_class class;
>>   #ifdef HAS_DEVICE_TREE
>>       struct dt_device_node *of_node; /* Used by drivers imported from Linux
>> */
>>   #endif
>>       struct dev_archdata archdata;
>> +#ifdef HAS_PCI
>> +    void *pci_dev;
>> +#endif
>>   };
>>    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,
>> -    DEVICE_IOMMU,
>> -    DEVICE_GIC,
>> -    /* Use for error */
>> -    DEVICE_UNKNOWN,
>> -};
>> +struct pci_dev;
>> +device_t* pci_to_dev(struct pci_dev *pci);
>> +#ifndef HAS_PCI
>> +#define to_pci_dev(dev) (NULL)
>> +#else
>> +struct pci_dev* to_pci_dev(device_t *dev);
>> +#endif
>> +int dev_is_pci(device_t *dev);
>>    struct device_desc {
>>       /* Device name */
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 9e0419e..41ae847 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -42,6 +42,8 @@ struct vtimer {
>>           uint64_t cval;
>>   };
>>   +#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
>> +
>>   struct arch_domain
>>   {
>>   #ifdef CONFIG_ARM_64
>> @@ -56,6 +58,7 @@ struct arch_domain
>>       xen_pfn_t *grant_table_gpfn;
>>        struct io_handler io_handlers;
>> +    struct list_head pdev_list;
>>       /* Continuable domain_relinquish_resources(). */
>>       enum {
>>           RELMEM_not_started,
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index de13359..b8ec882 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -1,7 +1,8 @@
>> -#ifndef __X86_PCI_H__
>> -#define __X86_PCI_H__
>> +#ifndef __ARM_PCI_H__
>> +#define __ARM_PCI_H__
>>    struct arch_pci_dev {
>> +    void *dev;
>>   };
>>   -#endif /* __X86_PCI_H__ */
>> +#endif /* __ARM_PCI_H__ */
>> -- 
>> 1.7.9.5
>>

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Julien Grall]

Hi Manish,

On 13/04/15 08:48, Manish Jaggi wrote:
> Add ARM PCI Support
> ---------------
> a) Place holder functions are added for pci_conf_read/write calls.
> b) Macros dev_is_pci, pci_to_dev are implemented in
> drivers/passthrough/pci/arm code
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@caviumnetworks.com>
> ---
>  xen/arch/arm/Makefile                |    1 +
>  xen/arch/arm/pci.c                   |   60 +++++++++++++++++++++++
>  xen/drivers/passthrough/arm/Makefile |    1 +
>  xen/drivers/passthrough/arm/pci.c    |   88
> ++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/arm/smmu.c   |    1 -
>  xen/drivers/passthrough/pci.c        |    9 ++--
>  xen/include/asm-arm/device.h         |   33 +++++++++----
>  xen/include/asm-arm/domain.h         |    3 ++
>  xen/include/asm-arm/pci.h            |    7 +--
>  9 files changed, 186 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 935999e..aaf5d88 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,6 +39,7 @@ obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
>  obj-y += smc.o
> +obj-$(HAS_PCI) += pci.o
>  
>  #obj-bin-y += ....o
>  
> diff --git a/xen/arch/arm/pci.c b/xen/arch/arm/pci.c
> new file mode 100644
> index 0000000..9438f41
> --- /dev/null
> +++ b/xen/arch/arm/pci.c
> @@ -0,0 +1,60 @@
> +#include <xen/pci.h>
> +
> +void _pci_conf_write(

The _ is not necessary here. Please name the function pci_conf_write.

> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, uint32_t data, int bytes)

unsigned int bytes?

> +{
> +}
> +
> +uint32_t _pci_conf_read(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg, int bytes)
> +{
> +    return 0;
> +}

Same here.

> +
> +uint8_t pci_conf_read8(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)
> +{
> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 1);
> +}
> +
> +uint16_t pci_conf_read16(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)
> +{
> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 2);

Wrong cast.

> +}
> +
> +uint32_t pci_conf_read32(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)
> +{
> +    return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 4);

Wrong cast.

[..]

> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 004aba9..68c292b 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1252,9 +1252,12 @@ static int assign_device(struct domain *d, u16
> seg, u8 bus, u8 devfn)
>      /* Prevent device assign if mem paging or mem sharing have been
>       * enabled for this domain */
>      if ( unlikely(!need_iommu(d) &&
> -            (d->arch.hvm_domain.mem_sharing_enabled ||
> -             d->mem_event->paging.ring_page ||
> -             p2m_get_hostp2m(d)->global_logdirty)) )
> +            (d->mem_event->paging.ring_page
> +#ifdef CONFIG_X86
> +             || d->arch.hvm_domain.mem_sharing_enabled ||
> +             p2m_get_hostp2m(d)->global_logdirty
> +#endif
> +            )) )

Please avoid #ifdef CONFIG_* and introduce an arch macro.

>          return -EXDEV;
>  
>      if ( !spin_trylock(&pcidevs_lock) )
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index a72f7c9..009ff0a 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -6,6 +6,17 @@
>  enum device_type
>  {
>      DEV_DT,
> +    DEV_ENUMERATED,
> +};
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_GIC,
> +    DEVICE_PCI,
> +    /* Use for error */
> +    DEVICE_UNKNOWN,
>  };

Why? It will be very unlikely that we have to create a structure device
for the IOMMU, GIC and SERIAL.

It would make more sense to add a DEV_PCI directly to device_type.

>  
>  struct dev_archdata {
> @@ -16,28 +27,30 @@ struct dev_archdata {
>  struct device
>  {
>      enum device_type type;
> +    enum device_class class;
>  #ifdef HAS_DEVICE_TREE
>      struct dt_device_node *of_node; /* Used by drivers imported from
> Linux */
>  #endif
>      struct dev_archdata archdata;
> +#ifdef HAS_PCI
> +    void *pci_dev;

This field is not necessary. I've added one for the DT for keeping
compatibility with Linux.

> +#endif

[..]

> +int dev_is_pci(device_t *dev);

This could easily be a macro.

>  
>  struct device_desc {
>      /* Device name */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 9e0419e..41ae847 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -42,6 +42,8 @@ struct vtimer {
>          uint64_t cval;
>  };
>  
> +#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
> +
>  struct arch_domain
>  {
>  #ifdef CONFIG_ARM_64
> @@ -56,6 +58,7 @@ struct arch_domain
>      xen_pfn_t *grant_table_gpfn;
>  
>      struct io_handler io_handlers;
> +    struct list_head pdev_list;
>      /* Continuable domain_relinquish_resources(). */
>      enum {
>          RELMEM_not_started,
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index de13359..b8ec882 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -1,7 +1,8 @@
> -#ifndef __X86_PCI_H__
> -#define __X86_PCI_H__
> +#ifndef __ARM_PCI_H__
> +#define __ARM_PCI_H__
>  
>  struct arch_pci_dev {
> +    void *dev;

void * is error-prone. Why can't you use the use the real structure?

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Jaggi, Manish]

Hi Julien,

________________________________________
From: xen-devel-bounces@lists.xen.org <xen-devel-bounces@lists.xen.org> on behalf of Julien Grall <julien.grall@citrix.com>
Sent: Monday, April 13, 2015 4:12 PM
To: Jaggi, Manish; Xen Devel; Stefano Stabellini; Julien Grall; Ian Campbell; Kumar, Vijaya; Prasun.kapoor@cavium.com
Subject: Re: [Xen-devel] [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

Hi Manish,

On 13/04/15 08:48, Manish Jaggi wrote:
> Add ARM PCI Support
> ---------------
> a) Place holder functions are added for pci_conf_read/write calls.
> b) Macros dev_is_pci, pci_to_dev are implemented in
> drivers/passthrough/pci/arm code
>
> Signed-off-by: Manish Jaggi <manish.jaggi@caviumnetworks.com>
> ---
>  xen/arch/arm/Makefile                |    1 +
>  xen/arch/arm/pci.c                   |   60 +++++++++++++++++++++++
>  xen/drivers/passthrough/arm/Makefile |    1 +
>  xen/drivers/passthrough/arm/pci.c    |   88
> ++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/arm/smmu.c   |    1 -
>  xen/drivers/passthrough/pci.c        |    9 ++--
>  xen/include/asm-arm/device.h         |   33 +++++++++----
>  xen/include/asm-arm/domain.h         |    3 ++
>  xen/include/asm-arm/pci.h            |    7 +--
>  9 files changed, 186 insertions(+), 17 deletions(-)
>
[...]
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 004aba9..68c292b 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1252,9 +1252,12 @@ static int assign_device(struct domain *d, u16
> seg, u8 bus, u8 devfn)
>      /* Prevent device assign if mem paging or mem sharing have been
>       * enabled for this domain */
>      if ( unlikely(!need_iommu(d) &&
> -            (d->arch.hvm_domain.mem_sharing_enabled ||
> -             d->mem_event->paging.ring_page ||
> -             p2m_get_hostp2m(d)->global_logdirty)) )
> +            (d->mem_event->paging.ring_page
> +#ifdef CONFIG_X86
> +             || d->arch.hvm_domain.mem_sharing_enabled ||
> +             p2m_get_hostp2m(d)->global_logdirty
> +#endif
> +            )) )

Please avoid #ifdef CONFIG_* and introduce an arch macro. 
[Manish] ok
>          return -EXDEV;
>
>      if ( !spin_trylock(&pcidevs_lock) )
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index a72f7c9..009ff0a 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -6,6 +6,17 @@
>  enum device_type
>  {
>      DEV_DT,
> +    DEV_ENUMERATED,
> +};
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_GIC,
> +    DEVICE_PCI,
> +    /* Use for error */
> +    DEVICE_UNKNOWN,
>  };

Why? It will be very unlikely that we have to create a structure device
for the IOMMU, GIC and SERIAL.

It would make more sense to add a DEV_PCI directly to device_type.
[manish] ok. 

>
>  struct dev_archdata {
> @@ -16,28 +27,30 @@ struct dev_archdata {
>  struct device
>  {
>      enum device_type type;
> +    enum device_class class;
>  #ifdef HAS_DEVICE_TREE
>      struct dt_device_node *of_node; /* Used by drivers imported from
> Linux */
>  #endif
>      struct dev_archdata archdata;
> +#ifdef HAS_PCI
> +    void *pci_dev;

This field is not necessary. I've added one for the DT for keeping
compatibility with Linux.
[Manish] pci_dev is not in struct device currently. Didn't get you
 I have added as It is required  for to_pci_dev call.

> +#endif

[..]

> +int dev_is_pci(device_t *dev);

This could easily be a macro.
[manish] ok

>
>  struct device_desc {
>      /* Device name */
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 9e0419e..41ae847 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -42,6 +42,8 @@ struct vtimer {
>          uint64_t cval;
>  };
>
> +#define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
> +
>  struct arch_domain
>  {
>  #ifdef CONFIG_ARM_64
> @@ -56,6 +58,7 @@ struct arch_domain
>      xen_pfn_t *grant_table_gpfn;
>
>      struct io_handler io_handlers;
> +    struct list_head pdev_list;
>      /* Continuable domain_relinquish_resources(). */
>      enum {
>          RELMEM_not_started,
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index de13359..b8ec882 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -1,7 +1,8 @@
> -#ifndef __X86_PCI_H__
> -#define __X86_PCI_H__
> +#ifndef __ARM_PCI_H__
> +#define __ARM_PCI_H__
>
>  struct arch_pci_dev {
> +    void *dev;

void * is error-prone. Why can't you use the use the real structure?

[manish]Will change it.  I believe dev_archdata structure has also a void *  (in asm-arm/device.h). So all void * are error prone in xen ?

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Stefano Stabellini]

On Tue, 14 Apr 2015, Jaggi, Manish wrote:
> > diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> > index de13359..b8ec882 100644
> > --- a/xen/include/asm-arm/pci.h
> > +++ b/xen/include/asm-arm/pci.h
> > @@ -1,7 +1,8 @@
> > -#ifndef __X86_PCI_H__
> > -#define __X86_PCI_H__
> > +#ifndef __ARM_PCI_H__
> > +#define __ARM_PCI_H__
> >
> >  struct arch_pci_dev {
> > +    void *dev;
> 
> void * is error-prone. Why can't you use the use the real structure?
> 
> [manish]Will change it.  I believe dev_archdata structure has also a void *  (in asm-arm/device.h). So all void * are error prone in xen ?
> 

As you know void* works around the type system, so it prevents the
compiler from making many type safety checks. We should try to avoid
them if we can.

I think that you are right, the void *iommu in dev_archdata should
actually be struct arm_smmu_xen_device *iommu.

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Julien Grall]

On 14/04/15 10:28, Stefano Stabellini wrote:
> On Tue, 14 Apr 2015, Jaggi, Manish wrote:
>>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>>> index de13359..b8ec882 100644
>>> --- a/xen/include/asm-arm/pci.h
>>> +++ b/xen/include/asm-arm/pci.h
>>> @@ -1,7 +1,8 @@
>>> -#ifndef __X86_PCI_H__
>>> -#define __X86_PCI_H__
>>> +#ifndef __ARM_PCI_H__
>>> +#define __ARM_PCI_H__
>>>
>>>  struct arch_pci_dev {
>>> +    void *dev;
>>
>> void * is error-prone. Why can't you use the use the real structure?
>>
>> [manish]Will change it.  I believe dev_archdata structure has also a void *  (in asm-arm/device.h). So all void * are error prone in xen ?
>>
> 
> As you know void* works around the type system, so it prevents the
> compiler from making many type safety checks. We should try to avoid
> them if we can.

In this case, the pointer add more management (allocation...).

As for the device tree solution, the field should be a struct device.

> I think that you are right, the void *iommu in dev_archdata should
> actually be struct arm_smmu_xen_device *iommu.

I agree that void* should be void in most of the case when we know the
underlaying type. Although there is some place where the number of type
of unknown because it could be used to store driver specific case.

This is actually the case of this field. It contains private data for
IOMMU driver. When a new driver will be implemented, it will likely use
a different structure.

Furthermore, the SMMU drivers is self contained in a file, using struct
arm_smmu_xen_device* would require to export/define some part of the
driver in an header.

So, I don't think this is the right things to do.

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Julien Grall]

On 14/04/15 11:33, Julien Grall wrote:
> On 14/04/15 10:28, Stefano Stabellini wrote:
>> On Tue, 14 Apr 2015, Jaggi, Manish wrote:
>>>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>>>> index de13359..b8ec882 100644
>>>> --- a/xen/include/asm-arm/pci.h
>>>> +++ b/xen/include/asm-arm/pci.h
>>>> @@ -1,7 +1,8 @@
>>>> -#ifndef __X86_PCI_H__
>>>> -#define __X86_PCI_H__
>>>> +#ifndef __ARM_PCI_H__
>>>> +#define __ARM_PCI_H__
>>>>
>>>>  struct arch_pci_dev {
>>>> +    void *dev;
>>>
>>> void * is error-prone. Why can't you use the use the real structure?
>>>
>>> [manish]Will change it.  I believe dev_archdata structure has also a void *  (in asm-arm/device.h). So all void * are error prone in xen ?
>>>
>>
>> As you know void* works around the type system, so it prevents the
>> compiler from making many type safety checks. We should try to avoid
>> them if we can.
> 
> In this case, the pointer add more management (allocation...).
> 
> As for the device tree solution, the field should be a struct device.
> 
>> I think that you are right, the void *iommu in dev_archdata should
>> actually be struct arm_smmu_xen_device *iommu.
> 
> I agree that void* should be void in most of the case when we know the
> underlaying type. Although there is some place where the number of type
> of unknown because it could be used to store driver specific case.
> 
> This is actually the case of this field. It contains private data for
> IOMMU driver. When a new driver will be implemented, it will likely use
> a different structure.
> 
> Furthermore, the SMMU drivers is self contained in a file, using struct
> arm_smmu_xen_device* would require to export/define some part of the
> driver in an header.

A similar example would be the scheduler coder (see sched_data in
include/xen/sched-if.h). We don't want to expose the underlying
structure out of the file.

Regards,


-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Ian Campbell]

On Mon, 2015-04-13 at 13:18 +0530, Manish Jaggi wrote:
> uint8_t pci_conf_read8(
> +    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
> +    uint32_t reg)

Shouldn't this (and the other functions) match the prototype in
xen/pci.h, which is:
uint8_t pci_conf_read8(
    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
    unsigned int reg);
?

We generally try and use precisely sized types in the ARM code, but
where we interact with generic/common interfaces we should either follow
what they do or make an argument (via a preparatory patch) for changing
them.

Ian.