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.
