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

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

On 2025/3/31 16:53, Roger Pau Monné wrote:
> On Mon, Mar 31, 2025 at 08:13:50AM +0000, Chen, Jiqian wrote:
>> 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.
> 
> Seems a bit fragile, what about if there's further code added to
> init_msix() that could return an error after the vpci_add_register()
> call?  It would be safer to have a cleanup function that removes the
> config space handlers, plus the MMIO one (see the call to
> register_mmio_handler()), and the addition to the
> d->arch.hvm.msix_tables list.
I am only talking about the current implementation of init_msix(), which does 
not need a cleanup function.
But if you want such a function, even if it is not needed now, I will add it in 
the next version.

> 
>>>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>>>>  
>>>>      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?
> 
> No, we would need to fetch the size of the capability in the cleanup
> function, or figure it otherwise.  Note the same applies to MSI
> capability, which has a variable size depending on whether 64bit
> addresses and masking is supported.
Got it, I originally thought you wanted a general helper function.
But it seems the case is each capability would have its own separate cleanup 
function instead.

> 
>> I mean if I remove the registers(with range 13 to 52), is it possible that I 
>> deleted registers belonging to other abilities?
> 
> It is, indeed.  You need to know or calculate the size of the
> capability to be removed, but that's likely easier and more robust
> that keeping an array with the list of added registers?
Right.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.