Re: [PATCH 09/11] xen/arm: Setup MMIO range trap handlers for hardware domain

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3061 bytes --]

On Wed, 15 Sep 2021, Rahul Singh wrote:
> Hi Oleksandr, Stefano,
> 
> > On 15 Sep 2021, at 6:30 am, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
> > 
> > Hi, Rahul!
> > 
> > On 14.09.21 17:24, Oleksandr Andrushchenko wrote:
> >> 
> >> }
> >>>>   +static int pci_ecam_register_mmio_handler(struct domain *d,
> >>>> +                                          struct pci_host_bridge *bridge,
> >>>> +                                          const struct mmio_handler_ops *ops)
> >>>> +{
> >>>> +    struct pci_config_window *cfg = bridge->sysdata;
> >>>> +
> >>>> +    register_mmio_handler(d, ops, cfg->phys_addr, cfg->size, NULL);
> >>>> +    return 0;
> >>>> +}
> >>> Given that struct pci_config_window is generic (it is not specific to
> >>> one bridge), I wonder if we even need the .register_mmio_handler
> >>> callback here.
> >>> 
> >>> In fact,pci_host_bridge->sysdata doesn't even need to be a void*, it
> >>> could be a struct pci_config_window*, right?
> >> 
> >> Rahul, this actually may change your series.
> >> 
> >> Do you think we can do that?
> >> 
> > This is the only change requested that left unanswered by now.
> > 
> > Will it be possible that you change the API accordingly, so I can
> > 
> > implement as Stefano suggests?
> 
> We need pci_host_bridge->sysdata as void* in case we need to implement the new non-ecam PCI controller in XEN.
> Please have a look once in Linux code[1] how bridge->sysdata will be used. struct pci_config_window is used only for 
> ecam supported host controller. Different PCI host controller will have different PCI interface to access the PCI controller.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rcar-host.c#n309
> 
> I think we need bridge->sysdata in future to implement the new PCI controller.

In my opinion the pci_config_window is too important of an information
to be left inside an opaque pointer, especially when the info under
pci_config_window is both critical and vendor-neutral.

My preference would be something like this:


diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 9c28a4bdc4..c80d846da3 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -55,7 +55,6 @@ struct pci_config_window {
     uint8_t         busn_start;
     uint8_t         busn_end;
     void __iomem    *win;
-    const struct    pci_ecam_ops *ops;
 };
 
 /*
@@ -68,7 +67,8 @@ struct pci_host_bridge {
     uint16_t segment;                /* Segment number */
     u8 bus_start;                    /* Bus start of this bridge. */
     u8 bus_end;                      /* Bus end of this bridge. */
-    void *sysdata;                   /* Pointer to the config space window*/
+    struct pci_config_window* cfg;   /* Pointer to the bridge config window */
+    void *sysdata;                   /* Pointer to bridge private data */
     const struct pci_ops *ops;
 };


As a reference the attached patch builds. However, I had to remove const
where struct pci_ecam_ops *ops is used.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=cfg.patch, Size: 4990 bytes --]

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ecfa6822e4..f9d57ca0fa 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -7,6 +7,7 @@ config ARM_64
 	depends on !ARM_32
 	select 64BIT
 	select HAS_FAST_MULTIPLY
+	select HAS_PCI
 
 config ARM
 	def_bool y
diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
index d32efb7fcb..f6d0d00c1b 100644
--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -26,8 +26,9 @@
 void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
                                       uint32_t sbdf, uint32_t where)
 {
-    const struct pci_config_window *cfg = bridge->sysdata;
-    unsigned int devfn_shift = cfg->ops->bus_shift - 8;
+    const struct pci_ecam_ops *ops = bridge->sysdata;
+    const struct pci_config_window *cfg = bridge->cfg;
+    unsigned int devfn_shift = ops->bus_shift - 8;
     void __iomem *base;
 
     pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ;
@@ -37,7 +38,7 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
         return NULL;
 
     busn -= cfg->busn_start;
-    base = cfg->win + (busn << cfg->ops->bus_shift);
+    base = cfg->win + (busn << ops->bus_shift);
 
     return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
 }
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index c04be63645..41a5457e80 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -97,7 +97,6 @@ static struct pci_config_window *gen_pci_init(struct dt_device_node *dev,
 
     cfg->phys_addr = addr;
     cfg->size = size;
-    cfg->ops = ops;
 
     /*
      * On 64-bit systems, we do a single ioremap for the whole config space
@@ -225,7 +224,7 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
 }
 
 int pci_host_common_probe(struct dt_device_node *dev,
-                          const struct pci_ecam_ops *ops,
+                          struct pci_ecam_ops *ops,
                           int ecam_reg_idx)
 {
     struct pci_host_bridge *bridge;
@@ -245,7 +244,8 @@ int pci_host_common_probe(struct dt_device_node *dev,
     }
 
     bridge->dt_node = dev;
-    bridge->sysdata = cfg;
+    bridge->cfg = cfg;
+    bridge->sysdata = ops;
     bridge->ops = &ops->pci_ops;
     bridge->bus_start = cfg->busn_start;
     bridge->bus_end = cfg->busn_end;
diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
index 2d652e8910..66176f9658 100644
--- a/xen/arch/arm/pci/pci-host-generic.c
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -32,7 +32,7 @@ static const struct dt_device_match gen_pci_dt_match[] = {
 static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
 {
     const struct dt_device_match *of_id;
-    const struct pci_ecam_ops *ops;
+    struct pci_ecam_ops *ops;
 
     of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
     ops = (struct pci_ecam_ops *) of_id->data;
diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c b/xen/arch/arm/pci/pci-host-zynqmp.c
index fe103e3855..b4170c3bdd 100644
--- a/xen/arch/arm/pci/pci-host-zynqmp.c
+++ b/xen/arch/arm/pci/pci-host-zynqmp.c
@@ -32,7 +32,7 @@ static const struct dt_device_match gen_pci_dt_match[] = {
 static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
 {
     const struct dt_device_match *of_id;
-    const struct pci_ecam_ops *ops;
+    struct pci_ecam_ops *ops;
 
     of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
     ops = (struct pci_ecam_ops *) of_id->data;
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 9c28a4bdc4..c80d846da3 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -55,7 +55,6 @@ struct pci_config_window {
     uint8_t         busn_start;
     uint8_t         busn_end;
     void __iomem    *win;
-    const struct    pci_ecam_ops *ops;
 };
 
 /*
@@ -68,7 +67,8 @@ struct pci_host_bridge {
     uint16_t segment;                /* Segment number */
     u8 bus_start;                    /* Bus start of this bridge. */
     u8 bus_end;                      /* Bus end of this bridge. */
-    void *sysdata;                   /* Pointer to the config space window*/
+    struct pci_config_window* cfg;   /* Pointer to the bridge config window */
+    void *sysdata;                   /* Pointer to bridge private data */
     const struct pci_ops *ops;
 };
 
@@ -100,7 +100,7 @@ struct pci_ecam_ops {
 extern const struct pci_ecam_ops pci_generic_ecam_ops;
 
 int pci_host_common_probe(struct dt_device_node *dev,
-                          const struct pci_ecam_ops *ops,
+                          struct pci_ecam_ops *ops,
                           int ecam_reg_idx);
 int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
                             uint32_t reg, uint32_t len, uint32_t *value);