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

Re: [RFC PATCH v1 03/12] Arm: GICv3: Enable vreg_reg64_* macros for AArch32


  • To: Julien Grall <julien@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Ayan Kumar Halder <ayankuma@xxxxxxx>
  • Date: Mon, 24 Oct 2022 11:47:40 +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=iNnGKL9Sez5Pv6RkaHdYhArAypCrVURdZJKj/LfAKok=; b=Py8OfBHRooUv9rfcqecJiXzfWWShrtlUpo+3HtBIcxR5IUhEiMYq48/agbrr2R6KVBqKCxD1212nJ+3hasJR9HJN9jjTFzSGgURP/feHEHnU0Yg44b+leHEkBntWxweID7pKrTwD11La02Go1G8IIs5LJQFNRokkw6ViZbFtticLDmU4rGeIdMjbnBIMWqvOggT5QJE/GnkYTzT1AfcyCgmZf9EOq3+nerhVzrag+HfH1p2jvzzH5uuBxC68/LfEOmIihl7QVtHUGRUleKv9+vBUZEXYlaVtFf41XTP7JGYfgu0SlE1hA2AAMn1rxTDOZhHJmCEIK6lS/IngqLv2TQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XKPK18TL43YTznMAZ0hpTLEBehJhD3PstFimnsvV99YguVcI0B63r2pSNKYiPaHvYrdw4/Uj0flii+VEAdVB07M6QoZxYa1Uj+2Si4AMoKh81KuCS9J5oiVtx6wCKzv008aWC0oN2RJohlvJx6B/wl09vhbTecwTotUus5faTkoilCqS3bxg5KDuOjOIf7trSDWOJQO0ZzFl4lsyy8r4Wj9RdFG6JVfjf+C8yCZNygCJ0IWpzYlEFth1BVPxVv5tI/kLT2Vlx9eL8/u7LcJxt6kkTQEgaL/czkc1+fIrvbQe/T91NU1rgTUxLO4O5bY5UdIm0jnv7Gvue6yCT6m4iw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: sstabellini@xxxxxxxxxx, stefanos@xxxxxxxxxx, Volodymyr_Babchuk@xxxxxxxx, bertrand.marquis@xxxxxxx
  • Delivery-date: Mon, 24 Oct 2022 10:47:53 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 22/10/2022 11:13, Julien Grall wrote:
Hi Ayan,

Hi Julien,

I need some clarification.


Title: The code you are modifying below is not GICv3 specific. I would suggest the following title:

xen/arm: vreg: Support vreg_reg64_* helpers on Aarch32

On 21/10/2022 16:31, Ayan Kumar Halder wrote:
In some situations (eg GICR_TYPER), the hypervior may need to emulate
64bit registers in aarch32 mode. In such situations, the hypervisor may
need to read/modify the lower or upper 32 bits of the 64 bit register.

In aarch32, 64 bit is represented by unsigned long long. Thus, we need
to change the prototype accordingly.

Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
---
  xen/arch/arm/include/asm/vreg.h | 23 ++++++++---------------
  1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/include/asm/vreg.h b/xen/arch/arm/include/asm/vreg.h
index f26a70d024..ac6e702c5c 100644
--- a/xen/arch/arm/include/asm/vreg.h
+++ b/xen/arch/arm/include/asm/vreg.h
@@ -95,7 +95,7 @@ static inline bool vreg_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr
   * Note that the alignment fault will always be taken in the guest
   * (see B3.12.7 DDI0406.b).
   */
-static inline register_t vreg_reg_extract(unsigned long reg,
+static inline register_t vreg_reg_extract(unsigned long long reg,
                                            unsigned int offset,
                                            enum dabt_size size)
  {
@@ -105,7 +105,7 @@ static inline register_t vreg_reg_extract(unsigned long reg,
      return reg;
  }
  -static inline void vreg_reg_update(unsigned long *reg, register_t val, +static inline void vreg_reg_update(unsigned long long *reg, register_t val,
                                     unsigned int offset,
                                     enum dabt_size size)
  {
@@ -116,7 +116,7 @@ static inline void vreg_reg_update(unsigned long *reg, register_t val,
      *reg |= ((unsigned long)val & mask) << shift;
  }
  -static inline void vreg_reg_setbits(unsigned long *reg, register_t bits, +static inline void vreg_reg_setbits(unsigned long long *reg, register_t bits,
                                      unsigned int offset,
                                      enum dabt_size size)
  {
@@ -126,7 +126,7 @@ static inline void vreg_reg_setbits(unsigned long *reg, register_t bits,
      *reg |= ((unsigned long)bits & mask) << shift;
  }
  -static inline void vreg_reg_clearbits(unsigned long *reg, register_t bits, +static inline void vreg_reg_clearbits(unsigned long long *reg, register_t bits,
                                        unsigned int offset,
                                        enum dabt_size size)
  {
@@ -149,7 +149,7 @@ static inline void vreg_reg##sz##_update(uint##sz##_t *reg,             \                                            register_t val,                \                                            const mmio_info_t *info)       \
{ \
-    unsigned long tmp = *reg;                                           \ +    unsigned long long tmp = *reg;                                      \
\
      vreg_reg_update(&tmp, val, info->gpa & (offmask),                   \
info->dabt.size);                                   \
@@ -161,7 +161,7 @@ static inline void vreg_reg##sz##_setbits(uint##sz##_t *reg,            \                                             register_t bits,              \                                             const mmio_info_t *info)      \
{ \
-    unsigned long tmp = *reg;                                           \ +    unsigned long long tmp = *reg;                                      \
\
      vreg_reg_setbits(&tmp, bits, info->gpa & (offmask),                 \
info->dabt.size);                                  \
@@ -173,7 +173,7 @@ static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg,          \                                               register_t bits,            \                                               const mmio_info_t *info)    \
{ \
-    unsigned long tmp = *reg;                                           \ +    unsigned long long tmp = *reg;                                      \
\
      vreg_reg_clearbits(&tmp, bits, info->gpa & (offmask),               \
info->dabt.size);                                \
@@ -181,15 +181,8 @@ static inline void vreg_reg##sz##_clearbits(uint##sz##_t *reg,          \       *reg = tmp;                                                         \
  }
  -/*
- * 64 bits registers are only supported on platform with 64-bit long.
- * This is also allow us to optimize the 32 bit case by using
- * unsigned long rather than uint64_t
- */

The comment above explain why we never use uint64_t in the helpers above. IIRC, the compiler would end up to use 2 registers on AArch32 even for the vreg_reg32_* helpers. I wanted to avoid that and would like like to today. Can you check the code generated?

I am not sure I understood the comment very well.

With this patch, the disassembly is as follows :-

        vreg_reg32_update(&v->domain->arch.vgic.ctlr, r, info);
  28124c:   e597000c    ldr r0, [r7, #12]
VREG_REG_HELPERS(32, 0x3);
  281250:   e5d52002    ldrb    r2, [r5, #2]
  281254:   e1a02322    lsr r2, r2, #6
    unsigned long mask = VREG_REG_MASK(size);
  281258:   e3a03008    mov r3, #8
  28125c:   e1a03213    lsl r3, r3, r2
  281260:   e2633020    rsb r3, r3, #32
  281264:   e3e02000    mvn r2, #0
  281268:   e1a02332    lsr r2, r2, r3
VREG_REG_HELPERS(32, 0x3);
  28126c:   e5953010    ldr r3, [r5, #16]
  281270:   e2033003    and r3, r3, #3
    int shift = offset * 8;
  281274:   e1a03183    lsl r3, r3, #3
VREG_REG_HELPERS(32, 0x3);
  281278:   e59013f0    ldr r1, [r0, #1008] ; 0x3f0
  28127c:   e1c11312    bic r1, r1, r2, lsl r3
    *reg |= ((unsigned long)val & mask) << shift;
  281280:   e0022009    and r2, r2, r9
VREG_REG_HELPERS(32, 0x3);
  281284:   e1813312    orr r3, r1, r2, lsl r3
  281288:   e58033f0    str r3, [r0, #1008] ; 0x3f0
        v->domain->arch.vgic.ctlr &= GICD_CTL_ENABLE;
  28128c:   e597200c    ldr r2, [r7, #12]
  281290:   e59233f0    ldr r3, [r2, #1008] ; 0x3f0
  281294:   e2033001    and r3, r3, #1
  281298:   e58233f0    str r3, [r2, #1008] ; 0x3f0

Without the patch (ie original code) , the disassembly is :-

        vreg_reg32_update(&v->domain->arch.vgic.ctlr, r, info);
  27f8f4:   e597000c    ldr r0, [r7, #12]
VREG_REG_HELPERS(32, 0x3);
  27f8f8:   e5d52002    ldrb    r2, [r5, #2]
  27f8fc:   e1a02322    lsr r2, r2, #6
    unsigned long mask = VREG_REG_MASK(size);
  27f900:   e3a03008    mov r3, #8
  27f904:   e1a03213    lsl r3, r3, r2
  27f908:   e2633020    rsb r3, r3, #32
  27f90c:   e3e02000    mvn r2, #0
  27f910:   e1a02332    lsr r2, r2, r3
VREG_REG_HELPERS(32, 0x3);
  27f914:   e5953010    ldr r3, [r5, #16]
  27f918:   e2033003    and r3, r3, #3
    int shift = offset * 8;
  27f91c:   e1a03183    lsl r3, r3, #3
    *reg &= ~(mask << shift);
  27f920:   e5901400    ldr r1, [r0, #1024] ; 0x400
  27f924:   e1c11312    bic r1, r1, r2, lsl r3
    *reg |= ((unsigned long)val & mask) << shift;
  27f928:   e0022009    and r2, r2, r9
  27f92c:   e1813312    orr r3, r1, r2, lsl r3
VREG_REG_HELPERS(32, 0x3);
  27f930:   e5803400    str r3, [r0, #1024] ; 0x400
        v->domain->arch.vgic.ctlr &= GICD_CTL_ENABLE;
  27f934:   e597200c    ldr r2, [r7, #12]
  27f938:   e5923400    ldr r3, [r2, #1024] ; 0x400
  27f93c:   e2033001    and r3, r3, #1
  27f940:   e5823400    str r3, [r2, #1024] ; 0x400

Sorry, I can't spot the difference. :(

I had a look at commit 423e9ecdc26c4b40c8db1fcc63b3655463c29976 which introduced this. But I could not make out the reason from the commit message.


For other options, I would consider to either:
  1) Fold vreg_reg_* in the macros.

Can you explain this option a bit ?

- Ayan

  2) Write a separate vreg_reg64_*

My preference would be 1).

If we are planning to keep the code with "unsigned long long", then I think this should be addressed in the commit message.

-#if BITS_PER_LONG == 64
-VREG_REG_HELPERS(64, 0x7);
-#endif
  VREG_REG_HELPERS(32, 0x3);
+VREG_REG_HELPERS(64, 0x7);

Regardless what I wrote above, the code movement seems to be unwarranted.
    #undef VREG_REG_HELPERS

Cheers,




 


Rackspace

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