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

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

On 2025/3/31 19:04, Roger Pau Monné wrote:
> On Mon, Mar 31, 2025 at 09:32:02AM +0000, Chen, Jiqian wrote:
>> So, I need to refactor the emulating PCI capability list codes of 
>> init_header() to serve
>> for all domain(dom0+domUs) firstly, since current codes only emulate PCI 
>> capability list for domUs, right?
> 
> Yes, that would be my understanding.  The current logic in
> init_header() needs to be expanded/generalized so it can be used for
> masking random PCI capabilities, plus adapted to work with PCIe
> capabilities also.
OK, I will try to refactor the logic in next version.
Hoping the next version will be more in line with your ideas.
Thanks!
> 
>>>
>>>>
>>>>>
>>>>>> +    /*
>>>>>> +     * 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.
>>>
>>> Maybe the initialization of the MSI-X capability could be done after
>>> the header, and call vpci_make_msix_hole()?  There's a bit of
>>> redundancy here in that the BAR is first fully mapped, and then a hole
>>> is punched in place of the MSI-X related tables.  Seems like the
>>> easier option to break the depedency of init_msix() in being called
>>> ahead of init_header().
>> You mean the sequence should be:
>> vpci_init_header()
>> vpci_init_capability() // all capabilities
>> vpci_make_msix_hole()
>>
>> Right?
> 
> Yes, I think that would be my preference.  The call to
> vpci_make_msix_hole() should be inside of init_msix().
Got it, will do in next version.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.