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