Re: [PATCH] vpci: Add resizable bar support

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +
>>> +    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.

>>> +    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.

Jan