On Wed, Jan 08, 2025 at 08:19:55AM +0100, Jan Beulich wrote:
> On 07.01.2025 19:19, Roger Pau Monné wrote:
> > On Tue, Jan 07, 2025 at 04:58:07PM +0100, Jan Beulich wrote:
> >> On 07.01.2025 15:38, Roger Pau Monné wrote:
> >>> On Tue, Jan 07, 2025 at 11:06:33AM +0100, Jan Beulich wrote:
> >>>> On 19.12.2024 06:21, Jiqian Chen wrote:
> >>>>> --- /dev/null
> >>>>> +++ b/xen/drivers/vpci/rebar.c
> >>>>> @@ -0,0 +1,131 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
> >>>>> +/*
> >>>>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> >>>>> + *
> >>>>> + * Author: Jiqian Chen <jiqian.c...@amd.com>
> >>>>> + */
> >>>>> +
> >>>>> +#include <xen/sched.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)
> >>>>> +{
> >>>>> + struct vpci_bar *bar = data;
> >>>>> + uint64_t size = PCI_REBAR_CTRL_SIZE(val);
> >>>>> +
> >>>>> + if ( bar->enabled )
> >>>>> + {
> >>>>> + /*
> >>>>> + * 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.
> >>>>> + */
> >>>>> + if ( size != bar->size )
> >>>>> + gprintk(XENLOG_ERR,
> >>>>> + "%pp: refuse to resize BAR with memory decoding
> >>>>> enabled\n",
> >>>>> + &pdev->sbdf);
> >>>>> + return;
> >>>>> + }
> >>>>> +
> >>>>> + if ( !((size >> PCI_REBAR_SIZE_BIAS) & bar->resizable_sizes) )
> >>>>> + gprintk(XENLOG_WARNING,
> >>>>> + "%pp: new size %#lx is not supported by hardware\n",
> >>>>> + &pdev->sbdf, size);
> >>>>> +
> >>>>> + bar->size = size;
> >>>>
> >>>> Shouldn't at least this be in an "else" to the if() above?
> >>>
> >>> I think this was already raised in a previous version - would be good
> >>> to know how real hardware behaves when an invalid size is set. Is the
> >>> BAR register still reset?
> >>
> >> I'm pretty sure what happens is undefined. I'd expect though that the
> >> BAR size then doesn't change. Which would require the above assignment
> >> to not be unconditional.
> >
> > Might be better to just re-size the BAR, like you suggested to fetch
> > the BAR position from the register, instead of assuming 0.
>
> FTAOD by "re-size" you mean re-obtain its size (seeing we're talking of
> re-sizable BARs here)? As kind of confirmed ...
Indeed, I meant to re-obtain the size (I can see that being
confusing in this context, sorry).
> >>>>> --- a/xen/drivers/vpci/vpci.c
> >>>>> +++ b/xen/drivers/vpci/vpci.c
> >>>>> @@ -232,6 +232,12 @@ void cf_check vpci_hw_write16(
> >>>>> pci_conf_write16(pdev->sbdf, reg, val);
> >>>>> }
> >>>>>
> >>>>> +void cf_check vpci_hw_write32(
> >>>>> + const struct pci_dev *pdev, unsigned int reg, uint32_t val, void
> >>>>> *data)
> >>>>> +{
> >>>>> + pci_conf_write32(pdev->sbdf, reg, val);
> >>>>> +}
> >>>>
> >>>> This function is being added just to handle writing of a r/o register.
> >>>> Can't you better re-use vpci_ignored_write()?
> >>>
> >>> But vpci_ignored_write() ignores the write, OTOH here the write is
> >>> propagated to the hardware.
> >>
> >> Right, just for the hardware to drop it. I wouldn't have commented if
> >> the function needed to do things like this already existed. Adding yet
> >> another cf_check function just for this is what made me give the remark.
> >
> > According to the spec yes, they will be ignored. Yet for the hardware
> > domain we try to avoid changing behavior from native as much as
> > possible, hence propagating the write seems more appropriate.
>
> Okay; you're the maintainer of this code anyway.
Thanks for all your input Jan, you might not be the maintainer but
have certainly reviewed all vPCI code.
Roger.
