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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall