Re: [PATCH] vpci: Add resizable bar support

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

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.

> 
> Jan

-- 
Best regards,
Jiqian Chen.