Re: [PATCH] vpci: Add resizable bar support

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

On Mon, Nov 25, 2024 at 03:44:52AM +0000, Chen, Jiqian wrote:
> On 2024/11/21 17:52, Roger Pau Monné wrote:
> > On Thu, Nov 21, 2024 at 03:05:14AM +0000, Chen, Jiqian wrote:
> >> On 2024/11/20 17:01, Roger Pau Monné wrote:
> >>> On Wed, Nov 20, 2024 at 03:01:57AM +0000, Chen, Jiqian wrote:
> >>>> The only difference between our methods is the timing of updating the 
> >>>> size.
> >>>> Yours is later than mine because you updated the size when the driver 
> >>>> re-enabled memory decoding, while I updated the size in time when driver 
> >>>> resize it.
> >>>
> >>> Indeed, my last guess is the stale cached size is somehow used in my
> >>> approach, and that leads to the failures.  One last (possibly dummy?)
> >>> thing to try might be to use your patch to detect writes to the resize
> >>> control register, but update the BAR sizes in modify_bars(), while
> >>> keeping the traces of when the operations happen.
> >>>
> >> This can work, combine our method, use my patch to detect and write the 
> >> size into hardware register, and use your patch to update bar[i].size in 
> >> modify_bars().
> >> Attached the combined patch and the xl dmesg.
> > 
> > This is even weirder, so the attached patch works fine?  The only
> > difference with my proposal is that you trap the CTRL registers, but
> > the sizing is still done in modify_bars().
> > 
> > What happens if (based on the attached patch) you change
> > rebar_ctrl_write() to:
> > 
> > static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> >                                       unsigned int reg,
> >                                       uint32_t val,
> >                                       void *data)
> > {
> >     pci_conf_write32(pdev->sbdf, reg, val);
> > }
> > 
> If I change rebar_ctrl_write() to:
> static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>                                       unsigned int reg,
>                                       uint32_t val,
>                                       void *data)
> {
>     printk("cjq_debug %pp: bar ctrl write reg %u, val %x\n", &pdev->sbdf, 
> reg, val);
>     pci_conf_write32(pdev->sbdf, reg, val);
> }
> 
> I can see three time prints, it can't work.
> (XEN) cjq_debug 0000:03:00.0: bar ctrl write reg 520, val d40
> (XEN) cjq_debug 0000:03:00.0: bar ctrl write reg 520, val d40
> (XEN) cjq_debug 0000:03:00.0: bar ctrl write reg 528, val 102
> 
> If I change rebar_ctrl_write() to:
> static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>                                       unsigned int reg,
>                                       uint32_t val,
>                                       void *data)
> {
>     if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>         return;
>     printk("cjq_debug %pp: bar ctrl write reg %u, val %x\n", &pdev->sbdf, 
> reg, val);
>     pci_conf_write32(pdev->sbdf, reg, val);
> } 
> 
> I can only see one time print:
> (XEN) cjq_debug 0000:03:00.0: bar ctrl write reg 520, val d40
> 
> The check prevented the two times incorrect write actions.
>     if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>         return;
> 
> And why my original patch can work too, the check:
> +    ctrl = pci_conf_read32(pdev->sbdf, reg);
> +    if ( ctrl == val )
> +        return;
> happened to play the same role as PCI_COMMAND_MEMORY check.

Thank you very much for figuring this out.  So in the end it's a bug
in the driver that plays with PCI_REBAR_CTRL with memory decoding
enabled.

Won't this also cause issues when running natively without Xen?

I think we have no other option but to trap accesses to the capability
registers themselves in order to ensure a minimum amount of sanity
(iow: no writes to the ReBAR control registers decoding is enabled).

Thanks, Roger.