Re: [PATCH v2 1/1] vpci: Add resizable bar support

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

On 2024/12/11 16:40, Roger Pau Monné wrote:
> On Wed, Dec 11, 2024 at 07:57:29AM +0000, Chen, Jiqian wrote:
>> On 2024/12/10 19:25, Roger Pau Monné wrote:
>>> On Tue, Dec 10, 2024 at 10:54:43AM +0100, Jan Beulich wrote:
>>>> On 10.12.2024 08:57, Chen, Jiqian wrote:
>>>>> On 2024/12/10 15:17, Jan Beulich wrote:
>>>>>> On 10.12.2024 08:07, Chen, Jiqian wrote:
>>>>>>> On 2024/12/9 21:59, Jan Beulich wrote:
>>>>>>>> On 02.12.2024 07:09, Jiqian Chen wrote:
>>>>>>>>> +        if ( rc )
>>>>>>>>> +        {
>>>>>>>>> +            printk("%pp: add register for PCI_REBAR_CAP failed 
>>>>>>>>> (rc=%d)\n",
>>>>>>>>> +                   &pdev->sbdf, rc);
>>>>>>>>> +            break;
>>>>>>>>> +        }
>>>>>>>>> +
>>>>>>>>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, 
>>>>>>>>> rebar_ctrl_write,
>>>>>>>>> +                               rebar_offset + PCI_REBAR_CTRL, 4,
>>>>>>>>> +                               pdev->vpci->header.bars);
>>>>>>>>> +        if ( rc )
>>>>>>>>> +        {
>>>>>>>>> +            printk("%pp: add register for PCI_REBAR_CTRL failed 
>>>>>>>>> %d\n",
>>>>>>>>> +                   &pdev->sbdf, rc);
>>>>>>>>> +            break;
>>>>>>>>
>>>>>>>> Is it correct to keep the other handler installed? After all ...
>>>>>>> Will change to "return rc;" here and above in next version.
>>>>>>
>>>>>> I'm not convinced this is what we want, as per ...
>>>>>>
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>
>>>>>>>> ... you - imo sensibly - aren't communicating the error back up (to 
>>>>>>>> allow
>>>>>>>> the device to be used without BAR resizing.
>>>>>>
>>>>>> ... what I said here.
>>>>> Sorry, I didn’t understand.
>>>>> Do you mean it is not enough to return error code once a handler failed 
>>>>> to be installed, I need to remove the already installed handlers?
>>>>
>>>> No, if you return an error here, nothing else needs doing. However, I
>>>> question that returning an error here is good or even necessary. In
>>>> the event of an error, the device ought to still be usable, just
>>>> without the BAR-resizing capability.
>>>
>>> So you suggest that the capability should be hidden in that case?  We
>>> have logic to hide capabilities, just not used for the hardware
>>> domain.  It would need some extra wiring to be capable of hiding
>>> failed capabilities.
>> Can you give me a guidance on how to hide a failed capability?
>> What codes are current logic to hide capabilities?
> 
> This was done by Stewart for the legacy PCI capabilities, but not
> exactly to hide the capabilities that fail to init.  Take a look at
> commit:
> 
> d830b0a7bc7e xen/vpci: header: filter PCI capabilities
> 
> However that was designed to expose a fixed set of capabilities,
> always known when init_header() is executed.  If we want to hide
> capabilities on failure we will need a bit more flexible interface I
> think.
> 
> Ideally we would like to tie this to initialization hooks themselves,
> so that in vpci_assign_device() an init function failing automatically
> triggers the hiding of the failing capability.  That would need an
> interface similar to:
> 
> #define REGISTER_VPCI_INIT(<capability id>, <function>, <priority>,
> <pcie?>)
> 
> REGISTER_VPCI_INIT(PCI_CAP_ID_MSI, init_msi, VPCI_PRIORITY_LOW,
> false);
> 
> And then in vpci_assign_device() any init function that has a
> capability ID different than 0 and fails to initialize would lead to
> the capability being masked.
> 
> It would be great to have an interface like this in place, because the
> current error handling in vPCI is not great.  For the hardware domain
> init functions failing will just lead to the device being fully
> accessible by dom0 without any mediation.
> 
> Anyway, we don't do any of this for dom0 at the moment when MSI or
> MSI-X fail to init, so I'm not sure it's fair to ask that you do this
> for ReBAR.  It would be great if you want to, but it's not a trivial
> amount of work.
Thanks!
It sounds like not easy to do.
But I will try to implement this if I have time.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.