Re: [PATCH V2] Handle wrap around in limit calculation

[Peter Maydell]

On Mon, 15 Jan 2024 at 05:58, Shlomo Pongratz <shlomopongratz@gmail.com> wrote:
>
> Thank you.
> Please see comments inline.
>
> On Fri, Jan 12, 2024 at 7:03 PM Peter Maydell <peter.maydell@linaro.org> 
> wrote:
> >
> > On Tue, 9 Jan 2024 at 12:45, Shlomo Pongratz <shlomopongratz@gmail.com> 
> > wrote:
> >
> > Hi; thanks for this patch.
> >
> > > Hanlde wrap around caused by the fact that perior to version 460A
> >
> > Is this "460A" version number a version of the hardware
> > we're modelling ?
> >
> > > the limit was 32bit quantity.
> > > See Linux kernel code in:
> > > drivers/pci/controllers/dwc/pcie-designware.c
> >
> > "/controller/"
> >
> > > function: __dw_pcie_prog_outbound_atu
> >
> > There don't seem to be any comments in this kernel function
> > that say anything about wrap-around:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/dwc/pcie-designware.c#L468
> >
> > so I'm not sure what you're trying to explain by referring to it.
> This is just to give some  context.
> If you look at the original submission of the controller patch 
> d64e5eabc4c7e20c
> pci: Add support for Designware IP block by Andrey Smirnov it is written there
> "Add code needed to get a functional PCI subsytem when using in
>     conjunction with upstream Linux guest (4.13+)."
> >
> > > Now in a 64bit system the range can be above 4G but as long as
> > > the limit itself is less then 4G the overflow is avoided
> > >
> > > Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
> > >
> > > ----
> > >
> > > Changes since v1:
> > >  * Seperate subject and description
> > > ---
> > >  hw/pci-host/designware.c | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> > > index dd9e389c07..7ce4a6b64d 100644
> > > --- a/hw/pci-host/designware.c
> > > +++ b/hw/pci-host/designware.c
> > > @@ -269,11 +269,24 @@ static void 
> > > designware_pcie_update_viewport(DesignwarePCIERoot *root,
> > >  {
> > >      const uint64_t target = viewport->target;
> > >      const uint64_t base   = viewport->base;
> > > -    const uint64_t size   = (uint64_t)viewport->limit - base + 1;
> > >      const bool enabled    = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
> > > +    uint64_t tbase, tlimit, size;
> > >
> > >      MemoryRegion *current, *other;
> > >
> > > +    /*
> > > +     * Hanlde wrap around caused by the fact that perior to version 460A
> > > +     * the limit was 32bit quantity.
> > > +     * See Linux kernel code in:
> > > +     * drivers/pci/controllers/dwc/pcie-designware.c
> > > +     * function: __dw_pcie_prog_outbound_atu
> > > +     * Now in a 64bit system the range can be above 4G but as long as
> > > +     * the limit itself is less then 4G the overflow is avoided
> > > +     */
> > > +    tbase = base & 0xffffffff;
> > > +    tlimit = 0x100000000 + (uint64_t)viewport->limit;
> > > +    size = ((tlimit - tbase) & 0xffffffff) + 1;
> > > +
> >
> > I find this patch a bit confusing, partly because the comment
> > seems to be written from the perspective of what the kernel
> > driver is doing, not from the perspective of the hardware
> > behaviour.
> >
> Again I refer to the original patch comment.
> The original patch was written to support Linux kernel 4.13+ and a
> specific implementation i.MX6 Applications Processor.
> I've looked at the i.MX6 reference manual and it was silent regarding
> the wrap-around case.
> There is no reference to the  relevant Synopsys' Designware's specification.
> Note that the pci version of the QEMU is 0, therefore I assume that
> the implementation was tailored
> to a specific implementation.
> > What is the behaviour of the actual hardware here, both before
> > and after 460A ? Which version are we trying to model?
> I don't have access to the pantora of Synopsys' Designware's root port.
> I can only conclude from the Linux kernel code that although the base
> address was always 64 bit quantity,
> then before version 460A that the limit quantity was 32 bit quantity
> and from version 460A it is 64 bit quantity.
> And the document that the original patch was based on didn't say what
> is the behavior in case of wrap around.
> I don't want to model any specific version, I just want to work with
> device tree definitions that has regions above 4G,
> which are possible since the base address is 64 bit quantity as long
> as the regions size are
> less teh 4G.

But we must model *something*, which is ideally "what the spec
says" or possibly "what some specific version is". In this
particular case, we need to be clear about whether we are
modelling the pre-460A behaviour or the 460A-and-onward
behaviour. "This change seems to be enough to make Linux
work" is generally not sufficient to justify it.

If all we have is the Linux driver code then the flow
has to be:
 * look at what the kernel does
 * deduce what we think the hardware implementation must
   be, based on that plus common sense about what is and
   isn't typical and easy for hardware to do
 * implement that, with comments about where we're making
   guesses

For instance, the kernel code suggests that pre-460A
there's a 32 bit limit register, and post-460A there
is a 64-bit limit (with an "UPPER_LIMIT" register to
access the top 32 bits), plus a PCIE_ATU_INCREASE_REGION_SIZE
flag bit. That suggests we might either:
 (1) implement all that
 (2) say we're implementing a pre-460A version with a
     32 bit limit field
Presumably we're aiming for (2) here, but I find the
arithmetic you have in this patch completely confusing:
it doesn't look like something hardware would be doing
when it has a 64 bit base address register and a 32 bit limit
address register. It's also weird as C code, because you
add 0x100000000 when calculating tlimit, but this will
have no effect because of the AND with 0xffffffff in
the following line.

My guess at what the hardware is doing is "the actual
limit address takes its low 32 bits from the limit address
register and the high 32 bits from the high 32 bits of
the base address".

thanks
-- PMM