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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

Jan