* [PATCH v3] Handle wrap around in limit calculation
@ 2024-01-21 16:47 Shlomo Pongratz
2024-01-21 23:17 ` Philippe Mathieu-Daudé
2024-01-22 9:01 ` Peter Maydell
0 siblings, 2 replies; 5+ messages in thread
From: Shlomo Pongratz @ 2024-01-21 16:47 UTC (permalink / raw)
To: qemu-devel; +Cc: andrew.sminov, peter.maydell, shlomop, shlomopongratz
From: Shlomo Pongratz <shlomopongratz@gmail.com>
Hanlde wrap around when calculating the viewport size
caused by the fact that perior to version 460A the limit variable
was 32bit quantity and not 64 bits quantity.
In the i.MX 6Dual/6Quad Applications Processor Reference Manual
document on which the original code was based upon in the
description of the iATU Region Upper Base Address Register it is
written:
Forms bits [63:32] of the start (and end) address of the address region to be
translated.
That is in this register is the upper of both base and the limit.
In the current implementation this value was ignored for the limit
which caused a wrap around of the size calculaiton.
Using the documnet example:
Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
0x010000
The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 = 0x8000000000010000
Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
----
Changes since v2:
* Don't try to fix the calculation.
* Change the limit variable from 32bit to 64 bit
* Set the limit bits [63:32] same as the base according to
the specification on which the original code was base upon.
Changes since v1:
* Seperate subject and description
---
hw/pci-host/designware.c | 19 ++++++++++++++-----
include/hw/pci-host/designware.h | 2 +-
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index dd9e389c07..43cba9432f 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -269,7 +269,7 @@ 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 uint64_t size = viewport->limit - base + 1;
const bool enabled = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
MemoryRegion *current, *other;
@@ -351,6 +351,14 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
case DESIGNWARE_PCIE_ATU_UPPER_BASE:
viewport->base &= 0x00000000FFFFFFFFULL;
viewport->base |= (uint64_t)val << 32;
+ /* The documentatoin states that the value of this register
+ * "Forms bits [63:32] of the start (and end) address
+ * of the address region to be translated.
+ * Note that from version 406A there is a sperate
+ * register fot the upper end address
+ */
+ viewport->limit &= 0x00000000FFFFFFFFULL;
+ viewport->limit |= (uint64_t)val << 32;
break;
case DESIGNWARE_PCIE_ATU_LOWER_TARGET:
@@ -364,7 +372,8 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
break;
case DESIGNWARE_PCIE_ATU_LIMIT:
- viewport->limit = val;
+ viewport->limit &= 0xFFFFFFFF00000000ULL;
+ viewport->limit |= val;
break;
case DESIGNWARE_PCIE_ATU_CR1:
@@ -429,7 +438,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
viewport->inbound = true;
viewport->base = 0x0000000000000000ULL;
viewport->target = 0x0000000000000000ULL;
- viewport->limit = UINT32_MAX;
+ viewport->limit = 0x00000000FFFFFFFFULL;
viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
source = &host->pci.address_space_root;
@@ -453,7 +462,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
viewport->inbound = false;
viewport->base = 0x0000000000000000ULL;
viewport->target = 0x0000000000000000ULL;
- viewport->limit = UINT32_MAX;
+ viewport->limit = 0x00000000FFFFFFFFULL;
viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
destination = &host->pci.memory;
@@ -560,7 +569,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
.fields = (const VMStateField[]) {
VMSTATE_UINT64(base, DesignwarePCIEViewport),
VMSTATE_UINT64(target, DesignwarePCIEViewport),
- VMSTATE_UINT32(limit, DesignwarePCIEViewport),
+ VMSTATE_UINT64(limit, DesignwarePCIEViewport),
VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
VMSTATE_END_OF_LIST()
}
diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index 908f3d946b..51052683b7 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -41,7 +41,7 @@ typedef struct DesignwarePCIEViewport {
uint64_t base;
uint64_t target;
- uint32_t limit;
+ uint64_t limit;
uint32_t cr[2];
bool inbound;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] Handle wrap around in limit calculation
2024-01-21 16:47 [PATCH v3] Handle wrap around in limit calculation Shlomo Pongratz
@ 2024-01-21 23:17 ` Philippe Mathieu-Daudé
2024-01-22 7:14 ` Peter Xu
2024-01-22 8:37 ` Shlomo Pongratz
2024-01-22 9:01 ` Peter Maydell
1 sibling, 2 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-01-21 23:17 UTC (permalink / raw)
To: Shlomo Pongratz, qemu-devel
Cc: andrew.sminov, peter.maydell, shlomop, Peter Xu, Fabiano Rosas
Hi Shlomo,
On 21/1/24 17:47, Shlomo Pongratz wrote:
> From: Shlomo Pongratz <shlomopongratz@gmail.com>
>
> Hanlde wrap around when calculating the viewport size
> caused by the fact that perior to version 460A the limit variable
> was 32bit quantity and not 64 bits quantity.
> In the i.MX 6Dual/6Quad Applications Processor Reference Manual
> document on which the original code was based upon in the
> description of the iATU Region Upper Base Address Register it is
> written:
> Forms bits [63:32] of the start (and end) address of the address region to be
> translated.
> That is in this register is the upper of both base and the limit.
> In the current implementation this value was ignored for the limit
> which caused a wrap around of the size calculaiton.
> Using the documnet example:
> Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
> The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
> 0x010000
> The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 = 0x8000000000010000
>
> Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
>
> ----
>
> Changes since v2:
> * Don't try to fix the calculation.
> * Change the limit variable from 32bit to 64 bit
> * Set the limit bits [63:32] same as the base according to
> the specification on which the original code was base upon.
>
> Changes since v1:
> * Seperate subject and description
> ---
> hw/pci-host/designware.c | 19 ++++++++++++++-----
> include/hw/pci-host/designware.h | 2 +-
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index dd9e389c07..43cba9432f 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -269,7 +269,7 @@ 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 uint64_t size = viewport->limit - base + 1;
> const bool enabled = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
>
> MemoryRegion *current, *other;
> @@ -351,6 +351,14 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
> case DESIGNWARE_PCIE_ATU_UPPER_BASE:
> viewport->base &= 0x00000000FFFFFFFFULL;
> viewport->base |= (uint64_t)val << 32;
> + /* The documentatoin states that the value of this register
> + * "Forms bits [63:32] of the start (and end) address
> + * of the address region to be translated.
> + * Note that from version 406A there is a sperate
> + * register fot the upper end address
> + */
> + viewport->limit &= 0x00000000FFFFFFFFULL;
> + viewport->limit |= (uint64_t)val << 32;
This code is easier to review using:
viewport->limit = deposit64(viewport->limit, 32, 32, val);
> break;
>
> case DESIGNWARE_PCIE_ATU_LOWER_TARGET:
> @@ -364,7 +372,8 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
> break;
>
> case DESIGNWARE_PCIE_ATU_LIMIT:
> - viewport->limit = val;
> + viewport->limit &= 0xFFFFFFFF00000000ULL;
> + viewport->limit |= val;
Here:
viewport->limit = deposit64(viewport->limit, 0, 32, val);
> break;
>
> case DESIGNWARE_PCIE_ATU_CR1:
> @@ -429,7 +438,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
> viewport->inbound = true;
> viewport->base = 0x0000000000000000ULL;
> viewport->target = 0x0000000000000000ULL;
> - viewport->limit = UINT32_MAX;
> + viewport->limit = 0x00000000FFFFFFFFULL;
Previous code is easier to review IMHO.
> viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>
> source = &host->pci.address_space_root;
> @@ -453,7 +462,7 @@ static void designware_pcie_root_realize(PCIDevice *dev, Error **errp)
> viewport->inbound = false;
> viewport->base = 0x0000000000000000ULL;
> viewport->target = 0x0000000000000000ULL;
> - viewport->limit = UINT32_MAX;
> + viewport->limit = 0x00000000FFFFFFFFULL;
Ditto.
> viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>
> destination = &host->pci.memory;
> @@ -560,7 +569,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
> .fields = (const VMStateField[]) {
> VMSTATE_UINT64(base, DesignwarePCIEViewport),
> VMSTATE_UINT64(target, DesignwarePCIEViewport),
> - VMSTATE_UINT32(limit, DesignwarePCIEViewport),
> + VMSTATE_UINT64(limit, DesignwarePCIEViewport),
Unfortunately this breaks the migration stream. I'm not sure
what is the best way to deal with it (Cc'ing migration
maintainers).
> VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
> VMSTATE_END_OF_LIST()
> }
Regards,
Phil.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] Handle wrap around in limit calculation
2024-01-21 23:17 ` Philippe Mathieu-Daudé
@ 2024-01-22 7:14 ` Peter Xu
2024-01-22 8:37 ` Shlomo Pongratz
1 sibling, 0 replies; 5+ messages in thread
From: Peter Xu @ 2024-01-22 7:14 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Shlomo Pongratz, qemu-devel, andrew.sminov, peter.maydell,
shlomop, Fabiano Rosas
On Mon, Jan 22, 2024 at 12:17:24AM +0100, Philippe Mathieu-Daudé wrote:
> > @@ -560,7 +569,7 @@ static const VMStateDescription vmstate_designware_pcie_viewport = {
> > .fields = (const VMStateField[]) {
> > VMSTATE_UINT64(base, DesignwarePCIEViewport),
> > VMSTATE_UINT64(target, DesignwarePCIEViewport),
> > - VMSTATE_UINT32(limit, DesignwarePCIEViewport),
> > + VMSTATE_UINT64(limit, DesignwarePCIEViewport),
>
> Unfortunately this breaks the migration stream. I'm not sure
> what is the best way to deal with it (Cc'ing migration
> maintainers).
My understanding is that we can have at least two ways to do this, one
relying on machine type properties, the other one can be VMSD versioning.
Frankly I don't have a solid mind either on which is the best approach.
I never had a talk with either Juan / Dave before on this, but my
understanding is that VMSD versioning is just less-flexible, because it
doesn't support backward migrations (only forward). While machine-type
property based solution can support both (forward + backward).
I decided to draft a doc update for this, to put my thoughts here:
https://lore.kernel.org/qemu-devel/20240122070600.16681-1-peterx@redhat.com
It can be seen as a reference, or review comments also welcomed.
This device seems to be only supported by CONFIG_FSL_IMX7. Maybe vmsd
versioning would be good enough here, then? If so, instead of
VMSTATE_UINT64(), we may want a new VMSTATE_UINT32_V() with a boosted
version.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] Handle wrap around in limit calculation
2024-01-21 23:17 ` Philippe Mathieu-Daudé
2024-01-22 7:14 ` Peter Xu
@ 2024-01-22 8:37 ` Shlomo Pongratz
1 sibling, 0 replies; 5+ messages in thread
From: Shlomo Pongratz @ 2024-01-22 8:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: andrew.sminov, peter.maydell, shlomop, Peter Xu, Fabiano Rosas
Please see inline
On 22/01/2024 01:17, Philippe Mathieu-Daudé wrote:
> Hi Shlomo,
>
> On 21/1/24 17:47, Shlomo Pongratz wrote:
>> From: Shlomo Pongratz <shlomopongratz@gmail.com>
>>
>> Hanlde wrap around when calculating the viewport size
>> caused by the fact that perior to version 460A the limit variable
>> was 32bit quantity and not 64 bits quantity.
>> In the i.MX 6Dual/6Quad Applications Processor Reference Manual
>> document on which the original code was based upon in the
>> description of the iATU Region Upper Base Address Register it is
>> written:
>> Forms bits [63:32] of the start (and end) address of the address
>> region to be
>> translated.
>> That is in this register is the upper of both base and the limit.
>> In the current implementation this value was ignored for the limit
>> which caused a wrap around of the size calculaiton.
>> Using the documnet example:
>> Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
>> The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
>> 0x010000
>> The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 =
>> 0x8000000000010000
>>
>> Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
>>
>> ----
>>
>> Changes since v2:
>> * Don't try to fix the calculation.
>> * Change the limit variable from 32bit to 64 bit
>> * Set the limit bits [63:32] same as the base according to
>> the specification on which the original code was base upon.
>>
>> Changes since v1:
>> * Seperate subject and description
>> ---
>> hw/pci-host/designware.c | 19 ++++++++++++++-----
>> include/hw/pci-host/designware.h | 2 +-
>> 2 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
>> index dd9e389c07..43cba9432f 100644
>> --- a/hw/pci-host/designware.c
>> +++ b/hw/pci-host/designware.c
>> @@ -269,7 +269,7 @@ 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 uint64_t size = viewport->limit - base + 1;
>> const bool enabled = viewport->cr[1] &
>> DESIGNWARE_PCIE_ATU_ENABLE;
>> MemoryRegion *current, *other;
>> @@ -351,6 +351,14 @@ static void
>> designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>> case DESIGNWARE_PCIE_ATU_UPPER_BASE:
>> viewport->base &= 0x00000000FFFFFFFFULL;
>> viewport->base |= (uint64_t)val << 32;
>> + /* The documentatoin states that the value of this register
>> + * "Forms bits [63:32] of the start (and end) address
>> + * of the address region to be translated.
>> + * Note that from version 406A there is a sperate
>> + * register fot the upper end address
>> + */
>> + viewport->limit &= 0x00000000FFFFFFFFULL;
>> + viewport->limit |= (uint64_t)val << 32;
>
> This code is easier to review using:
>
> viewport->limit = deposit64(viewport->limit, 32, 32, val);
>
It will be strange to have
viewport->base &= 0x00000000FFFFFFFFULL;
viewport->base |= (uint64_t)val << 32;
and then
viewport->limit = deposit64(viewport->limit, 32, 32, val);
I think that the code for base and limit should be the same.
And I don't think the original for base should be change to
viewport->base = deposit64(viewport->base, 32, 32, val);
SO everything will look the same.
>> break;
>> case DESIGNWARE_PCIE_ATU_LOWER_TARGET:
>> @@ -364,7 +372,8 @@ static void
>> designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>> break;
>> case DESIGNWARE_PCIE_ATU_LIMIT:
>> - viewport->limit = val;
>> + viewport->limit &= 0xFFFFFFFF00000000ULL;
>> + viewport->limit |= val;
>
> Here:
>
> viewport->limit = deposit64(viewport->limit, 0, 32, val);
My opinion is that the code should be identical to
case DESIGNWARE_PCIE_ATU_LOWER_BASE:
viewport->base &= 0xFFFFFFFF00000000ULL;
viewport->base |= val;
break;
I don't think it is good to mix two styles.
>> break;
>> case DESIGNWARE_PCIE_ATU_CR1:
>> @@ -429,7 +438,7 @@ static void
>> designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>> viewport->inbound = true;
>> viewport->base = 0x0000000000000000ULL;
>> viewport->target = 0x0000000000000000ULL;
>> - viewport->limit = UINT32_MAX;
>> + viewport->limit = 0x00000000FFFFFFFFULL;
>
> Previous code is easier to review IMHO.
Just want to make it clear that this is 64 bit values and that the
upper value is the same as the base's upper value, and according to spec.
>> viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>> source = &host->pci.address_space_root;
>> @@ -453,7 +462,7 @@ static void
>> designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>> viewport->inbound = false;
>> viewport->base = 0x0000000000000000ULL;
>> viewport->target = 0x0000000000000000ULL;
>> - viewport->limit = UINT32_MAX;
>> + viewport->limit = 0x00000000FFFFFFFFULL;
>
> Ditto.
Ditto.
>
>> viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>> destination = &host->pci.memory;
>> @@ -560,7 +569,7 @@ static const VMStateDescription
>> vmstate_designware_pcie_viewport = {
>> .fields = (const VMStateField[]) {
>> VMSTATE_UINT64(base, DesignwarePCIEViewport),
>> VMSTATE_UINT64(target, DesignwarePCIEViewport),
>> - VMSTATE_UINT32(limit, DesignwarePCIEViewport),
>> + VMSTATE_UINT64(limit, DesignwarePCIEViewport),
>
> Unfortunately this breaks the migration stream. I'm not sure
> what is the best way to deal with it (Cc'ing migration
> maintainers).
My bad forgot to update version_id and minimum_version_id.
I'll consult Peter Xu's document.
>
>> VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
>> VMSTATE_END_OF_LIST()
>> }
> Regards,
>
> Phil.
------------------------------------------------------------------------
*From:* Philippe Mathieu-Daudé [mailto:philmd@linaro.org]
*Sent:* Monday, January 22, 2024, 1:17 AM
*To:* Shlomo Pongratz; qemu-devel@nongnu.org
*Cc:* andrew.sminov@gmail.com, peter.maydell@linaro.com,
shlomop@pliops.com, Peter Xu; Fabiano Rosas
*Subject:* [PATCH v3] Handle wrap around in limit calculation
> Hi Shlomo,
>
> On 21/1/24 17:47, Shlomo Pongratz wrote:
>> From: Shlomo Pongratz <shlomopongratz@gmail.com>
>>
>> Hanlde wrap around when calculating the viewport size
>> caused by the fact that perior to version 460A the limit variable
>> was 32bit quantity and not 64 bits quantity.
>> In the i.MX 6Dual/6Quad Applications Processor Reference Manual
>> document on which the original code was based upon in the
>> description of the iATU Region Upper Base Address Register it is
>> written:
>> Forms bits [63:32] of the start (and end) address of the address
>> region to be
>> translated.
>> That is in this register is the upper of both base and the limit.
>> In the current implementation this value was ignored for the limit
>> which caused a wrap around of the size calculaiton.
>> Using the documnet example:
>> Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
>> The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
>> 0x010000
>> The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 =
>> 0x8000000000010000
>>
>> Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
>>
>> ----
>>
>> Changes since v2:
>> * Don't try to fix the calculation.
>> * Change the limit variable from 32bit to 64 bit
>> * Set the limit bits [63:32] same as the base according to
>> the specification on which the original code was base upon.
>>
>> Changes since v1:
>> * Seperate subject and description
>> ---
>> hw/pci-host/designware.c | 19 ++++++++++++++-----
>> include/hw/pci-host/designware.h | 2 +-
>> 2 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
>> index dd9e389c07..43cba9432f 100644
>> --- a/hw/pci-host/designware.c
>> +++ b/hw/pci-host/designware.c
>> @@ -269,7 +269,7 @@ 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 uint64_t size = viewport->limit - base + 1;
>> const bool enabled = viewport->cr[1] &
>> DESIGNWARE_PCIE_ATU_ENABLE;
>> MemoryRegion *current, *other;
>> @@ -351,6 +351,14 @@ static void
>> designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>> case DESIGNWARE_PCIE_ATU_UPPER_BASE:
>> viewport->base &= 0x00000000FFFFFFFFULL;
>> viewport->base |= (uint64_t)val << 32;
>> + /* The documentatoin states that the value of this register
>> + * "Forms bits [63:32] of the start (and end) address
>> + * of the address region to be translated.
>> + * Note that from version 406A there is a sperate
>> + * register fot the upper end address
>> + */
>> + viewport->limit &= 0x00000000FFFFFFFFULL;
>> + viewport->limit |= (uint64_t)val << 32;
>
> This code is easier to review using:
>
> viewport->limit = deposit64(viewport->limit, 32, 32, val);
>
>> break;
>> case DESIGNWARE_PCIE_ATU_LOWER_TARGET:
>> @@ -364,7 +372,8 @@ static void
>> designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
>> break;
>> case DESIGNWARE_PCIE_ATU_LIMIT:
>> - viewport->limit = val;
>> + viewport->limit &= 0xFFFFFFFF00000000ULL;
>> + viewport->limit |= val;
>
> Here:
>
> viewport->limit = deposit64(viewport->limit, 0, 32, val);
>
>> break;
>> case DESIGNWARE_PCIE_ATU_CR1:
>> @@ -429,7 +438,7 @@ static void
>> designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>> viewport->inbound = true;
>> viewport->base = 0x0000000000000000ULL;
>> viewport->target = 0x0000000000000000ULL;
>> - viewport->limit = UINT32_MAX;
>> + viewport->limit = 0x00000000FFFFFFFFULL;
>
> Previous code is easier to review IMHO.
>
>> viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>> source = &host->pci.address_space_root;
>> @@ -453,7 +462,7 @@ static void
>> designware_pcie_root_realize(PCIDevice *dev, Error **errp)
>> viewport->inbound = false;
>> viewport->base = 0x0000000000000000ULL;
>> viewport->target = 0x0000000000000000ULL;
>> - viewport->limit = UINT32_MAX;
>> + viewport->limit = 0x00000000FFFFFFFFULL;
>
> Ditto.
>
>> viewport->cr[0] = DESIGNWARE_PCIE_ATU_TYPE_MEM;
>> destination = &host->pci.memory;
>> @@ -560,7 +569,7 @@ static const VMStateDescription
>> vmstate_designware_pcie_viewport = {
>> .fields = (const VMStateField[]) {
>> VMSTATE_UINT64(base, DesignwarePCIEViewport),
>> VMSTATE_UINT64(target, DesignwarePCIEViewport),
>> - VMSTATE_UINT32(limit, DesignwarePCIEViewport),
>> + VMSTATE_UINT64(limit, DesignwarePCIEViewport),
>
> Unfortunately this breaks the migration stream. I'm not sure
> what is the best way to deal with it (Cc'ing migration
> maintainers).
>
>> VMSTATE_UINT32_ARRAY(cr, DesignwarePCIEViewport, 2),
>> VMSTATE_END_OF_LIST()
>> }
> Regards,
>
> Phil.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] Handle wrap around in limit calculation
2024-01-21 16:47 [PATCH v3] Handle wrap around in limit calculation Shlomo Pongratz
2024-01-21 23:17 ` Philippe Mathieu-Daudé
@ 2024-01-22 9:01 ` Peter Maydell
1 sibling, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2024-01-22 9:01 UTC (permalink / raw)
To: Shlomo Pongratz; +Cc: qemu-devel, andrew.sminov, peter.maydell, shlomop
On Sun, 21 Jan 2024 at 16:48, Shlomo Pongratz <shlomopongratz@gmail.com> wrote:
>
> From: Shlomo Pongratz <shlomopongratz@gmail.com>
>
> Hanlde wrap around when calculating the viewport size
> caused by the fact that perior to version 460A the limit variable
> was 32bit quantity and not 64 bits quantity.
> In the i.MX 6Dual/6Quad Applications Processor Reference Manual
> document on which the original code was based upon in the
> description of the iATU Region Upper Base Address Register it is
> written:
> Forms bits [63:32] of the start (and end) address of the address region to be
> translated.
> That is in this register is the upper of both base and the limit.
> In the current implementation this value was ignored for the limit
> which caused a wrap around of the size calculaiton.
> Using the documnet example:
> Base HI: 0x80000000 Base LO: 0xd0000000 Limit LO: 0xd000ffff
> The correct size is 0x80000000d000ffff - 0x80000000d0000000 + 1 =
> 0x010000
> The wrong result is 0xd000ffff - 0x80000000d0000000 + 1 = 0x8000000000010000
>
> Signed-off-by: Shlomo Pongratz <shlomop@pliops.com>
>
> ----
>
> Changes since v2:
> * Don't try to fix the calculation.
> * Change the limit variable from 32bit to 64 bit
> * Set the limit bits [63:32] same as the base according to
> the specification on which the original code was base upon.
>
> Changes since v1:
> * Seperate subject and description
> ---
> hw/pci-host/designware.c | 19 ++++++++++++++-----
> include/hw/pci-host/designware.h | 2 +-
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index dd9e389c07..43cba9432f 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -269,7 +269,7 @@ 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 uint64_t size = viewport->limit - base + 1;
> const bool enabled = viewport->cr[1] & DESIGNWARE_PCIE_ATU_ENABLE;
>
> MemoryRegion *current, *other;
> @@ -351,6 +351,14 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
> case DESIGNWARE_PCIE_ATU_UPPER_BASE:
> viewport->base &= 0x00000000FFFFFFFFULL;
> viewport->base |= (uint64_t)val << 32;
> + /* The documentatoin states that the value of this register
"documentation".
Multiline comments should have the /* on a line of its own.
> + * "Forms bits [63:32] of the start (and end) address
> + * of the address region to be translated.
> + * Note that from version 406A there is a sperate
> + * register fot the upper end address
> + */
> + viewport->limit &= 0x00000000FFFFFFFFULL;
> + viewport->limit |= (uint64_t)val << 32;
> break;
Better to calculate the effective limit address when we need it,
rather than when the register is written. It's not a very
expensive calculation.
> diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
> index 908f3d946b..51052683b7 100644
> --- a/include/hw/pci-host/designware.h
> +++ b/include/hw/pci-host/designware.h
> @@ -41,7 +41,7 @@ typedef struct DesignwarePCIEViewport {
>
> uint64_t base;
> uint64_t target;
> - uint32_t limit;
> + uint64_t limit;
> uint32_t cr[2];
Making this field 64 bits makes the code to read and write
the register more complicated, and introduces a migration
compat break that we need to deal with. Why bother?
You can fix the problem that you're trying to solve in
the way that I suggested to you without introducing this
extra complication to this patch.
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-22 9:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-21 16:47 [PATCH v3] Handle wrap around in limit calculation Shlomo Pongratz
2024-01-21 23:17 ` Philippe Mathieu-Daudé
2024-01-22 7:14 ` Peter Xu
2024-01-22 8:37 ` Shlomo Pongratz
2024-01-22 9:01 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).