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

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

On 2024/12/17 16:44, Jan Beulich wrote:
> On 17.12.2024 09:22, Chen, Jiqian wrote:
>> On 2024/12/16 19:24, Jan Beulich wrote:
>>> On 13.12.2024 06:42, Jiqian Chen wrote:
>>>> +static int cf_check init_rebar(struct pci_dev *pdev)
>>>> +{
>>>> +    uint32_t ctrl;
>>>> +    unsigned int rebar_offset, nbars;
>>>> +
>>>> +    rebar_offset = pci_find_ext_capability(pdev->sbdf, 
>>>> PCI_EXT_CAP_ID_REBAR);
>>>> +
>>>> +    if ( !rebar_offset )
>>>> +        return 0;
>>>> +
>>>> +    if ( !is_hardware_domain(pdev->domain) )
>>>> +    {
>>>> +        printk("ReBar is not supported for domUs\n");
>>>> +        return -EOPNOTSUPP;
>>>> +    }
>>>> +
>>>> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL);
>>>> +    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>>>> +
>>>> +    for ( unsigned int i = 0; i < nbars; i++, rebar_offset += 
>>>> PCI_REBAR_CTRL )
>>>
>>> PCI_REBAR_CTRL is an offset; it can't be used to bump rebar_offset here.
>>> That'll need a separate constant, even if both evaluate to 8.
>> I will add a new macro to represent the '8' in rebar.c
>> Maybe I can name it "PCI_REBAR_SINGLE_BAR_LEN" ?
> 
> Naming is a 2nd step only, I think (and no really suitable name comes to 
> mind).
> Before thinking of names, I think the approach of doing the accesses here 
> wants
> reconsidering. It isn't quite right to bump rebar_offset. When using 
> #define-s,
> I'd instead expect to first move _just_ past the capability header, and then
> use constants to get at capability and control registers. Alternatively, if we
> want to express everything relative to rebar_offset, I think we'd want
> 
> #define PCI_REBAR_CAP(n) (4 + 8 *(n))
> #define PCI_REBAR_CTRL(n) (8 + 8 *(n))
> 
> eliminating the need to alter rebar_offset (and hence disconnecting variable
> name from its purpose).
It sounds much better, thank you very much!

> 
> Jan

-- 
Best regards,
Jiqian Chen.