Re: [PATCH v3] vpci: Add resizable bar support

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

On 2024/12/16 19:24, Jan Beulich wrote:
> On 13.12.2024 06:42, Jiqian Chen wrote:
>> --- /dev/null
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -0,0 +1,130 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * 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;
>> +    struct vpci_bar *bar = data;
>> +
>> +    size = PCI_REBAR_CTRL_SIZE(val);
>> +    if ( bar->enabled )
>> +    {
>> +        if ( size == bar->size )
>> +            return;
>> +
>> +        /*
>> +        * Refuse to resize a BAR while memory decoding is enabled, as
>> +        * otherwise the size of the mapped region in the p2m would become
>> +        * stale with the newly set BAR size, and the position of the BAR
>> +        * would be reset to undefined.  Note the PCIe specification also
>> +        * forbids resizing a BAR with memory decoding enabled.
>> +        */
>> +        gprintk(XENLOG_ERR,
>> +                "%pp: refuse to resize BAR with memory decoding enabled\n",
>> +                &pdev->sbdf);
>> +        return;
>> +    }
>> +
>> +    if ( !((size >> PCI_REBAR_SIZE_BIAS) &
>> +           MASK_EXTR(pci_conf_read32(pdev->sbdf,
>> +                                     reg - PCI_REBAR_CTRL + PCI_REBAR_CAP),
>> +                                     PCI_REBAR_CAP_SIZES)) )
>> +        gprintk(XENLOG_WARNING,
>> +                "%pp: new size %#lx is not supported by hardware\n",
>> +                &pdev->sbdf, size);
> 
> This only covers the 1Mb ... 128Tb range. What about the 256Tb ... 8Eb one?
Thank you for reminding me!
I will consider that in next version.

> 
>> +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;
>> +
>> +    if ( !is_hardware_domain(pdev->domain) )
>> +    {
>> +        printk("ReBar is not supported for domUs\n");
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    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 )
> 
> PCI_REBAR_CTRL is an offset; it can't be used to bump rebar_offset here.
> That'll need a separate constant, even if both evaluate to 8.
I will add a new macro to represent the '8' in rebar.c
Maybe I can name it "PCI_REBAR_SINGLE_BAR_LEN" ?

> 
> Jan

-- 
Best regards,
Jiqian Chen.