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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

+Jan for the header include question at the bottom


On Tue, 14 Sep 2021, Rahul Singh wrote:
> 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.  

Yes. A macro is good even if you are going to use it once because it
also serves as documentation for the error. For instance:

/* PCI host bridge not found */
#define PCI_ERR_NOTFOUND(len) GENMASK(0, len * 8)

really helps with the explanation of what the error is about.


> >> +    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".

Sorry I missed it!


> >> +        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.

This is unfortunate. One way around it is to make the appended change to
xen/include/xen/pci.h and then simply add:

typedef union pci_sbdf pci_sbdf_t;

to xen/include/asm-arm/pci.h.

Jan do you have any better suggestions?


diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8e3d4d9454..ae8d48135b 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -41,7 +41,7 @@
 #define PCI_SBDF3(s,b,df) \
     ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) })
 
-typedef union {
+union pci_sbdf {
     uint32_t sbdf;
     struct {
         union {
@@ -60,7 +60,9 @@ typedef union {
         };
         uint16_t                seg;
     };
-} pci_sbdf_t;
+};
+
+typedef union pci_sbdf pci_sbdf_t;
 
 struct pci_dev_info {
     /*