Re: [PATCH v1 1/3] vpci: Hide capability when it fails to initialize

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/3/27 17:25, Roger Pau Monné wrote:
> On Thu, Mar 27, 2025 at 03:32:12PM +0800, Jiqian Chen wrote:
>> When vpci fails to initialize a capability of a device, it just
>> return error instead of catching and processing exception. That
>> makes the entire device unusable.
>>
>> So, refactor REGISTER_VPCI_INIT to contain more capability specific
>> information, and try to hide capability when initialization fails
>> in vpci_assign_device().
>>
>> What's more, change the definition of init_header() since it is
>> not a capability and it is needed for all devices' PCI config space.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> Hi all,
>>
>> This patch aims to hide a capability when its initialization fails.
>> That causes we can't rely on vpci_deassign_device() to clean up assigned
>> resources, so, following two patches clean up resources in the failure
>> path of init function.
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>>  xen/drivers/vpci/header.c |  3 +-
>>  xen/drivers/vpci/msi.c    |  2 +-
>>  xen/drivers/vpci/msix.c   |  2 +-
>>  xen/drivers/vpci/rebar.c  |  2 +-
>>  xen/drivers/vpci/vpci.c   | 65 +++++++++++++++++++++++++++++++++------
>>  xen/include/xen/vpci.h    | 27 ++++++++++++----
>>  6 files changed, 81 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index ef6c965c081c..8c8e4ac5698a 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -745,7 +745,7 @@ static int bar_add_rangeset(const struct pci_dev *pdev, 
>> struct vpci_bar *bar,
>>      return !bar->mem ? -ENOMEM : 0;
>>  }
>>  
>> -static int cf_check init_header(struct pci_dev *pdev)
>> +int vpci_init_header(struct pci_dev *pdev)
>>  {
>>      uint16_t cmd;
>>      uint64_t addr, size;
>> @@ -1007,7 +1007,6 @@ static int cf_check init_header(struct pci_dev *pdev)
>>      pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
>>      return rc;
>>  }
>> -REGISTER_VPCI_INIT(init_header, VPCI_PRIORITY_MIDDLE);
>>  
>>  /*
>>   * Local variables:
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index 66e5a8a116be..9d7a9fd8dba1 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -270,7 +270,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
>>  
>>      return 0;
>>  }
>> -REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW);
>> +REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSI, init_msi, VPCI_PRIORITY_LOW);
>>  
>>  void vpci_dump_msi(void)
>>  {
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 6bd8c55bb48e..50e5f38c1e09 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -753,7 +753,7 @@ static int cf_check init_msix(struct pci_dev *pdev)
>>  
>>      return 0;
>>  }
>> -REGISTER_VPCI_INIT(init_msix, VPCI_PRIORITY_HIGH);
>> +REGISTER_VPCI_LEGACY_CAP(PCI_CAP_ID_MSIX, init_msix, VPCI_PRIORITY_HIGH);
>>  
>>  /*
>>   * Local variables:
>> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
>> index 793937449af7..7c53ee031887 100644
>> --- a/xen/drivers/vpci/rebar.c
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -118,7 +118,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
>>  
>>      return 0;
>>  }
>> -REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
>> +REGISTER_VPCI_EXTEND_CAP(PCI_EXT_CAP_ID_REBAR, init_rebar, 
>> VPCI_PRIORITY_LOW);
>>  
>>  /*
>>   * Local variables:
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 1e6aa5d799b9..a8362e46e097 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -36,8 +36,8 @@ struct vpci_register {
>>  };
>>  
>>  #ifdef __XEN__
>> -extern vpci_register_init_t *const __start_vpci_array[];
>> -extern vpci_register_init_t *const __end_vpci_array[];
>> +extern vpci_capability_t *const __start_vpci_array[];
>> +extern vpci_capability_t *const __end_vpci_array[];
>>  #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>  
>>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> @@ -83,6 +83,47 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
>>  
>>  #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>>  
>> +static int vpci_init_cap_with_priority(struct pci_dev *pdev,
>> +                                       const char *priority)
>> +{
>> +    for ( unsigned int i = 0; i < NUM_VPCI_INIT; i++ )
>> +    {
>> +        const vpci_capability_t *capability = __start_vpci_array[i];
>> +        const unsigned int cap_id = capability->id;
>> +        unsigned int pos;
>> +        int rc;
>> +
>> +        if ( *(capability->priority) != *priority )
>> +            continue;
>> +
>> +        if ( !capability->is_ext )
>> +            pos = pci_find_cap_offset(pdev->sbdf, cap_id);
>> +        else
>> +            pos = pci_find_ext_capability(pdev->sbdf, cap_id);
>> +
>> +        if ( !pos )
>> +            continue;
>> +
>> +        rc = capability->init(pdev);
>> +
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_WARNING "%pd %pp: cap init fail rc=%d, try to 
>> hide\n",
>> +                   pdev->domain, &pdev->sbdf, rc);
>> +            rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>> +                                   pos, capability->is_ext ? 4 : 1, NULL);
> 
> Are you sure this works as intended? 
Yes, I used failure test cases of init_msi/rebar.
From the "lspci" result, they were hided from the dom0.
But I forgot to test for domUs.

> The capability ID 0 is marked as "reserved" in the spec, so it's unclear to 
> me how OSes would handle
> finding such capability on the list - I won't be surprised if some
> implementations decide to terminate the walk.  It's fine to mask the
> capability ID for the ones that we don't want to expose, but there's
> further work to do IMO.
> 
> The usual way to deal with masking capabilities is to short circuit
> the capability from the linked list, by making the previous capability
> "Next Capability Offset" point to the next capability in the list,
> thus skipping the current one. So:
> 
> capability[n - 1].next_cap = capability[n].next_cap
> 
> IOW: you will need to add the handler to the previous capability on
> the list.  That's how it's already done in init_header().
Oh, I got your opinion.
But we may need to discuss this more.
In my opinion, there should be two situations:
First, if device belongs to hardware domain, there is no emulation of legacy or 
extended capabilities linked list if I understand codes right.
So, for this situation, I think current implementation of my patch is enough 
for hiding legacy or extended capabilities.

Second, if device belongs to common domain, we just need to consider legacy 
capabilities since all extended capabilities are hided in init_header().
So, for this situation, I need to what you said " capability[n - 1].next_cap = 
capability[n].next_cap "

I am not sure if above are right.
> 
>> +            if ( rc )
>> +            {
>> +                printk(XENLOG_ERR "%pd %pp: fail to hide cap rc=%d\n",
>> +                       pdev->domain, &pdev->sbdf, rc);
>> +                return rc;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  void vpci_deassign_device(struct pci_dev *pdev)
>>  {
>>      unsigned int i;
>> @@ -128,7 +169,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
>>  
>>  int vpci_assign_device(struct pci_dev *pdev)
>>  {
>> -    unsigned int i;
>>      const unsigned long *ro_map;
>>      int rc = 0;
>>  
>> @@ -159,12 +199,19 @@ int vpci_assign_device(struct pci_dev *pdev)
>>          goto out;
>>  #endif
>>  
>> -    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>> -    {
>> -        rc = __start_vpci_array[i](pdev);
>> -        if ( rc )
>> -            break;
>> -    }
>> +    /*
>> +     * Capabilities with high priority like MSI-X need to
>> +     * be initialized before header
>> +     */
>> +    rc = vpci_init_cap_with_priority(pdev, VPCI_PRIORITY_HIGH);
>> +    if ( rc )
>> +        goto out;
> 
> I understand this is not introduced by this change, but I wonder if
> there could be a way to ditch the priority stuff for capabilities,
> specially now that we only have two "priorities": before or after PCI
> header initialization.
I have an idea, but it seems like a hake.
Can we add a flag(maybe name it "msix_initialized") to struct vpci{}?
Then in vpci_make_msix_hole(), we can first check that flag, if it is false, we 
return an error to let modify_decoding() directly return in the process of 
init_header.
And in the start of init_msix(), to set msix_initialized=true, in the end of 
init_msix(), to call modify_decoding() to setup p2m.
Then we can remove the priorities.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.