Skip to contentSkip to conversation

xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

PATCH In review Manish Jaggi 5 participants 9 messages Apr 13, 2015 – Apr 16, 2015

lore
Share & tools
lore

Manish Jaggi

author
#0 lore raw

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

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 <[email protected]>
---
 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(-)
xen/arch/arm/Makefile +1 -0
index 935999e..aaf5d88 100644
@@ -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
xen/arch/arm/pci.c new file +60 -0

60 lines not on this page · raw message

xen/drivers/passthrough/arm/Makefile +1 -0
index f4cd26e..1a41549 100644
@@ -1,2 +1,3 @@
 obj-y += iommu.o
 obj-y += smmu.o
+obj-$(HAS_PCI) += pci.o
xen/drivers/passthrough/arm/pci.c new file +88 -0

88 lines not on this page · raw message

xen/drivers/passthrough/arm/smmu.c +0 -1
index 8a9b58b..1406261 100644
@@ -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),
xen/drivers/passthrough/pci.c +6 -3
index 004aba9..68c292b 100644
@@ -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) )
xen/include/asm-arm/device.h +23 -10
index a72f7c9..009ff0a 100644
@@ -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 */
xen/include/asm-arm/domain.h +3 -0
index 9e0419e..41ae847 100644
@@ -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,
xen/include/asm-arm/pci.h +4 -3
index de13359..b8ec882 100644
@@ -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__ */
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 40 lines · 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 <[email protected]>
---
 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(-)
xen/arch/arm/Makefile +1 -0
index 935999e..aaf5d88 100644
@@ -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

xen/arch/arm/pci.c new file +4 -0
@@ -0,0 +1,60 @@
+#include <xen/pci.h>
+
+void _pci_conf_write(
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,

xen/arch/arm/pci.c line 14

Quoted 10 lines
xen/arch/arm/pci.c new file +10 -0
+    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.

Quoted 35 lines
xen/arch/arm/pci.c new file +35 -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);
+}
+

xen/arch/arm/pci.c line 60

Quoted 10 lines
xen/arch/arm/pci.c new file +10 -0
+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

Quoted 62 lines
xen/drivers/passthrough/arm/Makefile +1 -0
index f4cd26e..1a41549 100644
@@ -1,2 +1,3 @@
 obj-y += iommu.o
 obj-y += smmu.o
+obj-$(HAS_PCI) += pci.o
xen/drivers/passthrough/arm/pci.c new file +15 -0
new file mode 100644
index 0000000..07a5a78
@@ -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 <[email protected]>
+ */

xen/drivers/passthrough/arm/pci.c new file +25 -0
+#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)
+{

xen/drivers/passthrough/arm/pci.c line 55

Quoted 10 lines
xen/drivers/passthrough/arm/pci.c new file +10 -0
+    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

Quoted 22 lines
xen/drivers/passthrough/arm/pci.c new file +22 -0
+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,

xen/drivers/passthrough/arm/pci.c line 88

Quoted 10 lines
xen/drivers/passthrough/arm/pci.c new file +10 -0
+                               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?

Quoted 128 lines
xen/drivers/passthrough/arm/smmu.c +0 -1
index 8a9b58b..1406261 100644
@@ -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,
xen/drivers/passthrough/arm/smmu.c +0 -0
 						    u16 alias, void *data),
xen/drivers/passthrough/pci.c +6 -3
index 004aba9..68c292b 100644
@@ -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) )
xen/include/asm-arm/device.h +23 -8
index a72f7c9..009ff0a 100644
@@ -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 */
xen/include/asm-arm/domain.h +2 -0
index 9e0419e..41ae847 100644
@@ -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,
xen/include/asm-arm/pci.h +4 -2
index de13359..b8ec882 100644
@@ -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__ */
Signature · 1 line
1.7.9.5
author
#2 lore raw
Quoted 46 lines · Stefano Stabellini
Quoted 45 lines · 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 <[email protected]>
---
  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(-)
xen/arch/arm/Makefile +1 -0
index 935999e..aaf5d88 100644
@@ -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

xen/arch/arm/pci.c new file +9 -0
@@ -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(
Quoted 9 lines

xen/arch/arm/pci.c line 14

Quoted 5 lines
xen/arch/arm/pci.c new file +5 -0
+    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

Quoted 39 lines
Quoted 39 lines
xen/arch/arm/pci.c new file +39 -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)
+{
Quoted 8 lines

xen/arch/arm/pci.c line 60

Quoted 6 lines
xen/arch/arm/pci.c new file +6 -0
+     _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.

Quoted 67 lines
Quoted 67 lines
xen/drivers/passthrough/arm/Makefile +1 -0
index f4cd26e..1a41549 100644
@@ -1,2 +1,3 @@
  obj-y += iommu.o
  obj-y += smmu.o
+obj-$(HAS_PCI) += pci.o
xen/drivers/passthrough/arm/pci.c new file +15 -0
new file mode 100644
index 0000000..07a5a78
@@ -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 <[email protected]>
+ */

xen/drivers/passthrough/arm/pci.c new file +28 -0
+#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;
Quoted 8 lines

xen/drivers/passthrough/arm/pci.c line 55

Quoted 7 lines
xen/drivers/passthrough/arm/pci.c new file +7 -0
+        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.

Quoted 27 lines
Quoted 27 lines
xen/drivers/passthrough/arm/pci.c new file +26 -0
+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;
Quoted 10 lines

xen/drivers/passthrough/arm/pci.c line 88

Quoted 6 lines
xen/drivers/passthrough/arm/pci.c new file +6 -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.

Quoted 128 lines
Quoted 128 lines
xen/drivers/passthrough/arm/smmu.c +0 -1
index 8a9b58b..1406261 100644
@@ -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),

xen/drivers/passthrough/pci.c +6 -3
index 004aba9..68c292b 100644
@@ -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) )
xen/include/asm-arm/device.h +23 -8
index a72f7c9..009ff0a 100644
@@ -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 */
xen/include/asm-arm/domain.h +2 -0
index 9e0419e..41ae847 100644
@@ -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,
xen/include/asm-arm/pci.h +4 -2
index de13359..b8ec882 100644
@@ -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__ */
Signature · 1 line
1.7.9.5
#3 lore raw
In reply to Manish Jaggi

Hi Manish,

Quoted 38 lines · 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 <[email protected]>
---
 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(-)
xen/arch/arm/Makefile +1 -0
index 935999e..aaf5d88 100644
@@ -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
xen/arch/arm/pci.c new file +0 -0
new file mode 100644
index 0000000..9438f41

xen/arch/arm/pci.c line 3

Quoted 3 lines
xen/arch/arm/pci.c new file +3 -0
@@ -0,0 +1,60 @@
+#include <xen/pci.h>
+
+void _pci_conf_write(

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

xen/arch/arm/pci.c line 5

Quoted 2 lines
xen/arch/arm/pci.c new file +2 -0
+    uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func,
+    uint32_t reg, uint32_t data, int bytes)

unsigned int bytes?

xen/arch/arm/pci.c line 14

Quoted 9 lines
xen/arch/arm/pci.c new file +9 -0
+{
+}
+
+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.

xen/arch/arm/pci.c line 27

Quoted 13 lines
xen/arch/arm/pci.c new file +13 -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);

Wrong cast.

xen/arch/arm/pci.c line 34

Quoted 7 lines
xen/arch/arm/pci.c new file +7 -0
+}
+
+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.

[..]

Quoted 8 lines
xen/drivers/passthrough/pci.c +0 -0
index 004aba9..68c292b 100644
@@ -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 */

xen/drivers/passthrough/pci.c static int assign_device(struct domain *d, u16 · line 1261

Quoted 10 lines
xen/drivers/passthrough/pci.c +6 -3
     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.

Quoted 13 lines
xen/drivers/passthrough/pci.c +0 -0
         return -EXDEV;

if ( !spin_trylock(&pcidevs_lock) )

xen/include/asm-arm/device.h +2 -0
index a72f7c9..009ff0a 100644
@@ -6,6 +6,17 @@
 enum device_type
 {
     DEV_DT,
+    DEV_ENUMERATED,
+};

xen/include/asm-arm/device.h line 20

Quoted 10 lines
xen/include/asm-arm/device.h +9 -0
+
+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.

Quoted 12 lines
xen/include/asm-arm/device.h +1 -0
 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;

xen/include/asm-arm/device.h struct dev_archdata { · line 36

Quoted 2 lines
xen/include/asm-arm/device.h +2 -0
+#ifdef HAS_PCI
+    void *pci_dev;

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

Quoted 1 line

+#endif

[..]

Quoted 1 line

+int dev_is_pci(device_t *dev);

This could easily be a macro.

Quoted 29 lines
 
 struct device_desc {
     /* Device name */
xen/include/asm-arm/domain.h +3 -0
index 9e0419e..41ae847 100644
@@ -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,
xen/include/asm-arm/pci.h +0 -0
index de13359..b8ec882 100644

xen/include/asm-arm/pci.h line 5

Quoted 7 lines
xen/include/asm-arm/pci.h +3 -2
@@ -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,

Signature · 1 line
Julien Grall
#4 lore raw

Hi Julien,

________________________________________
From: [email protected] <[email protected]> on behalf of Julien Grall <[email protected]>
Sent: Monday, April 13, 2015 4:12 PM
To: Jaggi, Manish; Xen Devel; Stefano Stabellini; Julien Grall; Ian Campbell; Kumar, Vijaya; [email protected]
Subject: Re: [Xen-devel] [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

Hi Manish,

Quoted 20 lines · 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 <[email protected]>
---
 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(-)

[...]

Quoted 8 lines
xen/drivers/passthrough/pci.c +0 -0
index 004aba9..68c292b 100644
@@ -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 */

xen/drivers/passthrough/pci.c static int assign_device(struct domain *d, u16 · line 1261

Quoted 10 lines
xen/drivers/passthrough/pci.c +6 -3
     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

Quoted 13 lines
xen/drivers/passthrough/pci.c +0 -0
         return -EXDEV;

if ( !spin_trylock(&pcidevs_lock) )

xen/include/asm-arm/device.h +2 -0
index a72f7c9..009ff0a 100644
@@ -6,6 +6,17 @@
 enum device_type
 {
     DEV_DT,
+    DEV_ENUMERATED,
+};

xen/include/asm-arm/device.h line 20

Quoted 10 lines
xen/include/asm-arm/device.h +9 -0
+
+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.

Quoted 12 lines
xen/include/asm-arm/device.h +1 -0
 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;

xen/include/asm-arm/device.h struct dev_archdata { · line 36

Quoted 2 lines
xen/include/asm-arm/device.h +2 -0
+#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.

Quoted 1 line

+#endif

[..]

Quoted 1 line

+int dev_is_pci(device_t *dev);

This could easily be a macro.
[manish] ok

Quoted 29 lines
 struct device_desc {
     /* Device name */
xen/include/asm-arm/domain.h +3 -0
index 9e0419e..41ae847 100644
@@ -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,
xen/include/asm-arm/pci.h +0 -0
index de13359..b8ec882 100644

xen/include/asm-arm/pci.h line 5

Quoted 7 lines
xen/include/asm-arm/pci.h +3 -2
@@ -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
[email protected]
http://lists.xen.org/xen-devel

#5 lore raw
Quoted 10 lines · Manish
Quoted 10 lines
xen/include/asm-arm/pci.h +2 -2
index de13359..b8ec882 100644
@@ -1,7 +1,8 @@
-#ifndef __X86_PCI_H__
-#define __X86_PCI_H__
+#ifndef __ARM_PCI_H__
+#define __ARM_PCI_H__
Quoted 7 lines

xen/include/asm-arm/pci.h line 5

Quoted 2 lines
xen/include/asm-arm/pci.h +1 -0
 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.

#6 lore raw
Quoted 16 lines · Stefano Stabellini
Quoted 15 lines · Manish

xen/include/asm-arm/pci.h line 5

Quoted 12 lines
xen/include/asm-arm/pci.h +3 -2
index de13359..b8ec882 100644
@@ -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?

Quoted 6 lines
Quoted 2 lines

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

Quoted 2 lines

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,

Signature · 1 line
Julien Grall
#7 lore raw
Quoted 35 lines · Julien Grall
Quoted 16 lines · Stefano Stabellini
Quoted 15 lines · Manish

xen/include/asm-arm/pci.h line 5

Quoted 12 lines
xen/include/asm-arm/pci.h +3 -2
index de13359..b8ec882 100644
@@ -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?

Quoted 6 lines
Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 7 lines

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,

Signature · 1 line
Julien Grall
#8 lore raw
In reply to Manish Jaggi

Review

Quoted 3 lines · Manish Jaggi

uint8_t pci_conf_read8(

(quoted diff) +2 -0
+    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.