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.c...@amd.com>
>>> + */
>>> +
>>> +#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
