Re: [PATCH] vpci: Add resizable bar support

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

On Wed, Nov 20, 2024 at 02:26:31AM +0000, Chen, Jiqian wrote:
> On 2024/11/19 15:44, Jan Beulich wrote:
> > On 19.11.2024 08:31, Chen, Jiqian wrote:
> >> On 2024/11/18 18:17, Roger Pau Monné wrote:
> >>> On Wed, Nov 13, 2024 at 04:00:27PM +0800, Jiqian Chen wrote:
> >>>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> >>>> +                                      unsigned int reg,
> >>>> +                                      uint32_t val,
> >>>> +                                      void *data)
> >>>> +{
> >>>> +    uint32_t ctrl, index;
> >>>
> >>> index should better be unsigned int, as it's the BAR index [0, 5], and
> >>> so fits perfectly in an unsigned int.
> >>>
> >>>> +    struct vpci_bar *bars = pdev->vpci->header.bars;
> >>>
> >>> You could pass bars as the data parameter.
> >>>
> >>> Additionally you need to check that memory decoding is not enabled for
> >>> the device, otherwise attempting to change the BAR size will lead to
> >>> the active p2m mappings getting out of sync w.r.t. the new BAR size.
> >>>
> >>>> +
> >>>> +    ctrl = pci_conf_read32(pdev->sbdf, reg);
> >>>> +    if ( ctrl == val )
> >>>> +        return;
> >>>
> >>> I would still carry out the write in this case, as that's what the
> >>> owner of the device requested.
> >>>
> >>>> +
> >>>> +    ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> >>>> +    if ( ctrl != ( val & ~PCI_REBAR_CTRL_BAR_SIZE ) )
> >>>                      ^ extra space here and         ^ here
> >>>> +        return;
> >>>
> >>> The feature only being exposed to dom0 ATM, I would suggest we allow
> >>> it to write any bits it wants in the control register, as that would
> >>> be what the OS would do when not running as a guest.
> >> But only PCI_REBAR_CTRL_BAR_SIZE bits of REBAR_CTRL register are RW, 
> >> others are RO.
> >> Is removing the check here fine?
> > 
> > A native kernel would write the full register (with r/o bits simply not
> > getting updated). Hence for Dom0 we ought to do the same, just in case
> > e.g. some future spec declares some other bit(s) writable.
> Got it, thanks for explaining.
> 
> > 
> >>>> +
> >>>> +    index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> >>>
> >>> Some sanity checking of the BAR index might be good.  At least a check
> >>> that the BAR is of type VPCI_BAR_MEM64_LO or VPCI_BAR_MEM32.
> >> But the information of the BAR that support resizing is from hardware(when 
> >> init_rebar), that shouldn't have any problems and doesn't need to check 
> >> the BAR's info.
> > 
> > Right, but also better to avoid confusing ourselves over bogus hardware.
> OK, will add some check for the index range and the bar's type.
> 
> > 
> >>>> +    bars[index].size = (1 << ((val & PCI_REBAR_CTRL_BAR_SIZE) >>
> >>>> +                              PCI_REBAR_CTRL_BAR_SHIFT)) *
> >>>> +                       PCI_REBAR_CTRL_BAR_UNIT;
> >>>
> >>> This would better be a macro in pci_regs.h I think, and you need to
> >>> use 1UL, plus using MASK_EXTR() simplifies it:
> >>>
> >>> PCI_REBAR_CTRL_SIZE(v) (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) + 
> >>> 20))
> >> OK, another question: Should I need to check the size is allowed by 
> >> hardware(the PCI_REBAR_CAP_SIZES bits in PCI_REBAR_CAP)?
> > 
> > Probably better to do so, yes. Whether to reject bogus attempts or
> > merely warn about them I'm less certain: It's (see above) Dom0, after
> > all.
> I would like to if the new size is allowed by hardware, then update the size, 
> otherwise do nothing.

I'm of the opinion that dom0 shouldn't be restricted like this: dom0
might have more information than Xen about the device.  Xen might
detect the size as invalid, but dom0 might have extra information (or
quirks) about this specific device that make the size valid.

I would just add a warning.

Thanks, Roger.