Re: [PATCH v1 08/14] xen:arm: Implement pci access functions

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

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