| Message ID | f05b02c04803809bbe4ebd49ed4abb5a4656b010.1632307952.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm | expand |
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@arm.com> > --- > 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 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 > + 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) ) > + return; > + > + if ( unlikely(!bridge->ops->write) ) > + return; > + > + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val); > +} > + > +/* > + * Wrappers for all PCI configuration access functions. > + */ > + > +#define PCI_OP_WRITE(size, type) \ > + void pci_conf_write##size(pci_sbdf_t sbdf, \ > + unsigned int reg, type val) \ > +{ \ > + pci_config_write(sbdf, reg, size / 8, val); \ > +} > + > +#define PCI_OP_READ(size, type) \ > + type pci_conf_read##size(pci_sbdf_t sbdf, \ > + unsigned int reg) \ > +{ \ > + return pci_config_read(sbdf, reg, size / 8); \ > +} > + > +PCI_OP_READ(8, uint8_t) > +PCI_OP_READ(16, uint16_t) > +PCI_OP_READ(32, uint32_t) > +PCI_OP_WRITE(8, uint8_t) > +PCI_OP_WRITE(16, uint16_t) > +PCI_OP_WRITE(32, uint32_t) > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c > index 4beec14f2f..3bdc336268 100644 > --- a/xen/arch/arm/pci/pci-host-common.c > +++ b/xen/arch/arm/pci/pci-host-common.c > @@ -243,6 +243,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 4b32c7088a..5406daecda 100644 > --- a/xen/include/asm-arm/pci.h > +++ b/xen/include/asm-arm/pci.h > @@ -18,6 +18,7 @@ > #ifdef CONFIG_HAS_PCI > > #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) > +#define PRI_pci "%04x:%02x:%02x.%u" This is added in this patch but it is unused here
Hi, On 23/09/2021 07:23, Stefano Stabellini wrote: >> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >> index 4b32c7088a..5406daecda 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -18,6 +18,7 @@ >> #ifdef CONFIG_HAS_PCI >> >> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) >> +#define PRI_pci "%04x:%02x:%02x.%u" > > This is added in this patch but it is unused here To add on this, this is technically not arch specific. So if this is necessary in a follow-up patch, then the definition should be moved to a generic header. Cheers,
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@arm.com> >> --- >> 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,
Hi Stefano, > On 23 Sep 2021, at 3:23 am, Stefano Stabellini <sstabellini@kernel.org> 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@arm.com> >> --- >> 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 Ack. > > >> + 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 > > 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 > Ok. Let me try to get rid of this in the next version. > >> + 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) ) >> + return; >> + >> + if ( unlikely(!bridge->ops->write) ) >> + return; >> + >> + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val); >> +} >> + >> +/* >> + * Wrappers for all PCI configuration access functions. >> + */ >> + >> +#define PCI_OP_WRITE(size, type) \ >> + void pci_conf_write##size(pci_sbdf_t sbdf, \ >> + unsigned int reg, type val) \ >> +{ \ >> + pci_config_write(sbdf, reg, size / 8, val); \ >> +} >> + >> +#define PCI_OP_READ(size, type) \ >> + type pci_conf_read##size(pci_sbdf_t sbdf, \ >> + unsigned int reg) \ >> +{ \ >> + return pci_config_read(sbdf, reg, size / 8); \ >> +} >> + >> +PCI_OP_READ(8, uint8_t) >> +PCI_OP_READ(16, uint16_t) >> +PCI_OP_READ(32, uint32_t) >> +PCI_OP_WRITE(8, uint8_t) >> +PCI_OP_WRITE(16, uint16_t) >> +PCI_OP_WRITE(32, uint32_t) >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c >> index 4beec14f2f..3bdc336268 100644 >> --- a/xen/arch/arm/pci/pci-host-common.c >> +++ b/xen/arch/arm/pci/pci-host-common.c >> @@ -243,6 +243,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 4b32c7088a..5406daecda 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -18,6 +18,7 @@ >> #ifdef CONFIG_HAS_PCI >> >> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) >> +#define PRI_pci "%04x:%02x:%02x.%u" > > This is added in this patch but it is unused here Ack. I will remove this and will use %pp to print SBDF. Regards, Rahul
Hi Julien, > On 23 Sep 2021, at 9:52 am, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 23/09/2021 07:23, Stefano Stabellini wrote: >>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >>> index 4b32c7088a..5406daecda 100644 >>> --- a/xen/include/asm-arm/pci.h >>> +++ b/xen/include/asm-arm/pci.h >>> @@ -18,6 +18,7 @@ >>> #ifdef CONFIG_HAS_PCI >>> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) >>> +#define PRI_pci "%04x:%02x:%02x.%u" >> This is added in this patch but it is unused here > > To add on this, this is technically not arch specific. So if this is necessary in a follow-up patch, then the definition should be moved to a generic header. I introduces this to print SDBF but I think it is better to use %pp to print SBDF. I will remove this in next version. Regards, Rahul > Cheers, > > -- > Julien Grall
Hi Julien, > On 23 Sep 2021, at 10:02 am, Julien Grall <julien@xen.org> 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@arm.com> >>> --- >>> 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
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); + + 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); + + 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) ) + return; + + if ( unlikely(!bridge->ops->write) ) + return; + + bridge->ops->write(bridge, (uint32_t) sbdf.sbdf, reg, len, val); +} + +/* + * Wrappers for all PCI configuration access functions. + */ + +#define PCI_OP_WRITE(size, type) \ + void pci_conf_write##size(pci_sbdf_t sbdf, \ + unsigned int reg, type val) \ +{ \ + pci_config_write(sbdf, reg, size / 8, val); \ +} + +#define PCI_OP_READ(size, type) \ + type pci_conf_read##size(pci_sbdf_t sbdf, \ + unsigned int reg) \ +{ \ + return pci_config_read(sbdf, reg, size / 8); \ +} + +PCI_OP_READ(8, uint8_t) +PCI_OP_READ(16, uint16_t) +PCI_OP_READ(32, uint32_t) +PCI_OP_WRITE(8, uint8_t) +PCI_OP_WRITE(16, uint16_t) +PCI_OP_WRITE(32, uint32_t) + /* * Local variables: * mode: C diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c index 4beec14f2f..3bdc336268 100644 --- a/xen/arch/arm/pci/pci-host-common.c +++ b/xen/arch/arm/pci/pci-host-common.c @@ -243,6 +243,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 4b32c7088a..5406daecda 100644 --- a/xen/include/asm-arm/pci.h +++ b/xen/include/asm-arm/pci.h @@ -18,6 +18,7 @@ #ifdef CONFIG_HAS_PCI #define pci_to_dev(pcidev) (&(pcidev)->arch.dev) +#define PRI_pci "%04x:%02x:%02x.%u" extern bool_t pci_passthrough_enabled; @@ -84,6 +85,7 @@ int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf, uint32_t reg, uint32_t len, uint32_t value); 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); static always_inline bool is_pci_passthrough_enabled(void) {
Implement generic pci access functions to read/write the configuration space. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- 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(+)