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

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

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:
> >>>>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> >>>>> +                                      unsigned int reg,
> >>>>> +                                      uint32_t val,
> >>>>> +                                      void *data)
> >>>>> +{
> >>>>> +    uint64_t size;
> >>>>> +    unsigned int index;
> >>>>> +    struct vpci_bar *bars = data;
> >>>>> +
> >>>>> +    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY 
> >>>>> )
> >>>>> +        return;
> >>>>
> >>>> I don't think something like this can go uncommented. I don't think the
> >>>> spec mandates to drop writes in this situation?
> >>> Spec says: Software must clear the Memory Space Enable bit in the Command 
> >>> register before writing the BAR Size field.
> >>> This check is suggested by Roger and it really helps to prevent erroneous 
> >>> writes in this case,
> >>> such as the result of debugging with Roger in the previous version.
> >>> I will add the spec's sentences as comments here in next version.
> >>
> >> What you quote from the spec may not be enough as a comment here. There's
> >> no direct implication that the write would simply be dropped on the floor
> >> if the bit is still set. So I think you want to go a little beyond just
> >> quoting from the spec.
> > How about quoting Roger's previous words: " The memory decoding must be 
> > disabled before writing the BAR size field.
> > Otherwise changing the BAR size will lead to the active p2m mappings 
> > getting out of sync w.r.t. the new BAR size." ?
> 
> That'll be better, but imo still not enough to explain the outright ignoring
> of the write.

I think we might want to do something along the lines of:

uint64_t size = PCI_REBAR_CTRL_SIZE(val);
struct vpci_bar *bar = data;

if ( bar->enabled )
{
    if ( size == bar->size )
        return;

    /*
     * Refuse to resize a BAR while memory decoding is enabled, as
     * otherwise the size of the mapped region in the p2m would become
     * stale with the newly set BAR size, and the position of the BAR
     * would be reset to undefined.  Note the PCIe specification also
     * forbids resizing a BAR with memory decoding enabled.
     */
    gprintk(XENLOG_ERR,
            "%pp: refuse to resize BAR with memory decoding enabled\n",
            &pci->sbdf);
    return;
}

Note this requires that the data parameter points to the BAR that
matches the ReBAR control register, this needs adjusting in
init_rebar().

> >>>>> +        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.

Regards, Roger.