Add a new function to emulate extended capability list for dom0,
and call it in init_header(). So that it will be easy to hide a
extended capability whose initialization fails.
As for the extended capability list of domU, just move the logic
into above function and keep hiding it for domU.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v2->v3 changes:
* In vpci_init_ext_capability_list(), when domain is domU, directly return after adding a handler(hiding all extended capability for domU).
* In vpci_init_ext_capability_list(), change condition to be "while ( pos >= 0x100U && ttl-- )" instead of "while ( pos && ttl-- )".
* Add new function vpci_hw_write32, and pass it to extended capability handler for dom0.
v1->v2 changes:
new patch
Best regards,
Jiqian Chen.
---
xen/drivers/vpci/header.c | 36 ++++++++++++++++++++++++++++--------
xen/drivers/vpci/vpci.c | 6 ++++++
xen/include/xen/vpci.h | 2 ++
3 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index c98cd211d9d7..ee94ad8e5037 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -817,6 +817,31 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
PCI_STATUS_RSVDZ_MASK);
}
+static int vpci_init_ext_capability_list(struct pci_dev *pdev)
+{
+ unsigned int pos = PCI_CFG_SPACE_SIZE, ttl = 480;
+
+ if ( !is_hardware_domain(pdev->domain) )
+ /* Extended capabilities read as zero, write ignore for guest */
+ return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
+ pos, 4, (void *)0);
+
+ while ( pos >= PCI_CFG_SPACE_SIZE && ttl-- )
+ {
+ uint32_t header = pci_conf_read32(pdev->sbdf, pos);
+ int rc;
+
+ rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
+ pos, 4, (void *)(uintptr_t)header);
+ if ( rc )
+ return rc;
+
+ pos = PCI_EXT_CAP_NEXT(header);
+ }
+
+ return 0;
+}
+
static int cf_check init_header(struct pci_dev *pdev)
{
uint16_t cmd;
@@ -869,14 +894,9 @@ static int cf_check init_header(struct pci_dev *pdev)
if ( rc )
return rc;
- if ( !is_hwdom )
- {
- /* Extended capabilities read as zero, write ignore */
- rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, 0x100, 4,
- (void *)0);
- if ( rc )
- return rc;
- }
+ rc = vpci_init_ext_capability_list(pdev);
+ if ( rc )
+ return rc;
if ( pdev->ignore_bars )
return 0;
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1e6aa5d799b9..3349b98389b8 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -232,6 +232,12 @@ void cf_check vpci_hw_write16(
pci_conf_write16(pdev->sbdf, reg, val);
}
+void cf_check vpci_hw_write32(
+ const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
+{
+ pci_conf_write32(pdev->sbdf, reg, val);
+}
+
int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler,
vpci_write_t *write_handler, unsigned int offset,
unsigned int size, void *data, uint32_t ro_mask,
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 807401b2eaa2..9d47b8c1a50e 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -78,6 +78,8 @@ uint32_t cf_check vpci_hw_read32(
const struct pci_dev *pdev, unsigned int reg, void *data);
void cf_check vpci_hw_write16(
const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
+void cf_check vpci_hw_write32(
+ const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
/*
* Check for pending vPCI operations on this vcpu. Returns true if the vcpu
--
2.34.1
On Mon, Apr 21, 2025 at 02:18:56PM +0800, Jiqian Chen wrote:
> Add a new function to emulate extended capability list for dom0,
> and call it in init_header(). So that it will be easy to hide a
> extended capability whose initialization fails.
>
> As for the extended capability list of domU, just move the logic
> into above function and keep hiding it for domU.
>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v2->v3 changes:
> * In vpci_init_ext_capability_list(), when domain is domU, directly return after adding a handler(hiding all extended capability for domU).
> * In vpci_init_ext_capability_list(), change condition to be "while ( pos >= 0x100U && ttl-- )" instead of "while ( pos && ttl-- )".
> * Add new function vpci_hw_write32, and pass it to extended capability handler for dom0.
>
> v1->v2 changes:
> new patch
>
> Best regards,
> Jiqian Chen.
> ---
> xen/drivers/vpci/header.c | 36 ++++++++++++++++++++++++++++--------
> xen/drivers/vpci/vpci.c | 6 ++++++
> xen/include/xen/vpci.h | 2 ++
> 3 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index c98cd211d9d7..ee94ad8e5037 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -817,6 +817,31 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
> PCI_STATUS_RSVDZ_MASK);
> }
>
> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
> +{
> + unsigned int pos = PCI_CFG_SPACE_SIZE, ttl = 480;
> +
> + if ( !is_hardware_domain(pdev->domain) )
> + /* Extended capabilities read as zero, write ignore for guest */
> + return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> + pos, 4, (void *)0);
> +
> + while ( pos >= PCI_CFG_SPACE_SIZE && ttl-- )
> + {
> + uint32_t header = pci_conf_read32(pdev->sbdf, pos);
> + int rc;
I'm thinking it might be helpful to avoid setting the handler for the
last capability on the list, or simply for devices that have no
extended capabilities at all:
if ( PCI_EXT_CAP_NEXT(header) >= PCI_CFG_SPACE_SIZE )
{
int rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
pos, 4, (void *)(uintptr_t)header);
if ( rc )
return rc;
}
Otherwise on systems with a lot of devices it can be quite wasteful to
set a handler to just return the next capability as 0.
Thanks, Roger.
On 2025/5/6 22:14, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:18:56PM +0800, Jiqian Chen wrote:
>> Add a new function to emulate extended capability list for dom0,
>> and call it in init_header(). So that it will be easy to hide a
>> extended capability whose initialization fails.
>>
>> As for the extended capability list of domU, just move the logic
>> into above function and keep hiding it for domU.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v2->v3 changes:
>> * In vpci_init_ext_capability_list(), when domain is domU, directly return after adding a handler(hiding all extended capability for domU).
>> * In vpci_init_ext_capability_list(), change condition to be "while ( pos >= 0x100U && ttl-- )" instead of "while ( pos && ttl-- )".
>> * Add new function vpci_hw_write32, and pass it to extended capability handler for dom0.
>>
>> v1->v2 changes:
>> new patch
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>> xen/drivers/vpci/header.c | 36 ++++++++++++++++++++++++++++--------
>> xen/drivers/vpci/vpci.c | 6 ++++++
>> xen/include/xen/vpci.h | 2 ++
>> 3 files changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index c98cd211d9d7..ee94ad8e5037 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -817,6 +817,31 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>> PCI_STATUS_RSVDZ_MASK);
>> }
>>
>> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
>> +{
>> + unsigned int pos = PCI_CFG_SPACE_SIZE, ttl = 480;
>> +
>> + if ( !is_hardware_domain(pdev->domain) )
>> + /* Extended capabilities read as zero, write ignore for guest */
>> + return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> + pos, 4, (void *)0);
>> +
>> + while ( pos >= PCI_CFG_SPACE_SIZE && ttl-- )
>> + {
>> + uint32_t header = pci_conf_read32(pdev->sbdf, pos);
>> + int rc;
>
> I'm thinking it might be helpful to avoid setting the handler for the
> last capability on the list, or simply for devices that have no
> extended capabilities at all:
>
> if ( PCI_EXT_CAP_NEXT(header) >= PCI_CFG_SPACE_SIZE )
> {
> int rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
> pos, 4, (void *)(uintptr_t)header);
>
> if ( rc )
> return rc;
> }
But if adding this check, there is a problem, think about this situation:
a device only has one extended capability, then under your check, it does not add handler for it,
if the initialization of that extended capability fails, we can't hide it by removing handler from vpci.
If you want to avoid adding handler for devices that have no extended capabilities.
I think adding check
If ( header == 0 )
return 0;
is enough.
>
> Otherwise on systems with a lot of devices it can be quite wasteful to
> set a handler to just return the next capability as 0.
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
On Wed, May 07, 2025 at 03:32:47AM +0000, Chen, Jiqian wrote:
> On 2025/5/6 22:14, Roger Pau Monné wrote:
> > On Mon, Apr 21, 2025 at 02:18:56PM +0800, Jiqian Chen wrote:
> >> Add a new function to emulate extended capability list for dom0,
> >> and call it in init_header(). So that it will be easy to hide a
> >> extended capability whose initialization fails.
> >>
> >> As for the extended capability list of domU, just move the logic
> >> into above function and keep hiding it for domU.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> >> ---
> >> v2->v3 changes:
> >> * In vpci_init_ext_capability_list(), when domain is domU, directly return after adding a handler(hiding all extended capability for domU).
> >> * In vpci_init_ext_capability_list(), change condition to be "while ( pos >= 0x100U && ttl-- )" instead of "while ( pos && ttl-- )".
> >> * Add new function vpci_hw_write32, and pass it to extended capability handler for dom0.
> >>
> >> v1->v2 changes:
> >> new patch
> >>
> >> Best regards,
> >> Jiqian Chen.
> >> ---
> >> xen/drivers/vpci/header.c | 36 ++++++++++++++++++++++++++++--------
> >> xen/drivers/vpci/vpci.c | 6 ++++++
> >> xen/include/xen/vpci.h | 2 ++
> >> 3 files changed, 36 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> >> index c98cd211d9d7..ee94ad8e5037 100644
> >> --- a/xen/drivers/vpci/header.c
> >> +++ b/xen/drivers/vpci/header.c
> >> @@ -817,6 +817,31 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
> >> PCI_STATUS_RSVDZ_MASK);
> >> }
> >>
> >> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
> >> +{
> >> + unsigned int pos = PCI_CFG_SPACE_SIZE, ttl = 480;
> >> +
> >> + if ( !is_hardware_domain(pdev->domain) )
> >> + /* Extended capabilities read as zero, write ignore for guest */
> >> + return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> >> + pos, 4, (void *)0);
> >> +
> >> + while ( pos >= PCI_CFG_SPACE_SIZE && ttl-- )
> >> + {
> >> + uint32_t header = pci_conf_read32(pdev->sbdf, pos);
> >> + int rc;
> >
> > I'm thinking it might be helpful to avoid setting the handler for the
> > last capability on the list, or simply for devices that have no
> > extended capabilities at all:
> >
> > if ( PCI_EXT_CAP_NEXT(header) >= PCI_CFG_SPACE_SIZE )
> > {
> > int rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
> > pos, 4, (void *)(uintptr_t)header);
> >
> > if ( rc )
> > return rc;
> > }
> But if adding this check, there is a problem, think about this situation:
> a device only has one extended capability, then under your check, it does not add handler for it,
> if the initialization of that extended capability fails, we can't hide it by removing handler from vpci.
> If you want to avoid adding handler for devices that have no extended capabilities.
> I think adding check
> If ( header == 0 )
> return 0;
> is enough.
Hm, yes, extended PCI capabilities don't have a start pointer like
legacy ones, so masking the initial capability (as you have discovered)
is not easy. I agree with checking whether the initial header == 0
and then not adding any handler at all.
Thanks, Roger.
© 2016 - 2025 Red Hat, Inc.