From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 07DC9C433B4 for ; Thu, 29 Apr 2021 10:32:15 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7BADF61412 for ; Thu, 29 Apr 2021 10:32:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7BADF61412 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=tklengyel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.119867.226623 (Exim 4.92) (envelope-from ) id 1lc3xO-0002Qh-LE; Thu, 29 Apr 2021 10:31:42 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 119867.226623; Thu, 29 Apr 2021 10:31:42 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lc3xO-0002Qa-I9; Thu, 29 Apr 2021 10:31:42 +0000 Received: by outflank-mailman (input) for mailman id 119867; Thu, 29 Apr 2021 10:31:41 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1lc3xN-0002QV-QA for xen-devel@lists.xenproject.org; Thu, 29 Apr 2021 10:31:41 +0000 Received: from MTA-10-1.privateemail.com (unknown [68.65.122.30]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id fdbc17f1-1c6a-44d7-863d-9666b9c54a64; Thu, 29 Apr 2021 10:31:40 +0000 (UTC) Received: from MTA-10.privateemail.com (localhost [127.0.0.1]) by MTA-10.privateemail.com (Postfix) with ESMTP id 2E4FD60062 for ; Thu, 29 Apr 2021 06:31:39 -0400 (EDT) Received: from mail-wr1-f51.google.com (unknown [10.20.151.248]) by MTA-10.privateemail.com (Postfix) with ESMTPA id C2CB460059 for ; Thu, 29 Apr 2021 06:31:38 -0400 (EDT) Received: by mail-wr1-f51.google.com with SMTP id d11so8495248wrw.8 for ; Thu, 29 Apr 2021 03:31:38 -0700 (PDT) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: fdbc17f1-1c6a-44d7-863d-9666b9c54a64 X-Gm-Message-State: AOAM5331Mu3Qc7SN8yJBYOz0fRzuwvBzzjjHM8Y/F5JK30zAZUfiguoj OZZDuZneLXU7sHmL+xhqkm8yJ0muFz8DvsHY02c= X-Google-Smtp-Source: ABdhPJyeLhKkV+sStlHt/t5xZfevZ8rJ6uEhlgTphBIygXdqeQrDAbRsPriDdAL0pLnu/lDR+6g0BTXcOrtubwvFG/c= X-Received: by 2002:adf:8b02:: with SMTP id n2mr41127675wra.259.1619692297465; Thu, 29 Apr 2021 03:31:37 -0700 (PDT) MIME-Version: 1.0 References: <20210427093546.30703-1-michal.orzel@arm.com> <20210427093546.30703-11-michal.orzel@arm.com> <0c90579b-4861-8f90-2978-9e7f9015fae3@arm.com> In-Reply-To: <0c90579b-4861-8f90-2978-9e7f9015fae3@arm.com> From: Tamas K Lengyel Date: Thu, 29 Apr 2021 06:31:25 -0400 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 10/10] arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t To: Michal Orzel Cc: Julien Grall , Xen-devel , Stefano Stabellini , Volodymyr Babchuk , Alexandru Isaila , Petre Pircalabu , bertrand.marquis@arm.com Content-Type: multipart/alternative; boundary="00000000000009331e05c11a00de" X-Virus-Scanned: ClamAV using ClamSMTP --00000000000009331e05c11a00de Content-Type: text/plain; charset="UTF-8" On Thu, Apr 29, 2021, 4:53 AM Michal Orzel wrote: > Hi Julien, > > On 27.04.2021 13:09, Julien Grall wrote: > > Hi Michal, > > > > On 27/04/2021 10:35, Michal Orzel wrote: > >> AArch64 registers are 64bit whereas AArch32 registers > >> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus > >> we should get rid of helpers READ/WRITE_SYSREG32 > >> in favour of using READ/WRITE_SYSREG. > >> We should also use register_t type when reading sysregs > >> which can correspond to uint64_t or uint32_t. > >> Even though many AArch64 registers have upper 32bit reserved > >> it does not mean that they can't be widen in the future. > >> > >> Modify type of hsr, cpsr, spsr_el1 to uint64_t. > > > > As I pointed out in v1, the access to SPSR_EL1 has been quite fragile > because we relied on the padding afterwards. I think this was ought to be > explain in the commit message because it explain why the access in the > assembly code is not modified. > > > How about: > " > Modify type of hsr, cpsr, spsr_el1 to uint64_t. > Previously we relied on the padding after SPSR_EL1. As we removed the > padding, modify the union to be 64bit > so we don't corrupt SPSR_FIQ. > No need to modify the assembly code becuase the accesses were based on > 64bit registers as there was a 32bit padding after SPSR_EL1. > " > >> > >> Add 32bit RES0 members to structures inside hsr union. > >> > >> Remove 32bit padding in cpu_user_regs before spsr_fiq > >> as it is no longer needed due to upper union being 64bit now. > >> > >> Add 64bit padding in cpu_user_regs before spsr_el1 > >> because offset of spsr_el1 must be a multiple of 8. > >> > >> Signed-off-by: Michal Orzel > >> diff --git a/xen/include/asm-arm/hsr.h b/xen/include/asm-arm/hsr.h > >> index 29d4531f40..fb4a3b1274 100644 > >> --- a/xen/include/asm-arm/hsr.h > >> +++ b/xen/include/asm-arm/hsr.h > >> @@ -16,11 +16,12 @@ enum dabt_size { > >> }; > >> union hsr { > >> - uint32_t bits; > >> + register_t bits; > >> struct { > >> unsigned long iss:25; /* Instruction Specific Syndrome */ > >> unsigned long len:1; /* Instruction length */ > >> unsigned long ec:6; /* Exception Class */ > >> + unsigned long _res0:32; > > > > Sorry I wasn't clear in my original comment, what I meant I would rather > not add this field (and also the _res0) because they are not strictly > necessary. > > > Ok I'll remove _res0 members. But bits can be of type register_t, right? > >> diff --git a/xen/include/public/arch-arm.h > b/xen/include/public/arch-arm.h > >> index 713fd65317..c49bce2983 100644 > >> --- a/xen/include/public/arch-arm.h > >> +++ b/xen/include/public/arch-arm.h > >> @@ -267,10 +267,10 @@ struct vcpu_guest_core_regs > >> /* Return address and mode */ > >> __DECL_REG(pc64, pc32); /* ELR_EL2 */ > >> - uint32_t cpsr; /* SPSR_EL2 */ > >> + register_t cpsr; /* SPSR_EL2 */ > > > > You can't use register_t here because this is a public header (we don't > export register_t) and the header should be bitness agnostic. > > > > Also, because this is a public header, you ought to explain why breaking > the ABI is fine. > > > > In this case, this is an ABI between the tools and this is not stable. > However, we would still need to bump XEN_DOMCTL_INTERFACE_VERSION as I > think this wasn't done for this development cycle. > > > > Of course, this will also need a suitable mention in the commit message > (I can help with that). > > > Ok so I'll increment XEN_DOMCTL_INTERFACE_VERSION and write in commit msg: > " > Change type of cpsr to uint64_t in the public outside interface > "public/arch-arm.h" to allow ABI compatibility between 32bit and 64bit. > Increment XEN_DOMCTL_INTERFACE_VERSION. > " > >> union { > >> - uint32_t spsr_el1; /* AArch64 */ > >> + uint64_t spsr_el1; /* AArch64 */ > >> uint32_t spsr_svc; /* AArch32 */ > >> }; > >> diff --git a/xen/include/public/vm_event.h > b/xen/include/public/vm_event.h > >> index 36135ba4f1..ad3d141fe8 100644 > >> --- a/xen/include/public/vm_event.h > >> +++ b/xen/include/public/vm_event.h > >> @@ -266,8 +266,12 @@ struct vm_event_regs_arm { > >> uint64_t ttbr1; > >> uint64_t ttbcr; > >> uint64_t pc; > >> +#ifdef CONFIG_ARM_32 > >> uint32_t cpsr; > >> uint32_t _pad; > >> +#else > >> + uint64_t cpsr; > >> +#endif > > > > CONFIG_ARM_32 is not defined for public header. They also should be > bitness agnostic. So cpsr should always be uint64_t. > > > > Also, similar to public/arch-arm.h, this is not a stable ABI but you > will need to bump VM_EVENT_INTERFACE_VERSION if this hasn't been done for > this development cycle. > > > Ok so I will change type of cpsr here to uint64_t, increment > VM_EVENT_INTERFACE_VERSION and write in commit msg: > " > Change type of cpsr to uint64_t in the public outside interface > "public/vm_event.h" to allow ABI compatibility between 32bit and 64bit. > Increment VM_EVENT_INTERFACE_VERSION. > " > Ok? > There is no need to bump the interface version for this, you are not changing the layout or size of the structure since there was already 64bit space there for cspr for both 32bit and 64bit builds. You are just folding that padding field into cspr on 32bit builds. Thanks, Tamas > --00000000000009331e05c11a00de Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Thu, Apr 29, 2021, 4:53 AM Michal Orzel <michal.orzel@arm.com> wrote:
Hi Julien,

On 27.04.2021 13:09, Julien Grall wrote:
> Hi Michal,
>
> On 27/04/2021 10:35, Michal Orzel wrote:
>> AArch64 registers are 64bit whereas AArch32 registers
>> are 32bit or 64bit. MSR/MRS are expecting 64bit values thus
>> we should get rid of helpers READ/WRITE_SYSREG32
>> in favour of using READ/WRITE_SYSREG.
>> We should also use register_t type when reading sysregs
>> which can correspond to uint64_t or uint32_t.
>> Even though many AArch64 registers have upper 32bit reserved
>> it does not mean that they can't be widen in the future.
>>
>> Modify type of hsr, cpsr, spsr_el1 to uint64_t.
>
> As I pointed out in v1, the access to SPSR_EL1 has been quite fragile = because we relied on the padding afterwards. I think this was ought to be e= xplain in the commit message because it explain why the access in the assem= bly code is not modified.
>
How about:
"
Modify type of hsr, cpsr, spsr_el1 to uint64_t.
Previously we relied on the padding after SPSR_EL1. As we removed the paddi= ng, modify the union to be 64bit
so we don't corrupt SPSR_FIQ.
No need to modify the assembly code becuase the accesses were based on 64bi= t registers as there was a 32bit padding after SPSR_EL1.
"
>>
>> Add 32bit RES0 members to structures inside hsr union.
>>
>> Remove 32bit padding in cpu_user_regs before spsr_fiq
>> as it is no longer needed due to upper union being 64bit now.
>>
>> Add 64bit padding in cpu_user_regs before spsr_el1
>> because offset of spsr_el1 must be a multiple of 8.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> diff --git a/xen/include/asm-arm/hsr.h b/xen/include/asm-arm/hsr.h=
>> index 29d4531f40..fb4a3b1274 100644
>> --- a/xen/include/asm-arm/hsr.h
>> +++ b/xen/include/asm-arm/hsr.h
>> @@ -16,11 +16,12 @@ enum dabt_size {
>> =C2=A0 };
>> =C2=A0 =C2=A0 union hsr {
>> -=C2=A0=C2=A0=C2=A0 uint32_t bits;
>> +=C2=A0=C2=A0=C2=A0 register_t bits;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned lo= ng iss:25;=C2=A0 /* Instruction Specific Syndrome */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned lo= ng len:1;=C2=A0=C2=A0 /* Instruction length */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned lo= ng ec:6;=C2=A0=C2=A0=C2=A0 /* Exception Class */
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long _res0:32= ;
>
> Sorry I wasn't clear in my original comment, what I meant I would = rather not add this field (and also the _res0) because they are not strictl= y necessary.
>
Ok I'll remove _res0 members. But bits can be of type register_t, right= ?
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/ar= ch-arm.h
>> index 713fd65317..c49bce2983 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -267,10 +267,10 @@ struct vcpu_guest_core_regs
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Return address and mode *= /
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __DECL_REG(pc64,=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pc32);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* ELR_EL2 */
>> -=C2=A0=C2=A0=C2=A0 uint32_t cpsr;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* SP= SR_EL2 */
>> +=C2=A0=C2=A0=C2=A0 register_t cpsr;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* SPSR_EL2 */ >
> You can't use register_t here because this is a public header (we = don't export register_t) and the header should be bitness agnostic.
>
> Also, because this is a public header, you ought to explain why breaki= ng the ABI is fine.
>
> In this case, this is an ABI between the tools and this is not stable.= However, we would still need to bump XEN_DOMCTL_INTERFACE_VERSION as I thi= nk this wasn't done for this development cycle.
>
> Of course, this will also need a suitable mention in the commit messag= e (I can help with that).
>
Ok so I'll increment XEN_DOMCTL_INTERFACE_VERSION and write in commit m= sg:
"
Change type of cpsr to uint64_t in the public outside interface "publi= c/arch-arm.h" to allow ABI compatibility between 32bit and 64bit.
Increment XEN_DOMCTL_INTERFACE_VERSION.
"
>> =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 union {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint32_t spsr_el1;=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* AArch64 */
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint64_t spsr_el1;=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* AArch64 */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint32_t sp= sr_svc;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* AArch32 */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
>> =C2=A0 diff --git a/xen/include/public/vm_event.h b/xen/include/pu= blic/vm_event.h
>> index 36135ba4f1..ad3d141fe8 100644
>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -266,8 +266,12 @@ struct vm_event_regs_arm {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint64_t ttbr1;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint64_t ttbcr;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint64_t pc;
>> +#ifdef CONFIG_ARM_32
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint32_t cpsr;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 uint32_t _pad;
>> +#else
>> +=C2=A0=C2=A0=C2=A0 uint64_t cpsr;
>> +#endif
>
> CONFIG_ARM_32 is not defined for public header. They also should be bi= tness agnostic. So cpsr should always be uint64_t.
>
> Also, similar to public/arch-arm.h, this is not a stable ABI but you w= ill need to bump VM_EVENT_INTERFACE_VERSION if this hasn't been done fo= r this development cycle.
>
Ok so I will change type of cpsr here to uint64_t, increment VM_EVENT_INTER= FACE_VERSION and write in commit msg:
"
Change type of cpsr to uint64_t in the public outside interface "publi= c/vm_event.h" to allow ABI compatibility between 32bit and 64bit.
Increment VM_EVENT_INTERFACE_VERSION.
"
Ok?

There is no need to bump the interface version for this, you are not cha= nging the layout or size of the structure since there was already 64bit spa= ce there for cspr for both 32bit and 64bit builds. You are just folding tha= t padding field into cspr on 32bit builds.

Thanks,
Tamas
--00000000000009331e05c11a00de--