Re: [PATCH v2 1/1] vpci: Add resizable bar support

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

On 2024/12/10 17:54, Jan Beulich wrote:
> On 10.12.2024 08:57, Chen, Jiqian wrote:
>> On 2024/12/10 15:17, Jan Beulich wrote:
>>> On 10.12.2024 08:07, Chen, Jiqian wrote:
>>>> On 2024/12/9 21:59, Jan Beulich wrote:
>>>>> On 02.12.2024 07:09, Jiqian Chen wrote: 
>>>>>> @@ -541,6 +542,16 @@
>>>>>>  #define  PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf)
>>>>>>  #define  PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff)
>>>>>>  
>>>>>> +/* Resizable BARs */
>>>>>> +#define PCI_REBAR_CAP           4       /* capability register */
>>>>>> +#define  PCI_REBAR_CAP_SIZES            0xFFFFFFF0  /* supported BAR 
>>>>>> sizes */
>>>>>
>>>>> Misra demands that this have a U suffix.
>>>> Do below PCI_REBAR_CTRL_BAR_IDX, PCI_REBAR_CTRL_NBAR_MASK and 
>>>> PCI_REBAR_CTRL_BAR_SIZE also need a U suffix?
>>>
>>> They may want to gain them for consistency, but they don't strictly need
>>> them. I wanted to say "See the rest of the file", but it looks like the
>>> file wasn't cleaned up yet Misra-wise.
>> Yes, I noticed that the rest of the file didn't add U suffix too.
>> So, I just need to add U suffixes for my new macros?
> 
> You only strictly need to add U to values with the top bit set.
Got it, thanks!

> 
>>>>>> +#define PCI_REBAR_CTRL          8       /* control register */
>>>>>> +#define  PCI_REBAR_CTRL_BAR_IDX 0x00000007  /* BAR index */
>>>>>> +#define  PCI_REBAR_CTRL_NBAR_MASK       0x000000E0  /* # of resizable 
>>>>>> BARs */
>>>>>> +#define  PCI_REBAR_CTRL_BAR_SIZE        0x00001F00  /* BAR size */
>>>>>> +#define  PCI_REBAR_CTRL_SIZE(v) \
>>>>>> +            (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) + 20))
>>>>>
>>>>> The literal 20 (appearing here the 2nd time) also wants hiding behind a
>>>>> #define.
>>>> OK, will add " #define PCI_REBAR_SIZE_UNIT_BYTES_LEN 20" to replace above 
>>>> two '20' case.
>>>
>>> What is "UNIT_BYTES_LEN" there? There's nothing byte-ish here, I don't
>>> think, 20 is simply the shift bias.
>> It's a naming problem. What I want to express here is that the basic unit is 
>> 1MB, which is 2^20 of bytes.
>> Since the spec has the definition about the value of the bar size bits of 
>> register:
>> BAR Size - This is an encoded value.
>> 0    1 MB (2^20 bytes)
>> 1    2 MB (2^21 bytes)
>> 2    4 MB (2^22 bytes)
>> 3    8 MB (2^23 bytes)
>> …
>> 43   8 EB (2^63 bytes)
>> Do you have suggestion about this macro name?
> 
> PCI_REBAR_SIZE_BIAS? PCI_REBAR_SIZE_SHIFT_BIAS? PCI_REBAR_SIZE_SHIFT?
Will change to PCI_REBAR_SIZE_BIAS, thanks.

> 
> Jan

-- 
Best regards,
Jiqian Chen.