| Message ID | 20240109124333.224240-1-shlomop@pliops.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] Handle wrap around in limit calculation | expand |
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. > 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. What is the behaviour of the actual hardware here, both before and after 460A ? Which version are we trying to model? thanks -- PMM
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
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
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
On 15/01/2024 12:37, Peter Maydell wrote: See inline. > 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". The original code which claims to be based on i.MX6 Applications Processor actually fails for the example given there in page 4131 where the lower is set to 0xd0000000 the upper to 0x8000000 and the limit to 0xd000ffff which gives us a size of 0x8000000000010000, which is a negative number. Therefore some fix need to be done. Your suggestion solve this issue and gives the correct address even if the addresses are translated by for example by a multiple of 4G, e.g 0x200000000, but it won't work if the range is translated in a way that is cross 4G boundary (e.g. translate by 0x2ffff000). After reviewing the mathematics I've found a solution which to my humiliation is more simple and it is likely that the HW is doing it, and this is just to ignore the high 32 bits of the calculation's result. That is: const uint64_t size = ((uint64_t)viewport->limit - base + 1) & 0xffffffff; So this brings the implementation to complies with the spec it claims that it is based upon. Is this acceptable or you have a counter example.? > thanks > -- PMM ------------------------------------------------------------------------ *From:* Peter Maydell [mailto:peter.maydell@linaro.org] *Sent:* Monday, January 15, 2024, 12:37 PM *To:* Shlomo Pongratz *Cc:* qemu-devel@nongnu.org, andrew.sminov@gmail.com, peter.maydell@linaro.com, shlomop@pliops.com *Subject:* [PATCH V2] Handle wrap around in limit calculation > 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
On Mon, 15 Jan 2024 at 13:51, Shlomo Pongratz <shlomopongratz@gmail.com> wrote: > > On 15/01/2024 12:37, Peter Maydell wrote: > > 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". > The original code which claims to be based on i.MX6 Applications Processor > actually fails for the example given there in page 4131 where the lower > is set to 0xd0000000 > the upper to 0x8000000 and the limit to 0xd000ffff which gives us a size > of 0x8000000000010000, > which is a negative number. Therefore some fix need to be done. Ah, I hadn't realised that this PCI controller was documented in the i.MX6 applications processor reference manual. Looking at that I see that the "upper base address register" is documented as "Forms bits [63:32] of the start (and end) address of the address region to be translated". That "(and end)" part confirms my guess that the 64-bit limit address is supposed to be formed by putting together the upper-base-address with the limit value. > Your suggestion solve this issue and gives the correct address even if > the addresses are translated by for example by a multiple of 4G, e.g > 0x200000000, > but it won't work if the range is translated in a way that is cross 4G > boundary (e.g. translate by 0x2ffff000). > > After reviewing the mathematics I've found a solution which to my > humiliation is more simple and it is likely that the HW > is doing it, and this is just to ignore the high 32 bits of the > calculation's result. That is: > const uint64_t size = ((uint64_t)viewport->limit - base + 1) & 0xffffffff; That gives the wrong answer for the case where the limit register is set to 0xFFFF_FFFF (it will give you 0 rather than 0x1_0000_0000). I think my suggestion is: uint64_t base = viewport->base; uint64_t limit = viewport->limit; uint64_t size; /* * Versions 460A and above of this PCI controller have a * 64-bit limit register. We currently model the older type * where the limit register is 32 bits, and the upper 32 bits * of the end address are the same as the upper 32 bits of * the start address. */ limit = deposit64(limit, 32, 32, extract64(base, 32, 32)); size = limit - base + 1; That's a fairly clear implementation of what the docs say the behaviour is, and it gives us a nice easy place to add 64-bit limit register support if we ever need to (by guarding the setting of 'limit' with an "if 64-bit limit registers not enabled" check). thanks -- PMM
See Inline. On 15/01/2024 18:47, Peter Maydell wrote: > On Mon, 15 Jan 2024 at 13:51, Shlomo Pongratz <shlomopongratz@gmail.com> wrote: >> On 15/01/2024 12:37, Peter Maydell wrote: >>> 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". >> The original code which claims to be based on i.MX6 Applications Processor >> actually fails for the example given there in page 4131 where the lower >> is set to 0xd0000000 >> the upper to 0x8000000 and the limit to 0xd000ffff which gives us a size >> of 0x8000000000010000, >> which is a negative number. Therefore some fix need to be done. > Ah, I hadn't realised that this PCI controller was documented > in the i.MX6 applications processor reference manual. Looking > at that I see that the "upper base address register" is documented > as "Forms bits [63:32] of the start (and end) address of the address > region to be translated". That "(and end)" part confirms my > guess that the 64-bit limit address is supposed to be formed by > putting together the upper-base-address with the limit value. Actually this is one implementation of one pre-460A and we don't even knows which. And QEMU version is 0 which I assume should be some ideal implementation. So as I suspect there is an implied HW limitation that a region should never cross 4G boundary e.g. [0x80000000fffff000 0x800000010000efff] will be sent as 0x80000000fffff000 and 0xefff resulting in[0x80000000fffff000 0x800000000000efff] >> Your suggestion solve this issue and gives the correct address even if >> the addresses are translated by for example by a multiple of 4G, e.g >> 0x200000000, >> but it won't work if the range is translated in a way that is cross 4G >> boundary (e.g. translate by 0x2ffff000). >> >> After reviewing the mathematics I've found a solution which to my >> humiliation is more simple and it is likely that the HW >> is doing it, and this is just to ignore the high 32 bits of the >> calculation's result. That is: >> const uint64_t size = ((uint64_t)viewport->limit - base + 1) & 0xffffffff; > That gives the wrong answer for the case where the limit register > is set to 0xFFFF_FFFF (it will give you 0 rather than 0x1_0000_0000). This was a typo The + 1 should have been added after masking. That is the calculation is done in 32 bit. uint32_t tbase; uint32_t tsize; uint64_t size; uint64_t tlimit; // Use 32 bit calculation tbase = base; // Truncate tsize = limit - tbase; // Return to 64 bit size = tsize; size++; > I think my suggestion is: > > uint64_t base = viewport->base; > uint64_t limit = viewport->limit; > uint64_t size; > > /* > * Versions 460A and above of this PCI controller have a > * 64-bit limit register. We currently model the older type > * where the limit register is 32 bits, and the upper 32 bits > * of the end address are the same as the upper 32 bits of > * the start address. > */ > limit = deposit64(limit, 32, 32, extract64(base, 32, 32)); > size = limit - base + 1; This will work the only thing is that it will not work if for example The original range is [0x80000000fffff000 0x800000010000efff] Stuffing 0x80000000fffff000 0x0efff to your solution yields size 0xffffffff00010000 and as result a limit of 0x800000000000efff Note that the 32bit calculation above gives the original values. Full simulation of all algorithms over some ranges can be found in my gist https://gist.github.com/shlomopongartz/f82466b6044f3a1653890b43b046c443 Anyway I assume that in your opinion we should keep the HW limitation and not try to fix the HW > That's a fairly clear implementation of what the docs say the > behaviour is, and it gives us a nice easy place to add 64-bit > limit register support if we ever need to (by guarding the setting > of 'limit' with an "if 64-bit limit registers not enabled" check). I'll be glad to see post 460A implementation. Since until then it will be impossible to use the same device tree in a project where the HW are working in a post-A460 controller and there range crosses 4G boundary. > thanks > -- PMM ------------------------------------------------------------------------ *From:* Peter Maydell [mailto:peter.maydell@linaro.org] *Sent:* Monday, January 15, 2024, 6:47 PM *To:* Shlomo Pongratz *Cc:* qemu-devel@nongnu.org, andrew.sminov@gmail.com, peter.maydell@linaro.com, shlomop@pliops.com *Subject:* [PATCH V2] Handle wrap around in limit calculation > On Mon, 15 Jan 2024 at 13:51, Shlomo Pongratz <shlomopongratz@gmail.com> wrote: >> On 15/01/2024 12:37, Peter Maydell wrote: >>> 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". >> The original code which claims to be based on i.MX6 Applications Processor >> actually fails for the example given there in page 4131 where the lower >> is set to 0xd0000000 >> the upper to 0x8000000 and the limit to 0xd000ffff which gives us a size >> of 0x8000000000010000, >> which is a negative number. Therefore some fix need to be done. > Ah, I hadn't realised that this PCI controller was documented > in the i.MX6 applications processor reference manual. Looking > at that I see that the "upper base address register" is documented > as "Forms bits [63:32] of the start (and end) address of the address > region to be translated". That "(and end)" part confirms my > guess that the 64-bit limit address is supposed to be formed by > putting together the upper-base-address with the limit value. > >> Your suggestion solve this issue and gives the correct address even if >> the addresses are translated by for example by a multiple of 4G, e.g >> 0x200000000, >> but it won't work if the range is translated in a way that is cross 4G >> boundary (e.g. translate by 0x2ffff000). >> >> After reviewing the mathematics I've found a solution which to my >> humiliation is more simple and it is likely that the HW >> is doing it, and this is just to ignore the high 32 bits of the >> calculation's result. That is: >> const uint64_t size = ((uint64_t)viewport->limit - base + 1) & 0xffffffff; > That gives the wrong answer for the case where the limit register > is set to 0xFFFF_FFFF (it will give you 0 rather than 0x1_0000_0000). > > I think my suggestion is: > > uint64_t base = viewport->base; > uint64_t limit = viewport->limit; > uint64_t size; > > /* > * Versions 460A and above of this PCI controller have a > * 64-bit limit register. We currently model the older type > * where the limit register is 32 bits, and the upper 32 bits > * of the end address are the same as the upper 32 bits of > * the start address. > */ > limit = deposit64(limit, 32, 32, extract64(base, 32, 32)); > size = limit - base + 1; > > That's a fairly clear implementation of what the docs say the > behaviour is, and it gives us a nice easy place to add 64-bit > limit register support if we ever need to (by guarding the setting > of 'limit' with an "if 64-bit limit registers not enabled" check). > > thanks > -- PMM
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; + if (viewport->cr[0] == DESIGNWARE_PCIE_ATU_TYPE_MEM) { current = &viewport->mem; other = &viewport->cfg;
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 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(-)