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.
