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

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

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:
>>>> --- /dev/null
>>>> +++ b/xen/drivers/vpci/rebar.c
>>>> @@ -0,0 +1,93 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>
>>> Was this a deliberate decision? We default to GPL-2.0-only, I think.
>> Will change to GPL-2.0-only.
>> What's the difference between GPL-2.0-only and GPL-2.0-or-later?
> 
> As the name says, the latter includes any known or yet to be written newer
> versions of the GPL.
> 
>>>> +/*
>>>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
>>>> + *
>>>> + * Author: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>>>> + */
>>>> +
>>>> +#include <xen/hypercall.h>
>>>> +#include <xen/vpci.h>
>>>> +
>>>> +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." ?

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

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

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

> 
> Jan

-- 
Best regards,
Jiqian Chen.