Implement generic pci access functions to read/write the configuration
space.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Change in v2: Fixed comments
---
xen/arch/arm/pci/pci-access.c | 58 ++++++++++++++++++++++++++++++
xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++
xen/include/asm-arm/pci.h | 2 ++
3 files changed, 79 insertions(+)
diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
index 04fe9fbf92..45500cec2a 100644
--- a/xen/arch/arm/pci/pci-access.c
+++ b/xen/arch/arm/pci/pci-access.c
@@ -16,6 +16,7 @@
#include <asm/io.h>
#define INVALID_VALUE (~0U)
+#define PCI_ERR_VALUE(len) GENMASK(0, len * 8)
int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
uint32_t reg, uint32_t len, uint32_t *value)
@@ -72,6 +73,63 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
return 0;
}
+static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
+ unsigned int len)
+{
+ uint32_t val = PCI_ERR_VALUE(len);
+
+ struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
+
+ if ( unlikely(!bridge) )
+ 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) )
+ return;
+
+ if ( unlikely(!bridge->ops->write) )
+ return;
+
+ bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);
+}
+
+/*
+ * Wrappers for all PCI configuration access functions.
+ */
+
+#define PCI_OP_WRITE(size, type) \
+ void pci_conf_write##size(pci_sbdf_t sbdf, \
+ unsigned int reg, type val) \
+{ \
+ pci_config_write(sbdf, reg, size / 8, val); \
+}
+
+#define PCI_OP_READ(size, type) \
+ type pci_conf_read##size(pci_sbdf_t sbdf, \
+ unsigned int reg) \
+{ \
+ return pci_config_read(sbdf, reg, size / 8); \
+}
+
+PCI_OP_READ(8, uint8_t)
+PCI_OP_READ(16, uint16_t)
+PCI_OP_READ(32, uint32_t)
+PCI_OP_WRITE(8, uint8_t)
+PCI_OP_WRITE(16, uint16_t)
+PCI_OP_WRITE(32, uint32_t)
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 4beec14f2f..3bdc336268 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -243,6 +243,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 4b32c7088a..5406daecda 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -18,6 +18,7 @@
#ifdef CONFIG_HAS_PCI
#define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
+#define PRI_pci "%04x:%02x:%02x.%u"
extern bool_t pci_passthrough_enabled;
@@ -84,6 +85,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
uint32_t reg, uint32_t len, uint32_t value);
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);
static always_inline bool is_pci_passthrough_enabled(void)
{
--
2.17.1
Subject should have xen/arm
On Wed, 22 Sep 2021, Rahul Singh wrote:
> Implement generic pci access functions to read/write the configuration
> space.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Change in v2: Fixed comments
> ---
> xen/arch/arm/pci/pci-access.c | 58 ++++++++++++++++++++++++++++++
> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++
> xen/include/asm-arm/pci.h | 2 ++
> 3 files changed, 79 insertions(+)
>
> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
> index 04fe9fbf92..45500cec2a 100644
> --- a/xen/arch/arm/pci/pci-access.c
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -16,6 +16,7 @@
> #include <asm/io.h>
>
> #define INVALID_VALUE (~0U)
> +#define PCI_ERR_VALUE(len) GENMASK(0, len * 8)
>
> int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> uint32_t reg, uint32_t len, uint32_t *value)
> @@ -72,6 +73,63 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> return 0;
> }
>
> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
> + unsigned int len)
> +{
> + uint32_t val = PCI_ERR_VALUE(len);
> +
No blank line
> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
> +
> + if ( unlikely(!bridge) )
> + return val;
> +
> + if ( unlikely(!bridge->ops->read) )
> + return val;
> +
> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
The more I look at these casts the less I like them :-D
One idea is to move the definition of pci_sbdf_t somewhere else
entirely, for instance xen/include/xen/types.h, then we can use
pci_sbdf_t everywhere
> + 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) )
> + return;
> +
> + if ( unlikely(!bridge->ops->write) )
> + return;
> +
> + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);
> +}
> +
> +/*
> + * Wrappers for all PCI configuration access functions.
> + */
> +
> +#define PCI_OP_WRITE(size, type) \
> + void pci_conf_write##size(pci_sbdf_t sbdf, \
> + unsigned int reg, type val) \
> +{ \
> + pci_config_write(sbdf, reg, size / 8, val); \
> +}
> +
> +#define PCI_OP_READ(size, type) \
> + type pci_conf_read##size(pci_sbdf_t sbdf, \
> + unsigned int reg) \
> +{ \
> + return pci_config_read(sbdf, reg, size / 8); \
> +}
> +
> +PCI_OP_READ(8, uint8_t)
> +PCI_OP_READ(16, uint16_t)
> +PCI_OP_READ(32, uint32_t)
> +PCI_OP_WRITE(8, uint8_t)
> +PCI_OP_WRITE(16, uint16_t)
> +PCI_OP_WRITE(32, uint32_t)
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 4beec14f2f..3bdc336268 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -243,6 +243,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 4b32c7088a..5406daecda 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -18,6 +18,7 @@
> #ifdef CONFIG_HAS_PCI
>
> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
> +#define PRI_pci "%04x:%02x:%02x.%u"
This is added in this patch but it is unused here
Hi, On 23/09/2021 07:23, Stefano Stabellini wrote: >> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >> index 4b32c7088a..5406daecda 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -18,6 +18,7 @@ >> #ifdef CONFIG_HAS_PCI >> >> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) >> +#define PRI_pci "%04x:%02x:%02x.%u" > > This is added in this patch but it is unused here To add on this, this is technically not arch specific. So if this is necessary in a follow-up patch, then the definition should be moved to a generic header. Cheers, -- Julien Grall
Hi Julien, > On 23 Sep 2021, at 9:52 am, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 23/09/2021 07:23, Stefano Stabellini wrote: >>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >>> index 4b32c7088a..5406daecda 100644 >>> --- a/xen/include/asm-arm/pci.h >>> +++ b/xen/include/asm-arm/pci.h >>> @@ -18,6 +18,7 @@ >>> #ifdef CONFIG_HAS_PCI >>> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) >>> +#define PRI_pci "%04x:%02x:%02x.%u" >> This is added in this patch but it is unused here > > To add on this, this is technically not arch specific. So if this is necessary in a follow-up patch, then the definition should be moved to a generic header. I introduces this to print SDBF but I think it is better to use %pp to print SBDF. I will remove this in next version. Regards, Rahul > Cheers, > > -- > Julien Grall
Hi Stefano,
On 23/09/2021 07:23, Stefano Stabellini wrote:
> Subject should have xen/arm
>
>
> On Wed, 22 Sep 2021, Rahul Singh wrote:
>> Implement generic pci access functions to read/write the configuration
>> space.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Change in v2: Fixed comments
>> ---
>> xen/arch/arm/pci/pci-access.c | 58 ++++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++
>> xen/include/asm-arm/pci.h | 2 ++
>> 3 files changed, 79 insertions(+)
>>
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> index 04fe9fbf92..45500cec2a 100644
>> --- a/xen/arch/arm/pci/pci-access.c
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -16,6 +16,7 @@
>> #include <asm/io.h>
>>
>> #define INVALID_VALUE (~0U)
>> +#define PCI_ERR_VALUE(len) GENMASK(0, len * 8)
>>
>> int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> uint32_t reg, uint32_t len, uint32_t *value)
>> @@ -72,6 +73,63 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> return 0;
>> }
>>
>> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>> + unsigned int len)
>> +{
>> + uint32_t val = PCI_ERR_VALUE(len);
>> +
>
> No blank line
>
>
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + return val;
>> +
>> + if ( unlikely(!bridge->ops->read) )
>> + return val;
>> +
>> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>
> The more I look at these casts the less I like them :-D
I really dislike them. This is kind of defeating the purpose of trying
to be more typesafe.
>
> One idea is to move the definition of pci_sbdf_t somewhere else
> entirely, for instance xen/include/xen/types.h, then we can use
> pci_sbdf_t everywhere
AFAIU, the problem is the prototype helpers are defined in asm/pci.h
which is included by xen/pci.h before defining sbdf_t. Is it correct?
If so there are two options:
1) define sbdf_t and then include asm/pci.h.
2) Name the union and then pre-declare it.
Option 1 is probably nicer is we have more types in the future that are
used by arch specific but defined in the common headers. We have a few
places that uses this approach.
Cheers,
--
Julien Grall
Hi Julien,
> On 23 Sep 2021, at 10:02 am, Julien Grall <julien@xen.org> wrote:
>
> Hi Stefano,
>
> On 23/09/2021 07:23, Stefano Stabellini wrote:
>> Subject should have xen/arm
>> On Wed, 22 Sep 2021, Rahul Singh wrote:
>>> Implement generic pci access functions to read/write the configuration
>>> space.
>>>
>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>>> ---
>>> Change in v2: Fixed comments
>>> ---
>>> xen/arch/arm/pci/pci-access.c | 58 ++++++++++++++++++++++++++++++
>>> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++
>>> xen/include/asm-arm/pci.h | 2 ++
>>> 3 files changed, 79 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>>> index 04fe9fbf92..45500cec2a 100644
>>> --- a/xen/arch/arm/pci/pci-access.c
>>> +++ b/xen/arch/arm/pci/pci-access.c
>>> @@ -16,6 +16,7 @@
>>> #include <asm/io.h>
>>> #define INVALID_VALUE (~0U)
>>> +#define PCI_ERR_VALUE(len) GENMASK(0, len * 8)
>>> int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>>> uint32_t reg, uint32_t len, uint32_t *value)
>>> @@ -72,6 +73,63 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>>> return 0;
>>> }
>>> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>>> + unsigned int len)
>>> +{
>>> + uint32_t val = PCI_ERR_VALUE(len);
>>> +
>> No blank line
>>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>>> +
>>> + if ( unlikely(!bridge) )
>>> + return val;
>>> +
>>> + if ( unlikely(!bridge->ops->read) )
>>> + return val;
>>> +
>>> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>> The more I look at these casts the less I like them :-D
>
> I really dislike them. This is kind of defeating the purpose of trying to be more typesafe.
>
>> One idea is to move the definition of pci_sbdf_t somewhere else
>> entirely, for instance xen/include/xen/types.h, then we can use
>> pci_sbdf_t everywhere
>
> AFAIU, the problem is the prototype helpers are defined in asm/pci.h which is included by xen/pci.h before defining sbdf_t. Is it correct?
>
> If so there are two options:
> 1) define sbdf_t and then include asm/pci.h.
> 2) Name the union and then pre-declare it.
>
> Option 1 is probably nicer is we have more types in the future that are used by arch specific but defined in the common headers. We have a few places that uses this approach.
>
Thanks for the pointer I will fix this in next version.
Regards,
Rahul
> Cheers,
>
> --
> Julien Grall
Hi Stefano,
> On 23 Sep 2021, at 3:23 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> Subject should have xen/arm
>
>
> On Wed, 22 Sep 2021, Rahul Singh wrote:
>> Implement generic pci access functions to read/write the configuration
>> space.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Change in v2: Fixed comments
>> ---
>> xen/arch/arm/pci/pci-access.c | 58 ++++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-common.c | 19 ++++++++++
>> xen/include/asm-arm/pci.h | 2 ++
>> 3 files changed, 79 insertions(+)
>>
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> index 04fe9fbf92..45500cec2a 100644
>> --- a/xen/arch/arm/pci/pci-access.c
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -16,6 +16,7 @@
>> #include <asm/io.h>
>>
>> #define INVALID_VALUE (~0U)
>> +#define PCI_ERR_VALUE(len) GENMASK(0, len * 8)
>>
>> int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> uint32_t reg, uint32_t len, uint32_t *value)
>> @@ -72,6 +73,63 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> return 0;
>> }
>>
>> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
>> + unsigned int len)
>> +{
>> + uint32_t val = PCI_ERR_VALUE(len);
>> +
>
> No blank line
Ack.
>
>
>> + struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
>> +
>> + if ( unlikely(!bridge) )
>> + return val;
>> +
>> + if ( unlikely(!bridge->ops->read) )
>> + return val;
>> +
>> + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val);
>
> The more I look at these casts the less I like them :-D
>
> One idea is to move the definition of pci_sbdf_t somewhere else
> entirely, for instance xen/include/xen/types.h, then we can use
> pci_sbdf_t everywhere
>
Ok. Let me try to get rid of this in the next version.
>
>> + 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) )
>> + return;
>> +
>> + if ( unlikely(!bridge->ops->write) )
>> + return;
>> +
>> + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val);
>> +}
>> +
>> +/*
>> + * Wrappers for all PCI configuration access functions.
>> + */
>> +
>> +#define PCI_OP_WRITE(size, type) \
>> + void pci_conf_write##size(pci_sbdf_t sbdf, \
>> + unsigned int reg, type val) \
>> +{ \
>> + pci_config_write(sbdf, reg, size / 8, val); \
>> +}
>> +
>> +#define PCI_OP_READ(size, type) \
>> + type pci_conf_read##size(pci_sbdf_t sbdf, \
>> + unsigned int reg) \
>> +{ \
>> + return pci_config_read(sbdf, reg, size / 8); \
>> +}
>> +
>> +PCI_OP_READ(8, uint8_t)
>> +PCI_OP_READ(16, uint16_t)
>> +PCI_OP_READ(32, uint32_t)
>> +PCI_OP_WRITE(8, uint8_t)
>> +PCI_OP_WRITE(16, uint16_t)
>> +PCI_OP_WRITE(32, uint32_t)
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 4beec14f2f..3bdc336268 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -243,6 +243,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 4b32c7088a..5406daecda 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -18,6 +18,7 @@
>> #ifdef CONFIG_HAS_PCI
>>
>> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
>> +#define PRI_pci "%04x:%02x:%02x.%u"
>
> This is added in this patch but it is unused here
Ack. I will remove this and will use %pp to print SBDF.
Regards,
Rahul
© 2016 - 2024 Red Hat, Inc.