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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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, Roger.