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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 23 Sep 2021, at 10:02 am, Julien Grall <julien@xxxxxxx> 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@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
>>> +    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