Re: [PATCH v2 13/17] xen:arm: Implement pci access functions

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 23 Sep 2021, at 3:23 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxx>
>> ---
>> 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