[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.