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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>>>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>>>>> +                                      unsigned int reg,
>>>>> +                                      uint32_t val,
>>>>> +                                      void *data)
>>>>> +{
>>>>> +    uint64_t size;
>>>>> +    unsigned int index;
>>>>> +    struct vpci_bar *bars = data;
>>>>> +
>>>>> +    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>>>>> +        return;
>>>>
>>>> I don't think something like this can go uncommented. I don't think the
>>>> spec mandates to drop writes in this situation?
>>> Spec says: Software must clear the Memory Space Enable bit in the Command 
>>> register before writing the BAR Size field.
>>> This check is suggested by Roger and it really helps to prevent erroneous 
>>> writes in this case,
>>> such as the result of debugging with Roger in the previous version.
>>> I will add the spec's sentences as comments here in next version.
>>
>> What you quote from the spec may not be enough as a comment here. There's
>> no direct implication that the write would simply be dropped on the floor
>> if the bit is still set. So I think you want to go a little beyond just
>> quoting from the spec.
> How about quoting Roger's previous words: " The memory decoding must be 
> disabled before writing the BAR size field.
> Otherwise changing the BAR size will lead to the active p2m mappings getting 
> out of sync w.r.t. the new BAR size." ?

That'll be better, but imo still not enough to explain the outright ignoring
of the write.

>>>>> +        if ( rc )
>>>>> +        {
>>>>> +            printk("%pp: add register for PCI_REBAR_CAP failed 
>>>>> (rc=%d)\n",
>>>>> +                   &pdev->sbdf, rc);
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, 
>>>>> rebar_ctrl_write,
>>>>> +                               rebar_offset + PCI_REBAR_CTRL, 4,
>>>>> +                               pdev->vpci->header.bars);
>>>>> +        if ( rc )
>>>>> +        {
>>>>> +            printk("%pp: add register for PCI_REBAR_CTRL failed %d\n",
>>>>> +                   &pdev->sbdf, rc);
>>>>> +            break;
>>>>
>>>> Is it correct to keep the other handler installed? After all ...
>>> Will change to "return rc;" here and above in next version.
>>
>> I'm not convinced this is what we want, as per ...
>>
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>
>>>> ... you - imo sensibly - aren't communicating the error back up (to allow
>>>> the device to be used without BAR resizing.
>>
>> ... what I said here.
> Sorry, I didn’t understand.
> Do you mean it is not enough to return error code once a handler failed to be 
> installed, I need to remove the already installed handlers?

No, if you return an error here, nothing else needs doing. However, I
question that returning an error here is good or even necessary. In
the event of an error, the device ought to still be usable, just
without the BAR-resizing capability.

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

>>>>> +#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?

Jan