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

[Shlomo Pongratz <shlomopongratz@gmail.com>]

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.
>> thanks
>> -- PMM