Re: [PATCH v3 04/11] vpci/header: Emulate extended capability list for dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxx>
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >> ---
> >> 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.