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.
