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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Dec 11, 2024 at 09:36:00AM +0000, Chen, Jiqian wrote:
> On 2024/12/11 16:16, Roger Pau Monné wrote:
> > On Wed, Dec 11, 2024 at 06:37:30AM +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:
> >>>>>>>>> +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;
> >>> }
> >> Thank you very much!
> >>
> >>>
> >>> Note this requires that the data parameter points to the BAR that
> >>> matches the ReBAR control register, this needs adjusting in
> >>> init_rebar().
> >> I think I can keep current implementation of init_rebar() and use bars[index] to get the corresponding BAR.
> > 
> > IMO it would be best if you can pass the corresponding bar struct into
> > the handler directly, as that will avoid having to do a PCI read just
> > to get the BAR index from PCI_REBAR_CTRL.  It should also avoid the
> > need for the index and BAR type checks in rebar_ctrl_write().
> OK, if so, then I need to move the logic of getting index from PCI_REBAR_CTRL register and checking the BAR type into init_rebar(), right?

Yes, I think that would be better, as then the check is done only once
at init rather than on every access.

Thanks, Roger.