Current logic of emulating legacy capability list is only for domU.
So, expand it to emulate for dom0 too. Then it will be easy to hide
a capability whose initialization fails in a function.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v2->v3 changes:
* Not to add handler of PCI_CAP_LIST_ID when domain is dom0.
v1->v2 changes:
new patch.
Best regards,
Jiqian Chen.
---
xen/drivers/vpci/header.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 3e9b44454b43..c98cd211d9d7 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -749,9 +749,9 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
{
int rc;
bool mask_cap_list = false;
+ bool is_hwdom = is_hardware_domain(pdev->domain);
- if ( !is_hardware_domain(pdev->domain) &&
- pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
+ if ( pci_conf_read16(pdev->sbdf, PCI_STATUS) & PCI_STATUS_CAP_LIST )
{
/* Only expose capabilities to the guest that vPCI can handle. */
unsigned int next, ttl = 48;
@@ -759,10 +759,11 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
PCI_CAP_ID_MSI,
PCI_CAP_ID_MSIX,
};
+ const unsigned int *caps = is_hwdom ? NULL : supported_caps;
+ const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(supported_caps);
next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST,
- supported_caps,
- ARRAY_SIZE(supported_caps), &ttl);
+ caps, n, &ttl);
rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
PCI_CAPABILITY_LIST, 1,
@@ -772,7 +773,7 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
next &= ~3;
- if ( !next )
+ if ( !next && !is_hwdom )
/*
* If we don't have any supported capabilities to expose to the
* guest, mask the PCI_STATUS_CAP_LIST bit in the status
@@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
next = pci_find_next_cap_ttl(pdev->sbdf,
pos + PCI_CAP_LIST_NEXT,
- supported_caps,
- ARRAY_SIZE(supported_caps), &ttl);
+ caps, n, &ttl);
- rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
- pos + PCI_CAP_LIST_ID, 1, NULL);
- if ( rc )
- return rc;
+ if ( !is_hwdom )
+ {
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
+ pos + PCI_CAP_LIST_ID, 1, NULL);
+ if ( rc )
+ return rc;
+ }
rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
pos + PCI_CAP_LIST_NEXT, 1,
--
2.34.1
On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote:
> Current logic of emulating legacy capability list is only for domU.
> So, expand it to emulate for dom0 too. Then it will be easy to hide
> a capability whose initialization fails in a function.
>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Sorry, one nit I've noticed while looking at the next patch.
> @@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>
> next = pci_find_next_cap_ttl(pdev->sbdf,
> pos + PCI_CAP_LIST_NEXT,
> - supported_caps,
> - ARRAY_SIZE(supported_caps), &ttl);
> + caps, n, &ttl);
>
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> - pos + PCI_CAP_LIST_ID, 1, NULL);
> - if ( rc )
> - return rc;
> + if ( !is_hwdom )
> + {
> + rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> + pos + PCI_CAP_LIST_ID, 1, NULL);
> + if ( rc )
> + return rc;
> + }
>
> rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
For the hardware domain the write handler should be vpci_hw_write8
instead of NULL.
Thanks, Roger.
On 2025/5/6 21:50, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote:
>> Current logic of emulating legacy capability list is only for domU.
>> So, expand it to emulate for dom0 too. Then it will be easy to hide
>> a capability whose initialization fails in a function.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>
> Sorry, one nit I've noticed while looking at the next patch.
>
>> @@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>>
>> next = pci_find_next_cap_ttl(pdev->sbdf,
>> pos + PCI_CAP_LIST_NEXT,
>> - supported_caps,
>> - ARRAY_SIZE(supported_caps), &ttl);
>> + caps, n, &ttl);
>>
>> - rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
The same here, NULL -> vpci_hw_write8, I think.
>> - pos + PCI_CAP_LIST_ID, 1, NULL);
>> - if ( rc )
>> - return rc;
>> + if ( !is_hwdom )
>> + {
>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> + pos + PCI_CAP_LIST_ID, 1, NULL);
>> + if ( rc )
>> + return rc;
>> + }
>>
>> rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>
> For the hardware domain the write handler should be vpci_hw_write8
> instead of NULL.
OK, I think I need to add definition of vpci_hw_write8.
But I have a question, if hardware domain write this register through vpci_hw_write8,
then the "next address data" of hardware will be in consistent with vpci.
Is it fine? Or should I update vpci's cache?
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
On Wed, May 07, 2025 at 02:46:52AM +0000, Chen, Jiqian wrote:
> On 2025/5/6 21:50, Roger Pau Monné wrote:
> > On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote:
> >> Current logic of emulating legacy capability list is only for domU.
> >> So, expand it to emulate for dom0 too. Then it will be easy to hide
> >> a capability whose initialization fails in a function.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >
> > Sorry, one nit I've noticed while looking at the next patch.
> >
> >> @@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
> >>
> >> next = pci_find_next_cap_ttl(pdev->sbdf,
> >> pos + PCI_CAP_LIST_NEXT,
> >> - supported_caps,
> >> - ARRAY_SIZE(supported_caps), &ttl);
> >> + caps, n, &ttl);
> >>
> >> - rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> The same here, NULL -> vpci_hw_write8, I think.
No, not here, since the PCI_CAP_LIST_ID handler is only added for
non-hardware domains, and in that case we do want to ignore writes to
the register.
> >> - pos + PCI_CAP_LIST_ID, 1, NULL);
> >> - if ( rc )
> >> - return rc;
> >> + if ( !is_hwdom )
> >> + {
> >> + rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> >> + pos + PCI_CAP_LIST_ID, 1, NULL);
> >> + if ( rc )
> >> + return rc;
> >> + }
> >>
> >> rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> >
> > For the hardware domain the write handler should be vpci_hw_write8
> > instead of NULL.
> OK, I think I need to add definition of vpci_hw_write8.
> But I have a question, if hardware domain write this register through vpci_hw_write8,
> then the "next address data" of hardware will be in consistent with vpci.
> Is it fine? Or should I update vpci's cache?
According to the spec this field is read-only, so writes should be
ignored. We allow hardware domain full access because for hardware
domain we aim to trap as little as possible to not diverge behavior
from native, and to allow possible device quirks to work.
It could be conceivable that some vendor has a hidden specific
functionality that somehow triggered by a write to this field.
Regards, Roger.
On 2025/5/7 15:49, Roger Pau Monné wrote:
> On Wed, May 07, 2025 at 02:46:52AM +0000, Chen, Jiqian wrote:
>> On 2025/5/6 21:50, Roger Pau Monné wrote:
>>> On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote:
>>>> Current logic of emulating legacy capability list is only for domU.
>>>> So, expand it to emulate for dom0 too. Then it will be easy to hide
>>>> a capability whose initialization fails in a function.
>>>>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>>
>>> Sorry, one nit I've noticed while looking at the next patch.
>>>
>>>> @@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>>>>
>>>> next = pci_find_next_cap_ttl(pdev->sbdf,
>>>> pos + PCI_CAP_LIST_NEXT,
>>>> - supported_caps,
>>>> - ARRAY_SIZE(supported_caps), &ttl);
>>>> + caps, n, &ttl);
>>>>
>>>> - rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> The same here, NULL -> vpci_hw_write8, I think.
>
> No, not here, since the PCI_CAP_LIST_ID handler is only added for
> non-hardware domains, and in that case we do want to ignore writes to
> the register.
Oh, I write the wrong place. I mean the codes to add handler for PCI_CAPABILITY_LIST when hardware domain.
>
>>>> - pos + PCI_CAP_LIST_ID, 1, NULL);
>>>> - if ( rc )
>>>> - return rc;
>>>> + if ( !is_hwdom )
>>>> + {
>>>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>>>> + pos + PCI_CAP_LIST_ID, 1, NULL);
>>>> + if ( rc )
>>>> + return rc;
>>>> + }
>>>>
>>>> rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>>
>>> For the hardware domain the write handler should be vpci_hw_write8
>>> instead of NULL.
>> OK, I think I need to add definition of vpci_hw_write8.
>> But I have a question, if hardware domain write this register through vpci_hw_write8,
>> then the "next address data" of hardware will be in consistent with vpci.
>> Is it fine? Or should I update vpci's cache?
>
> According to the spec this field is read-only, so writes should be
> ignored. We allow hardware domain full access because for hardware
> domain we aim to trap as little as possible to not diverge behavior
> from native, and to allow possible device quirks to work.
>
> It could be conceivable that some vendor has a hidden specific
> functionality that somehow triggered by a write to this field.
Got it.
>
> Regards, Roger.
--
Best regards,
Jiqian Chen.
On 2025/5/7 10:46, Chen, Jiqian wrote:
> On 2025/5/6 21:50, Roger Pau Monné wrote:
>> On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote:
>>> Current logic of emulating legacy capability list is only for domU.
>>> So, expand it to emulate for dom0 too. Then it will be easy to hide
>>> a capability whose initialization fails in a function.
>>>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>
>> Sorry, one nit I've noticed while looking at the next patch.
>>
>>> @@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev *pdev)
>>>
>>> next = pci_find_next_cap_ttl(pdev->sbdf,
>>> pos + PCI_CAP_LIST_NEXT,
>>> - supported_caps,
>>> - ARRAY_SIZE(supported_caps), &ttl);
>>> + caps, n, &ttl);
>>>
>>> - rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
> The same here, NULL -> vpci_hw_write8, I think.
>
>>> - pos + PCI_CAP_LIST_ID, 1, NULL);
>>> - if ( rc )
>>> - return rc;
>>> + if ( !is_hwdom )
>>> + {
>>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>>> + pos + PCI_CAP_LIST_ID, 1, NULL);
>>> + if ( rc )
>>> + return rc;
>>> + }
>>>
>>> rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>
>> For the hardware domain the write handler should be vpci_hw_write8
>> instead of NULL.
> OK, I think I need to add definition of vpci_hw_write8.
> But I have a question, if hardware domain write this register through vpci_hw_write8,
> then the "next address data" of hardware will be in consistent with vpci.
be in consistent with -> be inconsistent with
I am sorry.
> Is it fine? Or should I update vpci's cache?
>
>>
>> Thanks, Roger.
>
--
Best regards,
Jiqian Chen.
On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote: > Current logic of emulating legacy capability list is only for domU. > So, expand it to emulate for dom0 too. Then it will be easy to hide > a capability whose initialization fails in a function. > > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> With the comment from Jan addressed: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 21.04.2025 08:18, Jiqian Chen wrote: > @@ -759,10 +759,11 @@ static int vpci_init_capability_list(struct pci_dev *pdev) > PCI_CAP_ID_MSI, > PCI_CAP_ID_MSIX, > }; > + const unsigned int *caps = is_hwdom ? NULL : supported_caps; > + const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(supported_caps); > > next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST, > - supported_caps, > - ARRAY_SIZE(supported_caps), &ttl); > + caps, n, &ttl); As per the v3 adjustment to patch 02, you can pass supported_caps here in all cases. Only n needs to be zero for the hwdom case. Jan
On 2025/4/23 00:01, Jan Beulich wrote: > On 21.04.2025 08:18, Jiqian Chen wrote: >> @@ -759,10 +759,11 @@ static int vpci_init_capability_list(struct pci_dev *pdev) >> PCI_CAP_ID_MSI, >> PCI_CAP_ID_MSIX, >> }; >> + const unsigned int *caps = is_hwdom ? NULL : supported_caps; >> + const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(supported_caps); >> >> next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST, >> - supported_caps, >> - ARRAY_SIZE(supported_caps), &ttl); >> + caps, n, &ttl); > > As per the v3 adjustment to patch 02, you can pass supported_caps here in > all cases. Only n needs to be zero for the hwdom case. Oh, right. I will change in next version. > > Jan -- Best regards, Jiqian Chen.
On 23.04.2025 05:31, Chen, Jiqian wrote: > On 2025/4/23 00:01, Jan Beulich wrote: >> On 21.04.2025 08:18, Jiqian Chen wrote: >>> @@ -759,10 +759,11 @@ static int vpci_init_capability_list(struct pci_dev *pdev) >>> PCI_CAP_ID_MSI, >>> PCI_CAP_ID_MSIX, >>> }; >>> + const unsigned int *caps = is_hwdom ? NULL : supported_caps; >>> + const unsigned int n = is_hwdom ? 0 : ARRAY_SIZE(supported_caps); >>> >>> next = pci_find_next_cap_ttl(pdev->sbdf, PCI_CAPABILITY_LIST, >>> - supported_caps, >>> - ARRAY_SIZE(supported_caps), &ttl); >>> + caps, n, &ttl); >> >> As per the v3 adjustment to patch 02, you can pass supported_caps here in >> all cases. Only n needs to be zero for the hwdom case. > Oh, right. I will change in next version. And, at the risk of stating the obvious, a brief comment might be a good idea here. Jan
© 2016 - 2025 Red Hat, Inc.