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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 15 Sep 2021, Rahul Singh wrote:
> Hi Oleksandr, Stefano,
> 
> > On 15 Sep 2021, at 6:30 am, Oleksandr Andrushchenko 
> > <Oleksandr_Andrushchenko@xxxxxxxx> 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.

Attachment: cfg.patch
Description: Text Data