Re: [PATCH v4] vpci: Add resizable bar support

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

On Wed, Jan 08, 2025 at 08:19:55AM +0100, Jan Beulich wrote:
> On 07.01.2025 19:19, Roger Pau Monné wrote:
> > On Tue, Jan 07, 2025 at 04:58:07PM +0100, Jan Beulich wrote:
> >> On 07.01.2025 15:38, Roger Pau Monné wrote:
> >>> On Tue, Jan 07, 2025 at 11:06:33AM +0100, Jan Beulich wrote:
> >>>> On 19.12.2024 06:21, Jiqian Chen wrote:
> >>>>> --- /dev/null
> >>>>> +++ b/xen/drivers/vpci/rebar.c
> >>>>> @@ -0,0 +1,131 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
> >>>>> +/*
> >>>>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> >>>>> + *
> >>>>> + * Author: Jiqian Chen <Jiqian.Chen@amd.com>
> >>>>> + */
> >>>>> +
> >>>>> +#include <xen/sched.h>
> >>>>> +#include <xen/vpci.h>
> >>>>> +
> >>>>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> >>>>> +                                      unsigned int reg,
> >>>>> +                                      uint32_t val,
> >>>>> +                                      void *data)
> >>>>> +{
> >>>>> +    struct vpci_bar *bar = data;
> >>>>> +    uint64_t size = PCI_REBAR_CTRL_SIZE(val);
> >>>>> +
> >>>>> +    if ( bar->enabled )
> >>>>> +    {
> >>>>> +        /*
> >>>>> +         * 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.
> >>>>> +         */
> >>>>> +        if ( size != bar->size )
> >>>>> +            gprintk(XENLOG_ERR,
> >>>>> +                    "%pp: refuse to resize BAR with memory decoding enabled\n",
> >>>>> +                    &pdev->sbdf);
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>>> +    if ( !((size >> PCI_REBAR_SIZE_BIAS) & bar->resizable_sizes) )
> >>>>> +        gprintk(XENLOG_WARNING,
> >>>>> +                "%pp: new size %#lx is not supported by hardware\n",
> >>>>> +                &pdev->sbdf, size);
> >>>>> +
> >>>>> +    bar->size = size;
> >>>>
> >>>> Shouldn't at least this be in an "else" to the if() above?
> >>>
> >>> I think this was already raised in a previous version - would be good
> >>> to know how real hardware behaves when an invalid size is set.  Is the
> >>> BAR register still reset?
> >>
> >> I'm pretty sure what happens is undefined. I'd expect though that the
> >> BAR size then doesn't change. Which would require the above assignment
> >> to not be unconditional.
> > 
> > Might be better to just re-size the BAR, like you suggested to fetch
> > the BAR position from the register, instead of assuming 0.
> 
> FTAOD by "re-size" you mean re-obtain its size (seeing we're talking of
> re-sizable BARs here)? As kind of confirmed ...

Indeed, I meant to re-obtain the size (I can see that being
confusing in this context, sorry).

> >>>>> --- a/xen/drivers/vpci/vpci.c
> >>>>> +++ b/xen/drivers/vpci/vpci.c
> >>>>> @@ -232,6 +232,12 @@ void cf_check vpci_hw_write16(
> >>>>>      pci_conf_write16(pdev->sbdf, reg, val);
> >>>>>  }
> >>>>>  
> >>>>> +void cf_check vpci_hw_write32(
> >>>>> +    const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
> >>>>> +{
> >>>>> +    pci_conf_write32(pdev->sbdf, reg, val);
> >>>>> +}
> >>>>
> >>>> This function is being added just to handle writing of a r/o register.
> >>>> Can't you better re-use vpci_ignored_write()?
> >>>
> >>> But vpci_ignored_write() ignores the write, OTOH here the write is
> >>> propagated to the hardware.
> >>
> >> Right, just for the hardware to drop it. I wouldn't have commented if
> >> the function needed to do things like this already existed. Adding yet
> >> another cf_check function just for this is what made me give the remark.
> > 
> > According to the spec yes, they will be ignored.  Yet for the hardware
> > domain we try to avoid changing behavior from native as much as
> > possible, hence propagating the write seems more appropriate.
> 
> Okay; you're the maintainer of this code anyway.

Thanks for all your input Jan, you might not be the maintainer but
have certainly reviewed all vPCI code.

Roger.