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

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

On Tue, Dec 10, 2024 at 01:15:00PM +0100, Jan Beulich wrote:
> On 10.12.2024 12: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:
> >>>>>>> +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().
> 
> SGTM.

One nit that I'm kind of unsure, is whether writing the ReBAR with the
same BAR size as it's currently set also causes the matching BAR
register to be reset.  I'm unsure whether dropping the write when size
== bar->size without logging a message is appropriate, as it could be
a change in behavior if the software expects the BAR position to be
reset.

Thanks, Roger.