Implement generic pci access functions to read/write the configuration
space.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/arch/arm/pci/pci-access.c | 31 +++++++++++++++++++++++++++++-
xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++++++++++
xen/include/asm-arm/pci.h | 2 ++
3 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
index f39f6a3a38..b94de3c3ac 100644
--- a/xen/arch/arm/pci/pci-access.c
+++ b/xen/arch/arm/pci/pci-access.c
@@ -72,12 +72,41 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
unsigned int len)
{
- return ~0U;
+ uint32_t val = GENMASK(0, len * 8);
+
+ struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
+
+ if ( unlikely(!bridge) )
+ {
+ printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
+ sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
+ return val;
+ }
+
+ if ( unlikely(!bridge->ops->read) )
+ return val;
+
+ bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
+
+ return val;
}
static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
unsigned int len, uint32_t val)
{
+ struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
+
+ if ( unlikely(!bridge) )
+ {
+ printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
+ sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
+ return;
+ }
+
+ if ( unlikely(!bridge->ops->write) )
+ return;
+
+ bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);
}
/*
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index c582527e92..62715b4676 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -261,6 +261,25 @@ err_exit:
return err;
}
+/*
+ * This function will lookup an hostbridge based on the segment and bus
+ * number.
+ */
+struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus)
+{
+ struct pci_host_bridge *bridge;
+
+ list_for_each_entry( bridge, &pci_host_bridges, node )
+ {
+ if ( bridge->segment != segment )
+ continue;
+ if ( (bus < bridge->bus_start) || (bus > bridge->bus_end) )
+ continue;
+ return bridge;
+ }
+
+ return NULL;
+}
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 22866244d2..756f8637ab 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -20,6 +20,7 @@
#ifdef CONFIG_HAS_PCI
#define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
+#define PRI_pci "%04x:%02x:%02x.%u"
/* Arch pci dev struct */
struct arch_pci_dev {
@@ -86,6 +87,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
uint32_t sbdf, uint32_t where);
+struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
#else /*!CONFIG_HAS_PCI*/
struct arch_pci_dev { };
--
2.17.1
On Thu, 19 Aug 2021, Rahul Singh wrote:
> Implement generic pci access functions to read/write the configuration
> space.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/arm/pci/pci-access.c | 31 +++++++++++++++++++++++++++++-
> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++++++++++
> xen/include/asm-arm/pci.h | 2 ++
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
> index f39f6a3a38..b94de3c3ac 100644
> --- a/xen/arch/arm/pci/pci-access.c
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -72,12 +72,41 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
> unsigned int len)
> {
> - return ~0U;
> + uint32_t val = GENMASK(0, len * 8);
This seems to be another default error value that it would be better to
define with its own macro
> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> +
> + if ( unlikely(!bridge) )
> + {
> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
You are not actually printing sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn ?
> + return val;
> + }
> +
> + if ( unlikely(!bridge->ops->read) )
> + return val;
> +
> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
Would it make sense to make the interface take a pci_sbdf_t directly
instead of casting to uint32_t and back?
> + return val;
> }
>
> static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
> unsigned int len, uint32_t val)
> {
> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> +
> + if ( unlikely(!bridge) )
> + {
> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
same here
> + return;
> + }
> +
> + if ( unlikely(!bridge->ops->write) )
> + return;
> +
> + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);
same here
> }
>
> /*
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index c582527e92..62715b4676 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -261,6 +261,25 @@ err_exit:
> return err;
> }
>
> +/*
> + * This function will lookup an hostbridge based on the segment and bus
> + * number.
> + */
> +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus)
> +{
> + struct pci_host_bridge *bridge;
> +
> + list_for_each_entry( bridge, &pci_host_bridges, node )
> + {
> + if ( bridge->segment != segment )
> + continue;
> + if ( (bus < bridge->bus_start) || (bus > bridge->bus_end) )
> + continue;
> + return bridge;
> + }
> +
> + return NULL;
> +}
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 22866244d2..756f8637ab 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -20,6 +20,7 @@
> #ifdef CONFIG_HAS_PCI
>
> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
> +#define PRI_pci "%04x:%02x:%02x.%u"
>
> /* Arch pci dev struct */
> struct arch_pci_dev {
> @@ -86,6 +87,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> uint32_t sbdf, uint32_t where);
>
> +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
> #else /*!CONFIG_HAS_PCI*/
>
> struct arch_pci_dev { };
> --
> 2.17.1
>
Hi Stefano,
> On 10 Sep 2021, at 12:41 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Thu, 19 Aug 2021, Rahul Singh wrote:
>> Implement generic pci access functions to read/write the configuration
>> space.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/arch/arm/pci/pci-access.c | 31 +++++++++++++++++++++++++++++-
>> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++++++++++
>> xen/include/asm-arm/pci.h | 2 ++
>> 3 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> index f39f6a3a38..b94de3c3ac 100644
>> --- a/xen/arch/arm/pci/pci-access.c
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -72,12 +72,41 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>> unsigned int len)
>> {
>> - return ~0U;
>> + uint32_t val = GENMASK(0, len * 8);
>
> This seems to be another default error value that it would be better to
> define with its own macro
This default error is used once do you want to me define as macro.
>
>
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + {
>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>
> You are not actually printing sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn ?
Yes I am printing with “PRI_pci".
>
>
>> + return val;
>> + }
>> +
>> + if ( unlikely(!bridge->ops->read) )
>> + return val;
>> +
>> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>
> Would it make sense to make the interface take a pci_sbdf_t directly
> instead of casting to uint32_t and back?
pci_sbdf_t is defined in "xen/pci.h” and "xen/pci.h” includes "asm-arm/pci.h”.
If I modify the function argument in "asm-arm/pci.h” to use pci_sbdf_t I will get compilation error.
>
>
>> + return val;
>> }
>>
>> static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
>> unsigned int len, uint32_t val)
>> {
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + {
>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>
> same here
Yes I am printing with “PRI_pci".
>
Regards
Rahul
+Jan for the header include question at the bottom
On Tue, 14 Sep 2021, Rahul Singh wrote:
> Hi Stefano,
>
> > On 10 Sep 2021, at 12:41 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Thu, 19 Aug 2021, Rahul Singh wrote:
> >> Implement generic pci access functions to read/write the configuration
> >> space.
> >>
> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> >> ---
> >> xen/arch/arm/pci/pci-access.c | 31 +++++++++++++++++++++++++++++-
> >> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++++++++++
> >> xen/include/asm-arm/pci.h | 2 ++
> >> 3 files changed, 51 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
> >> index f39f6a3a38..b94de3c3ac 100644
> >> --- a/xen/arch/arm/pci/pci-access.c
> >> +++ b/xen/arch/arm/pci/pci-access.c
> >> @@ -72,12 +72,41 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
> >> unsigned int len)
> >> {
> >> - return ~0U;
> >> + uint32_t val = GENMASK(0, len * 8);
> >
> > This seems to be another default error value that it would be better to
> > define with its own macro
>
> This default error is used once do you want to me define as macro.
Yes. A macro is good even if you are going to use it once because it
also serves as documentation for the error. For instance:
/* PCI host bridge not found */
#define PCI_ERR_NOTFOUND(len) GENMASK(0, len * 8)
really helps with the explanation of what the error is about.
> >> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> >> +
> >> + if ( unlikely(!bridge) )
> >> + {
> >> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
> >> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
> >
> > You are not actually printing sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn ?
>
> Yes I am printing with “PRI_pci".
Sorry I missed it!
> >> + return val;
> >> + }
> >> +
> >> + if ( unlikely(!bridge->ops->read) )
> >> + return val;
> >> +
> >> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
> >
> > Would it make sense to make the interface take a pci_sbdf_t directly
> > instead of casting to uint32_t and back?
>
> pci_sbdf_t is defined in "xen/pci.h” and "xen/pci.h” includes "asm-arm/pci.h”.
> If I modify the function argument in "asm-arm/pci.h” to use pci_sbdf_t I will get compilation error.
This is unfortunate. One way around it is to make the appended change to
xen/include/xen/pci.h and then simply add:
typedef union pci_sbdf pci_sbdf_t;
to xen/include/asm-arm/pci.h.
Jan do you have any better suggestions?
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8e3d4d9454..ae8d48135b 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -41,7 +41,7 @@
#define PCI_SBDF3(s,b,df) \
((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) })
-typedef union {
+union pci_sbdf {
uint32_t sbdf;
struct {
union {
@@ -60,7 +60,9 @@ typedef union {
};
uint16_t seg;
};
-} pci_sbdf_t;
+};
+
+typedef union pci_sbdf pci_sbdf_t;
struct pci_dev_info {
/*
Hi, Rahul!
>>> static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
>>> unsigned int len, uint32_t val)
>>> {
>>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>>> +
>>> + if ( unlikely(!bridge) )
>>> + {
>>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>> same here
> Yes I am printing with “PRI_pci".
vPCI and the rest are widely using
printk("%pp\n", &sbdf);
So, I think if we have SBDF then it is better to use %pp instead of trying to unfold it manually.
Hi Oleksandr,
> On 15 Sep 2021, at 8:54 am, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
>
> Hi, Rahul!
>>>> static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
>>>> unsigned int len, uint32_t val)
>>>> {
>>>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>>>> +
>>>> + if ( unlikely(!bridge) )
>>>> + {
>>>> + printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
>>>> + sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);
>>> same here
>> Yes I am printing with “PRI_pci".
>
> vPCI and the rest are widely using
>
> printk("%pp\n", &sbdf);
> So, I think if we have SBDF then it is better to use %pp instead of trying to unfold it manually.
Ok. I will use the %pp for printing the SBDF.
Regards,
Rahul
© 2016 - 2024 Red Hat, Inc.