Re: [PATCH v1 3/3] vpci/msi: Remove registers when init_msi() fails

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

On 2025/3/27 20:44, Roger Pau Monné wrote:
> On Thu, Mar 27, 2025 at 03:32:14PM +0800, Jiqian Chen wrote:
>> When init_msi() fails, the new codes will try to hide MSI
>> capability, so it can't rely on vpci_deassign_device() to
>> remove all MSI related registers anymore, those registers
>> must be cleaned up in failure path of init_msi.
>>
>> To do that, use a vpci_register array to record all MSI
>> registers and call vpci_remove_register() to remove registers.
> 
> As I'm just seeing 3 patches on the series, isn't there one missing
> for MSI-X at least?
No, because init_msix only call vpci_add_register once, there is no need to 
remove registers when it fails.

> 
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>>  xen/drivers/vpci/msi.c | 57 +++++++++++++++++++++++++++++-------------
>>  1 file changed, 39 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index 9d7a9fd8dba1..30ef97efb7b0 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -195,6 +195,9 @@ static void cf_check mask_write(
>>  
>>  static int cf_check init_msi(struct pci_dev *pdev)
>>  {
>> +    unsigned int offset;
>> +    unsigned int reg_index = 0;
>> +    struct vpci_register registers[VPCI_CAP_MAX_REGISTER];
>>      unsigned int pos = pdev->msi_pos;
>>      uint16_t control;
>>      int ret;
>> @@ -206,15 +209,13 @@ static int cf_check init_msi(struct pci_dev *pdev)
>>      if ( !pdev->vpci->msi )
>>          return -ENOMEM;
>>  
>> +    offset = msi_control_reg(pos);
>>      ret = vpci_add_register(pdev->vpci, control_read, control_write,
>> -                            msi_control_reg(pos), 2, pdev->vpci->msi);
>> +                            offset, 2, pdev->vpci->msi);
>>      if ( ret )
>> -        /*
>> -         * NB: there's no need to free the msi struct or remove the register
>> -         * handlers form the config space, the caller will take care of the
>> -         * cleanup.
>> -         */
>> -        return ret;
>> +        goto fail;
>> +    registers[reg_index].offset = offset;
>> +    registers[reg_index++].size = 2;
>>  
>>      /* Get the maximum number of vectors the device supports. */
>>      control = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> @@ -234,33 +235,42 @@ static int cf_check init_msi(struct pci_dev *pdev)
>>      pdev->vpci->msi->address64 = is_64bit_address(control);
>>      pdev->vpci->msi->masking = is_mask_bit_support(control);
>>  
>> +    offset = msi_lower_address_reg(pos);
>>      ret = vpci_add_register(pdev->vpci, address_read, address_write,
>> -                            msi_lower_address_reg(pos), 4, pdev->vpci->msi);
>> +                            offset, 4, pdev->vpci->msi);
>>      if ( ret )
>> -        return ret;
>> +        goto fail;
>> +    registers[reg_index].offset = offset;
>> +    registers[reg_index++].size = 4;
>>  
>> +    offset = msi_data_reg(pos, pdev->vpci->msi->address64);
>>      ret = vpci_add_register(pdev->vpci, data_read, data_write,
>> -                            msi_data_reg(pos, pdev->vpci->msi->address64), 
>> 2,
>> -                            pdev->vpci->msi);
>> +                            offset, 2, pdev->vpci->msi);
>>      if ( ret )
>> -        return ret;
>> +        goto fail;
>> +    registers[reg_index].offset = offset;
>> +    registers[reg_index++].size = 2;
>>  
>>      if ( pdev->vpci->msi->address64 )
>>      {
>> +        offset = msi_upper_address_reg(pos);
>>          ret = vpci_add_register(pdev->vpci, address_hi_read, 
>> address_hi_write,
>> -                                msi_upper_address_reg(pos), 4, 
>> pdev->vpci->msi);
>> +                                offset, 4, pdev->vpci->msi);
>>          if ( ret )
>> -            return ret;
>> +            goto fail;
>> +        registers[reg_index].offset = offset;
>> +        registers[reg_index++].size = 4;
>>      }
>>  
>>      if ( pdev->vpci->msi->masking )
>>      {
>> +        offset = msi_mask_bits_reg(pos, pdev->vpci->msi->address64);
>>          ret = vpci_add_register(pdev->vpci, mask_read, mask_write,
>> -                                msi_mask_bits_reg(pos,
>> -                                                  
>> pdev->vpci->msi->address64),
>> -                                4, pdev->vpci->msi);
>> +                                offset, 4, pdev->vpci->msi);
>>          if ( ret )
>> -            return ret;
>> +            goto fail;
>> +        registers[reg_index].offset = offset;
>> +        registers[reg_index++].size = 4;
> 
> As commented on the previous patch, I don't like much the usage of
> this registers array to store which handlers have been added.  It
> would be best if you just removed every possible handler that could be
> added, without keeping track of them.
Make sense, it will indeed be simpler if it is fine to remove all possible 
registers.

> 
> Thinking about it, do we maybe need a helper vcpi function that wipes
> all handlers from a specific range?  I think it could be helpful here,
> as the size of the capabilities is well-known, so on error it would be
> easier to just call such a generic handler to wipe all hooks added to
> the region covered by the failing capability.
But I am not sure if that helper function is suitable for all capabilities.
Like Rebar, its structure can range from 12 bytes long(for a single BAR) to 52 
bytes long(for all six BARs).
If a device supports Rebar and only has a single resizable BAR, does hardware 
still reserved the range from 13 bytes to 52 bytes for that device?
I mean if I remove the registers(with range 13 to 52), is it possible that I 
deleted registers belonging to other abilities?

> 
> Let me know what you think of it.
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.