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.c...@amd.com>
>>>>
>>>> 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.
