Re: [PATCH v3 03/11] vpci/header: Emulate legacy capability list for dom0

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

On 2025/5/7 15:49, Roger Pau Monné wrote:
> On Wed, May 07, 2025 at 02:46:52AM +0000, Chen, Jiqian wrote:
>> On 2025/5/6 21:50, Roger Pau Monné wrote:
>>> On Mon, Apr 21, 2025 at 02:18:55PM +0800, Jiqian Chen wrote:
>>>> Current logic of emulating legacy capability list is only for domU.
>>>> So, expand it to emulate for dom0 too. Then it will be easy to hide
>>>> a capability whose initialization fails in a function.
>>>>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>>>
>>> Sorry, one nit I've noticed while looking at the next patch.
>>>
>>>> @@ -786,13 +787,15 @@ static int vpci_init_capability_list(struct pci_dev 
>>>> *pdev)
>>>>  
>>>>              next = pci_find_next_cap_ttl(pdev->sbdf,
>>>>                                           pos + PCI_CAP_LIST_NEXT,
>>>> -                                         supported_caps,
>>>> -                                         ARRAY_SIZE(supported_caps), 
>>>> &ttl);
>>>> +                                         caps, n, &ttl);
>>>>  
>>>> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>> The same here, NULL -> vpci_hw_write8, I think.
> 
> No, not here, since the PCI_CAP_LIST_ID handler is only added for
> non-hardware domains, and in that case we do want to ignore writes to
> the register.
Oh, I write the wrong place. I mean the codes to add handler for 
PCI_CAPABILITY_LIST when hardware domain.

> 
>>>> -                                   pos + PCI_CAP_LIST_ID, 1, NULL);
>>>> -            if ( rc )
>>>> -                return rc;
>>>> +            if ( !is_hwdom )
>>>> +            {
>>>> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read8, NULL,
>>>> +                                       pos + PCI_CAP_LIST_ID, 1, NULL);
>>>> +                if ( rc )
>>>> +                    return rc;
>>>> +            }
>>>>  
>>>>              rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>>>
>>> For the hardware domain the write handler should be vpci_hw_write8
>>> instead of NULL.
>> OK, I think I need to add definition of vpci_hw_write8.
>> But I have a question, if hardware domain write this register through 
>> vpci_hw_write8,
>> then the "next address data" of hardware will be in consistent with vpci.
>> Is it fine? Or should I update vpci's cache?
> 
> According to the spec this field is read-only, so writes should be
> ignored.  We allow hardware domain full access because for hardware
> domain we aim to trap as little as possible to not diverge behavior
> from native, and to allow possible device quirks to work.
> 
> It could be conceivable that some vendor has a hidden specific
> functionality that somehow triggered by a write to this field.
Got it.

> 
> Regards, Roger.

-- 
Best regards,
Jiqian Chen.