| Message ID | c90c3088a592b41c477a0026446294a3b9422f76.1629366665.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
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@arm.com> > --- > 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 > + 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 ? > + 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? > + 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 > + return; > + } > + > + if ( unlikely(!bridge->ops->write) ) > + return; > + > + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val); same here > } > > /* > diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c > index c582527e92..62715b4676 100644 > --- a/xen/arch/arm/pci/pci-host-common.c > +++ b/xen/arch/arm/pci/pci-host-common.c > @@ -261,6 +261,25 @@ err_exit: > return err; > } > > +/* > + * This function will lookup an hostbridge based on the segment and bus > + * number. > + */ > +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus) > +{ > + struct pci_host_bridge *bridge; > + > + list_for_each_entry( bridge, &pci_host_bridges, node ) > + { > + if ( bridge->segment != segment ) > + continue; > + if ( (bus < bridge->bus_start) || (bus > bridge->bus_end) ) > + continue; > + return bridge; > + } > + > + return NULL; > +} > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h > index 22866244d2..756f8637ab 100644 > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -20,6 +20,7 @@ > #ifdef CONFIG_HAS_PCI > > #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) > +#define PRI_pci "%04x:%02x:%02x.%u" > > /* Arch pci dev struct */ > struct arch_pci_dev { > @@ -86,6 +87,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf, > void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge, > uint32_t sbdf, uint32_t where); > > +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus); > #else /*!CONFIG_HAS_PCI*/ > > struct arch_pci_dev { }; > -- > 2.17.1 >
Hi Stefano, > On 10 Sep 2021, at 12:41 am, Stefano Stabellini <sstabellini@kernel.org> 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@arm.com> >> --- >> 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
+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@kernel.org> 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@arm.com> > >> --- > >> 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 { /*
Hi, Rahul! >>> 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". vPCI and the rest are widely using printk("%pp\n", &sbdf); So, I think if we have SBDF then it is better to use %pp instead of trying to unfold it manually.
Hi Oleksandr, > On 15 Sep 2021, at 8:54 am, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote: > > Hi, Rahul! >>>> 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". > > vPCI and the rest are widely using > > printk("%pp\n", &sbdf); > So, I think if we have SBDF then it is better to use %pp instead of trying to unfold it manually. Ok. I will use the %pp for printing the SBDF. Regards, Rahul
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); + + 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); + return val; + } + + if ( unlikely(!bridge->ops->read) ) + return val; + + bridge->ops->read(bridge, (uint32_t) sbdf.sbdf, reg, len, &val); + + 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); + return; + } + + if ( unlikely(!bridge->ops->write) ) + return; + + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val); } /* diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c index c582527e92..62715b4676 100644 --- a/xen/arch/arm/pci/pci-host-common.c +++ b/xen/arch/arm/pci/pci-host-common.c @@ -261,6 +261,25 @@ err_exit: return err; } +/* + * This function will lookup an hostbridge based on the segment and bus + * number. + */ +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus) +{ + struct pci_host_bridge *bridge; + + list_for_each_entry( bridge, &pci_host_bridges, node ) + { + if ( bridge->segment != segment ) + continue; + if ( (bus < bridge->bus_start) || (bus > bridge->bus_end) ) + continue; + return bridge; + } + + return NULL; +} /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h index 22866244d2..756f8637ab 100644 --- a/xen/include/asm-arm/pci.h +++ b/xen/include/asm-arm/pci.h @@ -20,6 +20,7 @@ #ifdef CONFIG_HAS_PCI #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) +#define PRI_pci "%04x:%02x:%02x.%u" /* Arch pci dev struct */ struct arch_pci_dev { @@ -86,6 +87,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf, void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge, uint32_t sbdf, uint32_t where); +struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus); #else /*!CONFIG_HAS_PCI*/ struct arch_pci_dev { };
Implement generic pci access functions to read/write the configuration space. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- 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(-)