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

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

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?

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

> 
>> +    index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
>> +    if ( index >= PCI_HEADER_NORMAL_NR_BARS )
>> +        return;
>> +
>> +    if ( bars[index].type != VPCI_BAR_MEM64_LO &&
>> +         bars[index].type != VPCI_BAR_MEM32 )
>> +        return;
>> +
>> +    size = PCI_REBAR_CTRL_SIZE(val);
>> +    if ( !((size >> 20) &
>> +         MASK_EXTR(pci_conf_read32(pdev->sbdf, reg - 4), 
>> PCI_REBAR_CAP_SIZES)) )
> 
> No such literal 4 please. What I think you mean is reg - PCI_REBAR_CTRL +
> PCI_REBAR_CAP.
Yes, will change, thanks.

> 
> Also indentation is off (by 2) here.
> 
>> +        gprintk(XENLOG_WARNING,
>> +                "%pp: new size %#lx for BAR%u isn't supported\n",
>> +                &pdev->sbdf, size, index);
>> +
>> +    bars[index].size = size;
>> +    bars[index].addr = 0;
>> +    bars[index].guest_addr = 0;
>> +    pci_conf_write32(pdev->sbdf, reg, val);
>> +}
>> +
>> +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;
>> +
>> +    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 )
>> +    {
>> +        int rc;
>> +
>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
>> +                               rebar_offset + PCI_REBAR_CAP, 4, NULL);
> 
> The capability register is r/o aiui. While permitting hwdom to write it is
> fine, DomU-s shouldn't be permitted doing so, just in case. (An alternative
> to making handler selection conditional here would be to bail early for the
> !hwdom case, accompanied by a TODO comment. This would then also address
> the lack of virtualization of the extended capability chain, as we may not
> blindly expose all capabilities to DomU-s.)
Thanks, will add is_hwdom check and add "TODO" comment here.

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

> 
>> +        }
>> +    }
>> +
>> +    return 0;
> 
> ... you - imo sensibly - aren't communicating the error back up (to allow
> the device to be used without BAR resizing.
> 
>> @@ -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?

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

> 
> Jan

-- 
Best regards,
Jiqian Chen.