[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: GICv3: Aarch32: Need guidance on the atomic access of "union host_lpi" or if ITS is supported on R52


  • To: Andre Przywara <andre.przywara@xxxxxxx>, Julien Grall <julien@xxxxxxx>
  • From: Ayan Kumar Halder <ayankuma@xxxxxxx>
  • Date: Fri, 28 Oct 2022 12:44:08 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=W9Mldf5qUAwvWWbpAsoDm+xZrDAm0kaUATQDqGU47wc=; b=Q91rw6YMAGv6En1ar5HpQLFnORw2T9NUULX4PjnIri83Zmn/27hYkNk93zT0HWaBR3d03EV7VGxvGvysDbGD6LNYhB6ykX8GbK+Es/A8oxIzkzVcOrMqg0RgHSuYlEgrIlxXRzxCMqGZjzTosLwirYJTwvybZ/li/YKwkCl5FBBgK/C2ewQB2O7M+K4fxr6UmUtqOIKe81xoGG15RZRrbeT+5KoaGoyRgdSABNTUWHRpMJzpoVUOs+HatbiUC4UvokmEq/kj4a19O7kPuRmArOfgAuF2zAF49fZ4m2QGdmBkixgIYjGUDn4iLgUKRaI+EgtysGGioa2OkoxlMoPuBg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gaRgPiwbbZHFII0cYsJaIQEtk4g5HfA62g3oZtk79eYoEvFmHa4obLX9StkMnbBt6F98fts3Y/Zqu2sbIJe4CKj5RvDhS1fEL5WvDGSfE2hhsp7vZfJr0A/tgAP/lZ6O4orJy8Fd48oeTyvqUJ6wT9gfpUQ+xQgxn++nZYeWAowuex7Db9bOzpT8mhxc8iiwC9Lq/qgm/9I3fYbk4U6yidLSk4QnESlOzvOiUwfJD2yePQ6gFnCjynZff4Klv0SS02UMs4VmWRgtWffLOOCoVOdzKnzchkGkYKk6dMIABMG0dDZzgf+SFh0yDm/gdFizl7zfHVHXIAmTbQCLTBJAeQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: "stefanos@xxxxxxxxxx" <stefanos@xxxxxxxxxx>, bertrand Marquis <Bertrand.Marquis@xxxxxxx>, "Volodymyr_Babchuk@xxxxxxxx" <Volodymyr_Babchuk@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 28 Oct 2022 11:44:22 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 27/10/2022 15:36, Andre Przywara wrote:
On Thu, 27 Oct 2022 14:38:52 +0100
Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:

Hi Ayan,
Hi Andre / Julien,

On 25/10/2022 14:55, Andre Przywara wrote:
On Tue, 25 Oct 2022 13:25:52 +0100
Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:

Hi,
Hi Andre,

Many thanks for the explanation.

I need a clarification on the issue of atomic access to 64bit normal
memory on R52.

Hi Andre/All,

This came up while porting Xen on R52.

Refer "ARM DDI 0568A.cID110520", B1.3.1

"A Generic Interrupt Controller (GIC) implemented with an Armv8-R PE
must not implement LPI support. "

Does this mean ITS is not supported on R52 ? I am assuming yes, please
correct me if mistaken.
An ITS relies on LPIs, so yes: no ITS on a v8-R32 system. I cannot find
this restriction anymore in the v8-R64 supplement, so it would only apply
to the R52/AArch32.
If the answer is no, then my next query is follows :-
Answering to that anyway ...
diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
index 9ca74bc321..dea60aac0d 100644
--- a/xen/arch/arm/gic-v3-lpi.c
+++ b/xen/arch/arm/gic-v3-lpi.c
@@ -423,7 +423,7 @@ int gicv3_lpi_init_host_lpis(unsigned int host_lpi_bits)
        int rc;

        /* We rely on the data structure being atomically accessible. */
-    BUILD_BUG_ON(sizeof(union host_lpi) > sizeof(unsigned long));
+    BUILD_BUG_ON(sizeof(union host_lpi) > sizeof(uint64_t));

"unsigned long" on Aarch32 is 32 bits. So this bug gets triggered.

Is it fine to change it as above ?

Reading"ARM DDI 0487G.bID07202", E2.2.1, "Requirements for single-copy
atomicity".

"LDRD and STRD accesses to 64-bit aligned locations are 64-bit
single-copy atomic as seen by translation table walks and accesses to
translation tables"
This (and the explaining paragraph) indeed suggests that this should
work architecturally, if you use normal system memory (where you would
hold page tables). It would be confined to ARMv8 AArch32 and ARMv7 w/
LPAE, which matches Xen's requirements.
Does it mean that ldrd/strd will not work atomically on AArch32-v8R as
it uses MPU (not MMU, so no page tables) ?
No, this mentioning of page tables is more an example or a rationale, than
a requirement.
What this means (in the ARMv7-A/ARMv8-A AArch32 context) it:
Because on v7A-LPAE and v8-AArch32 PTEs are 64 bits wide, it's too painful
to use explicit locking to make sure just writing one PTE is atomic. So
the architecture demands that 64-bit aligned accesses using ldrd/strd
are single-copy atomic, so software can update just one PTE easily. But
this is only required for locations where page tables typically reside, so
system memory. This avoids this 64-bit atomicity requirement for *every*
part of the system, for instance separate buses, SRAM or flash on smaller
buses, or MMIO in general.

I don't find anything in the v8-R32 supplement that would step back from
this requirement, although indeed the original reason (atomic PTE writes)
would not apply to v8-R32. Both the LDRD/STRD description and the section
listing differences in the system memory architecture do not mention
anything, so I'd say that the ldrd atomicity requirement still holds.

Please note that this only applies to ARMv7 *LPAE* systems, but Xen
requires LPAE, and R52 is v8, so we are good, and the Xen code can rely on
this.

So for Xen on ARMv8-R32:
*LDRD/STRD* accesses to *64-bit aligned* addresses in *RAM* would be
atomic. You need to satisfy all three requirements:
- You must use ldrd/strd. Just dereferencing a uint64_t pointer in C does
not guarantee that, but read_atomic()/write_atomic() does.
- It must be 64-bit aligned. Shouldn't be a problem if the data type is
64 bits wide. Please note the slight nastiness that ldrd would silently
work on non-aligned addresses, but would lose the atomicity guarantee.
ldrexd would always fault if the address is not aligned.
We might want to check the alignment of data we access (assert?), if not
done already.
- It must be in system RAM, so not MMIO. Also I think TCM might be a
different story, but I would hope Xen would not use that directly.

Many thanks for the nice explanation.

I am trying to compare this with the atomicity requirement for AArch64 (ARM DDI 0487G.b ID072021, B2.2.1 Requirements for single-copy atomicit )

I seethat the alignment requirement is the same as for ARMv8-R32.

"-A read that is generated by a load instruction that loads a single general-purpose register and is aligned to the size of the read in the instruction is single-copy atomic.

-A write that is generated by a store instruction that stores a single general-purpose register and is aligned to the size of the write in the instruction is single-copy atomic"

I think the following code change should help us to confirm the correct behavior of atomic read/write on both AArch64 and AArch32 (including R52).

diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h
index ac2798d095..f22c65a853 100644
--- a/xen/arch/arm/include/asm/atomic.h
+++ b/xen/arch/arm/include/asm/atomic.h
@@ -78,6 +78,7 @@ static always_inline void read_atomic_size(const volatile void *p,
                                            void *res,
                                            unsigned int size)
 {
+    ASSERT(IS_ALIGNED((unsigned long)p, size));
     switch ( size )
     {
     case 1:
@@ -102,6 +103,7 @@ static always_inline void write_atomic_size(volatile void *p,
                                             void *val,
                                             unsigned int size)
 {
+    ASSERT(IS_ALIGNED((unsigned long)p, size));
     switch ( size )
     {
     case 1:

Please let me know if I misunderstand something.

- Ayan

If so, then is using ldrexd/strexd the solution for this ?
As mentioned above, you would not need that, just
read_atomic()/write_atomic() would do.

Hope that clears that up.

Cheers,
Andre

P.S. This above is my reading of the ARM ARM and the R32 supplement. I can
double check with the architects, but this might take a while.

IIUC "Memory accesses caused by LDREXD and STREXD instructions to
doubleword-aligned locations.", then the answer seems yes.

- Ayan

But it's only atomic if you are using ldrd/strd, which you cannot know for
sure in C, because it's up to the compiler to generate the instructions.

This is why we have that test. Changing the unsigned long to uint64_t
would make the check pointless, since the data structure is 64-bits long,
so it would always be true.

So given that you don't seem to need it, right now, it would leave the
test alone.

If you need that on AArch32 anyway, you would need to replace accesses to
the host_lpis array with inline assembly accessors, to ensure ldrd/strd
instructions. This seems doable (there are only so many places which
directly access the array members), but would need a good use case.

Cheers,
Andre
Does this imply that atomicity will be retained (with the above change)
? Os will this require ldrexd/strexd as R52 supports MPU (not MMU, so
translation tables are irrelevant).
Kind regards,
Ayan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.