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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan