[kvm-unit-tests PATCH 0/2] arm64: fix paging issues

[kvm-unit-tests PATCH 0/2] arm64: fix paging issues

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Two more arm64 fixes, besides the vtimer ISTATUS fix that I recently
sent.

The first patch fixes an actual issue I encountered on my setup and
caused tests to crash.

The second fix is a theoretical one, which I found while reading the
code.

Nadav Amit (2):
  arm64: set sctlr_el1.SPAN
  arm64: ensure tlbi is safe

 arm/cstart64.S         | 1 +
 lib/arm64/asm/mmu.h    | 4 ++--
 lib/arm64/asm/sysreg.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.

Without setting sctlr_el1.SPAN, tests crash when they access the memory
after an exception.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arm/cstart64.S         | 1 +
 lib/arm64/asm/sysreg.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arm/cstart64.S b/arm/cstart64.S
index 61e27d3..d4cee6f 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -245,6 +245,7 @@ asm_mmu_enable:
 	orr	x1, x1, SCTLR_EL1_C
 	orr	x1, x1, SCTLR_EL1_I
 	orr	x1, x1, SCTLR_EL1_M
+	orr	x1, x1, SCTLR_EL1_SPAN
 	msr	sctlr_el1, x1
 	isb
 
diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
index 18c4ed3..b9868ff 100644
--- a/lib/arm64/asm/sysreg.h
+++ b/lib/arm64/asm/sysreg.h
@@ -81,6 +81,7 @@ asm(
 
 /* System Control Register (SCTLR_EL1) bits */
 #define SCTLR_EL1_EE	(1 << 25)
+#define SCTLR_EL1_SPAN	(1 << 23)
 #define SCTLR_EL1_WXN	(1 << 19)
 #define SCTLR_EL1_I	(1 << 12)
 #define SCTLR_EL1_SA0	(1 << 4)
-- 
2.34.1

[kvm-unit-tests PATCH 2/2] arm64: ensure tlbi is safe

[Nadav Amit]

From: Nadav Amit <namit@vmware.com>

While no real problem was encountered, having an inline assembly without
volatile keyword and output can allow the compiler to ignore it. And
without a memory clobber, potentially reorder it.

Add volatile and memory clobber.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 lib/arm64/asm/mmu.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/arm64/asm/mmu.h b/lib/arm64/asm/mmu.h
index 5c27edb..cf94403 100644
--- a/lib/arm64/asm/mmu.h
+++ b/lib/arm64/asm/mmu.h
@@ -14,7 +14,7 @@
 static inline void flush_tlb_all(void)
 {
 	dsb(ishst);
-	asm("tlbi	vmalle1is");
+	asm volatile("tlbi	vmalle1is" ::: "memory");
 	dsb(ish);
 	isb();
 }
@@ -23,7 +23,7 @@ static inline void flush_tlb_page(unsigned long vaddr)
 {
 	unsigned long page = vaddr >> 12;
 	dsb(ishst);
-	asm("tlbi	vaae1is, %0" :: "r" (page));
+	asm volatile("tlbi	vaae1is, %0" :: "r" (page) : "memory");
 	dsb(ish);
 	isb();
 }
-- 
2.34.1

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nadav Amit]

> On Jun 16, 2023, at 6:31 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
> From: Nadav Amit <namit@vmware.com>
> 
> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
> 
> Without setting sctlr_el1.SPAN, tests crash when they access the memory
> after an exception.

Andrew,

You’ve been kind enough to review the other patch-set and perhaps this
one fell through the cracks. Can you please have a look at this one
specifically and on:

https://lore.kernel.org/all/20230615003832.161134-1-namit@vmware.com/

These issues cause problem on other environments.

Thanks,
Nadav

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nadav Amit]

> 
> On Jun 16, 2023, at 6:31 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
> From: Nadav Amit <namit@vmware.com>
> 
> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
> 
> Without setting sctlr_el1.SPAN, tests crash when they access the memory
> after an exception.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
> arm/cstart64.S         | 1 +
> lib/arm64/asm/sysreg.h | 1 +
> 2 files changed, 2 insertions(+)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index 61e27d3..d4cee6f 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -245,6 +245,7 @@ asm_mmu_enable:
> 	orr	x1, x1, SCTLR_EL1_C
> 	orr	x1, x1, SCTLR_EL1_I
> 	orr	x1, x1, SCTLR_EL1_M
> +	orr	x1, x1, SCTLR_EL1_SPAN
> 	msr	sctlr_el1, x1
> 	isb
> 
> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
> index 18c4ed3..b9868ff 100644
> --- a/lib/arm64/asm/sysreg.h
> +++ b/lib/arm64/asm/sysreg.h
> @@ -81,6 +81,7 @@ asm(
> 
> /* System Control Register (SCTLR_EL1) bits */
> #define SCTLR_EL1_EE	(1 << 25)
> +#define SCTLR_EL1_SPAN	(1 << 23)
> #define SCTLR_EL1_WXN	(1 << 19)
> #define SCTLR_EL1_I	(1 << 12)
> #define SCTLR_EL1_SA0	(1 << 4)
> -- 
> 2.34.1
> 

Ping? (this one specifically is an issue)

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Alexandru Elisei]

Hi,

On Sat, Jun 17, 2023 at 01:31:37AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.

In arm/cstart64.S

.globl start
start:
        /* get our base address */
	[..]

1:
        /* zero BSS */
	[..]

        /* zero and set up stack */
	[..]

        /* set SCTLR_EL1 to a known value */
        ldr     x4, =INIT_SCTLR_EL1_MMU_OFF
	[..]

        /* set up exception handling */
        bl      exceptions_init
	[..]

Where in lib/arm64/asm/sysreg.h:

#define SCTLR_EL1_RES1  (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \
                         _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29))
#define INIT_SCTLR_EL1_MMU_OFF  \
                        SCTLR_EL1_RES1

Look like bit 23 (SPAN) should be set.

How are you seeing SCTLR_EL1.SPAN unset?

Thanks,
Alex

> 
> Without setting sctlr_el1.SPAN, tests crash when they access the memory
> after an exception.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  arm/cstart64.S         | 1 +
>  lib/arm64/asm/sysreg.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index 61e27d3..d4cee6f 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -245,6 +245,7 @@ asm_mmu_enable:
>  	orr	x1, x1, SCTLR_EL1_C
>  	orr	x1, x1, SCTLR_EL1_I
>  	orr	x1, x1, SCTLR_EL1_M
> +	orr	x1, x1, SCTLR_EL1_SPAN
>  	msr	sctlr_el1, x1
>  	isb
>  
> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
> index 18c4ed3..b9868ff 100644
> --- a/lib/arm64/asm/sysreg.h
> +++ b/lib/arm64/asm/sysreg.h
> @@ -81,6 +81,7 @@ asm(
>  
>  /* System Control Register (SCTLR_EL1) bits */
>  #define SCTLR_EL1_EE	(1 << 25)
> +#define SCTLR_EL1_SPAN	(1 << 23)
>  #define SCTLR_EL1_WXN	(1 << 19)
>  #define SCTLR_EL1_I	(1 << 12)
>  #define SCTLR_EL1_SA0	(1 << 4)
> -- 
> 2.34.1
> 
> 

Re: [kvm-unit-tests PATCH 2/2] arm64: ensure tlbi is safe

[Alexandru Elisei]

Hi,

On Sat, Jun 17, 2023 at 01:31:38AM +0000, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> 
> While no real problem was encountered, having an inline assembly without
> volatile keyword and output can allow the compiler to ignore it. And
> without a memory clobber, potentially reorder it.
> 
> Add volatile and memory clobber.
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  lib/arm64/asm/mmu.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/arm64/asm/mmu.h b/lib/arm64/asm/mmu.h
> index 5c27edb..cf94403 100644
> --- a/lib/arm64/asm/mmu.h
> +++ b/lib/arm64/asm/mmu.h
> @@ -14,7 +14,7 @@
>  static inline void flush_tlb_all(void)
>  {
>  	dsb(ishst);
> -	asm("tlbi	vmalle1is");

From the gas manual [1]:

"asm statements that have no output operands and asm goto statements, are
implicitly volatile."

Looks to me like both TLBIs fall into this category.

And I think the "memory" clobber is not needed because the dsb macro before and
after the TLBI already have it.

[1] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile

Thanks,
Alex

> +	asm volatile("tlbi	vmalle1is" ::: "memory");
>  	dsb(ish);
>  	isb();
>  }
> @@ -23,7 +23,7 @@ static inline void flush_tlb_page(unsigned long vaddr)
>  {
>  	unsigned long page = vaddr >> 12;
>  	dsb(ishst);
> -	asm("tlbi	vaae1is, %0" :: "r" (page));
> +	asm volatile("tlbi	vaae1is, %0" :: "r" (page) : "memory");
>  	dsb(ish);
>  	isb();
>  }
> -- 
> 2.34.1
> 
> 

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Shaoqin Huang]

Hi,

On 7/14/23 18:31, Alexandru Elisei wrote:
> Hi,
> 
> On Sat, Jun 17, 2023 at 01:31:37AM +0000, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>>
>> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
> 
> In arm/cstart64.S
> 
> .globl start
> start:
>          /* get our base address */
> 	[..]
> 
> 1:
>          /* zero BSS */
> 	[..]
> 
>          /* zero and set up stack */
> 	[..]
> 
>          /* set SCTLR_EL1 to a known value */
>          ldr     x4, =INIT_SCTLR_EL1_MMU_OFF
> 	[..]
> 
>          /* set up exception handling */
>          bl      exceptions_init
> 	[..]
> 
> Where in lib/arm64/asm/sysreg.h:
> 
> #define SCTLR_EL1_RES1  (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \
>                           _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29))
> #define INIT_SCTLR_EL1_MMU_OFF  \
>                          SCTLR_EL1_RES1
> 
> Look like bit 23 (SPAN) should be set.
> 
> How are you seeing SCTLR_EL1.SPAN unset?

Yeah. the sctlr_el1.SPAN has always been set by the above flow. So Nadav 
you can describe what you encounter with more details. Like which tests 
crash you encounter, and how to reproduce it.

Thanks,
Shaoqin

> 
> Thanks,
> Alex
> 
>>
>> Without setting sctlr_el1.SPAN, tests crash when they access the memory
>> after an exception.
>>
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> ---
>>   arm/cstart64.S         | 1 +
>>   lib/arm64/asm/sysreg.h | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index 61e27d3..d4cee6f 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -245,6 +245,7 @@ asm_mmu_enable:
>>   	orr	x1, x1, SCTLR_EL1_C
>>   	orr	x1, x1, SCTLR_EL1_I
>>   	orr	x1, x1, SCTLR_EL1_M
>> +	orr	x1, x1, SCTLR_EL1_SPAN
>>   	msr	sctlr_el1, x1
>>   	isb
>>   
>> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
>> index 18c4ed3..b9868ff 100644
>> --- a/lib/arm64/asm/sysreg.h
>> +++ b/lib/arm64/asm/sysreg.h
>> @@ -81,6 +81,7 @@ asm(
>>   
>>   /* System Control Register (SCTLR_EL1) bits */
>>   #define SCTLR_EL1_EE	(1 << 25)
>> +#define SCTLR_EL1_SPAN	(1 << 23)
>>   #define SCTLR_EL1_WXN	(1 << 19)
>>   #define SCTLR_EL1_I	(1 << 12)
>>   #define SCTLR_EL1_SA0	(1 << 4)
>> -- 
>> 2.34.1
>>
>>
> 

-- 
Shaoqin

Re: [kvm-unit-tests PATCH 2/2] arm64: ensure tlbi is safe

[Nadav Amit]

> On Jul 14, 2023, at 3:55 AM, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> !! External Email
> 
> Hi,
> 
> On Sat, Jun 17, 2023 at 01:31:38AM +0000, Nadav Amit wrote:
>> From: Nadav Amit <namit@vmware.com>
>> 
>> While no real problem was encountered, having an inline assembly without
>> volatile keyword and output can allow the compiler to ignore it. And
>> without a memory clobber, potentially reorder it.
>> 
>> Add volatile and memory clobber.
>> 
>> Signed-off-by: Nadav Amit <namit@vmware.com>
>> ---
>> lib/arm64/asm/mmu.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/lib/arm64/asm/mmu.h b/lib/arm64/asm/mmu.h
>> index 5c27edb..cf94403 100644
>> --- a/lib/arm64/asm/mmu.h
>> +++ b/lib/arm64/asm/mmu.h
>> @@ -14,7 +14,7 @@
>> static inline void flush_tlb_all(void)
>> {
>>      dsb(ishst);
>> -     asm("tlbi       vmalle1is");
> 
> From the gas manual [1]:
> 
> "asm statements that have no output operands and asm goto statements, are
> implicitly volatile."
> 
> Looks to me like both TLBIs fall into this category.
> 
> And I think the "memory" clobber is not needed because the dsb macro before and
> after the TLBI already have it.

You are completely correct. I forgot about the implicit “volatile”.

This one can be dropped.

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nadav Amit]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]



> On Jul 14, 2023, at 4:29 AM, Shaoqin Huang <shahuang@redhat.com> wrote:
> 
> !! External Email
> 
> Hi,
> 
> On 7/14/23 18:31, Alexandru Elisei wrote:
>> Hi,
>> 
>> On Sat, Jun 17, 2023 at 01:31:37AM +0000, Nadav Amit wrote:
>>> From: Nadav Amit <namit@vmware.com>
>>> 
>>> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
>> 
>> In arm/cstart64.S
>> 
>> .globl start
>> start:
>>         /* get our base address */
>>      [..]
>> 
>> 1:
>>         /* zero BSS */
>>      [..]
>> 
>>         /* zero and set up stack */
>>      [..]
>> 
>>         /* set SCTLR_EL1 to a known value */
>>         ldr     x4, =INIT_SCTLR_EL1_MMU_OFF
>>      [..]
>> 
>>         /* set up exception handling */
>>         bl      exceptions_init
>>      [..]
>> 
>> Where in lib/arm64/asm/sysreg.h:
>> 
>> #define SCTLR_EL1_RES1  (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \
>>                          _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29))
>> #define INIT_SCTLR_EL1_MMU_OFF  \
>>                         SCTLR_EL1_RES1
>> 
>> Look like bit 23 (SPAN) should be set.
>> 
>> How are you seeing SCTLR_EL1.SPAN unset?
> 
> Yeah. the sctlr_el1.SPAN has always been set by the above flow. So Nadav
> you can describe what you encounter with more details. Like which tests
> crash you encounter, and how to reproduce it.

I am using Nikos’s work to run the test using EFI, not from QEMU.

So the code that you mentioned - which is supposed to initialize SCTLR -
is not executed (and actually not part of the EFI image).

Note that using EFI, the entry point is _start [1], and not “start”.

That is also the reason lack of BSS zeroing also caused me issues with the
EFI setup, which I reported before.



[1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/blob/master/arm/efi/crt0-efi-aarch64.S#L113


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Andrew Jones]

On Fri, Jul 14, 2023 at 06:42:25PM +0000, Nadav Amit wrote:
> 
> 
> > On Jul 14, 2023, at 4:29 AM, Shaoqin Huang <shahuang@redhat.com> wrote:
> > 
> > !! External Email
> > 
> > Hi,
> > 
> > On 7/14/23 18:31, Alexandru Elisei wrote:
> >> Hi,
> >> 
> >> On Sat, Jun 17, 2023 at 01:31:37AM +0000, Nadav Amit wrote:
> >>> From: Nadav Amit <namit@vmware.com>
> >>> 
> >>> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
> >> 
> >> In arm/cstart64.S
> >> 
> >> .globl start
> >> start:
> >>         /* get our base address */
> >>      [..]
> >> 
> >> 1:
> >>         /* zero BSS */
> >>      [..]
> >> 
> >>         /* zero and set up stack */
> >>      [..]
> >> 
> >>         /* set SCTLR_EL1 to a known value */
> >>         ldr     x4, =INIT_SCTLR_EL1_MMU_OFF
> >>      [..]
> >> 
> >>         /* set up exception handling */
> >>         bl      exceptions_init
> >>      [..]
> >> 
> >> Where in lib/arm64/asm/sysreg.h:
> >> 
> >> #define SCTLR_EL1_RES1  (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \
> >>                          _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29))
> >> #define INIT_SCTLR_EL1_MMU_OFF  \
> >>                         SCTLR_EL1_RES1
> >> 
> >> Look like bit 23 (SPAN) should be set.
> >> 
> >> How are you seeing SCTLR_EL1.SPAN unset?
> > 
> > Yeah. the sctlr_el1.SPAN has always been set by the above flow. So Nadav
> > you can describe what you encounter with more details. Like which tests
> > crash you encounter, and how to reproduce it.
> 
> I am using Nikos’s work to run the test using EFI, not from QEMU.
> 
> So the code that you mentioned - which is supposed to initialize SCTLR -
> is not executed (and actually not part of the EFI image).
> 
> Note that using EFI, the entry point is _start [1], and not “start”.
> 
> That is also the reason lack of BSS zeroing also caused me issues with the
> EFI setup, which I reported before.

Nadav,

Would you mind reposting this along with the BSS zeroing patch, the
way I proposed we do that, and anything else you've discovered when
trying to use the EFI unit tests without QEMU? We'll call that our
first non-QEMU EFI support series, since the first EFI series was
only targeting QEMU.

Thanks,
drew

> 
> 
> 
> [1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/blob/master/arm/efi/crt0-efi-aarch64.S#L113
> 

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Andrew Jones]

On Mon, Jul 17, 2023 at 08:50:30AM +0200, Andrew Jones wrote:
> On Fri, Jul 14, 2023 at 06:42:25PM +0000, Nadav Amit wrote:
> > 
> > 
> > > On Jul 14, 2023, at 4:29 AM, Shaoqin Huang <shahuang@redhat.com> wrote:
> > > 
> > > !! External Email
> > > 
> > > Hi,
> > > 
> > > On 7/14/23 18:31, Alexandru Elisei wrote:
> > >> Hi,
> > >> 
> > >> On Sat, Jun 17, 2023 at 01:31:37AM +0000, Nadav Amit wrote:
> > >>> From: Nadav Amit <namit@vmware.com>
> > >>> 
> > >>> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
> > >> 
> > >> In arm/cstart64.S
> > >> 
> > >> .globl start
> > >> start:
> > >>         /* get our base address */
> > >>      [..]
> > >> 
> > >> 1:
> > >>         /* zero BSS */
> > >>      [..]
> > >> 
> > >>         /* zero and set up stack */
> > >>      [..]
> > >> 
> > >>         /* set SCTLR_EL1 to a known value */
> > >>         ldr     x4, =INIT_SCTLR_EL1_MMU_OFF
> > >>      [..]
> > >> 
> > >>         /* set up exception handling */
> > >>         bl      exceptions_init
> > >>      [..]
> > >> 
> > >> Where in lib/arm64/asm/sysreg.h:
> > >> 
> > >> #define SCTLR_EL1_RES1  (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \
> > >>                          _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29))
> > >> #define INIT_SCTLR_EL1_MMU_OFF  \
> > >>                         SCTLR_EL1_RES1
> > >> 
> > >> Look like bit 23 (SPAN) should be set.
> > >> 
> > >> How are you seeing SCTLR_EL1.SPAN unset?
> > > 
> > > Yeah. the sctlr_el1.SPAN has always been set by the above flow. So Nadav
> > > you can describe what you encounter with more details. Like which tests
> > > crash you encounter, and how to reproduce it.
> > 
> > I am using Nikos’s work to run the test using EFI, not from QEMU.
> > 
> > So the code that you mentioned - which is supposed to initialize SCTLR -
> > is not executed (and actually not part of the EFI image).
> > 
> > Note that using EFI, the entry point is _start [1], and not “start”.
> > 
> > That is also the reason lack of BSS zeroing also caused me issues with the
> > EFI setup, which I reported before.
> 
> Nadav,
> 
> Would you mind reposting this along with the BSS zeroing patch, the
> way I proposed we do that, and anything else you've discovered when
> trying to use the EFI unit tests without QEMU? We'll call that our
> first non-QEMU EFI support series, since the first EFI series was
> only targeting QEMU.

Oh, and I meant to mention that, when reposting this patch, maybe we
can consider managing sctlr in a similar way to the non-efi start path?

Thanks,
drew

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nikos Nikoleris]

On 17/07/2023 07:52, Andrew Jones wrote:
> On Mon, Jul 17, 2023 at 08:50:30AM +0200, Andrew Jones wrote:
>> On Fri, Jul 14, 2023 at 06:42:25PM +0000, Nadav Amit wrote:
>>>
>>>
>>>> On Jul 14, 2023, at 4:29 AM, Shaoqin Huang <shahuang@redhat.com> wrote:
>>>>
>>>> !! External Email
>>>>
>>>> Hi,
>>>>
>>>> On 7/14/23 18:31, Alexandru Elisei wrote:
>>>>> Hi,
>>>>>
>>>>> On Sat, Jun 17, 2023 at 01:31:37AM +0000, Nadav Amit wrote:
>>>>>> From: Nadav Amit <namit@vmware.com>
>>>>>>
>>>>>> Do not assume PAN is not supported or that sctlr_el1.SPAN is already set.
>>>>>
>>>>> In arm/cstart64.S
>>>>>
>>>>> .globl start
>>>>> start:
>>>>>          /* get our base address */
>>>>>       [..]
>>>>>
>>>>> 1:
>>>>>          /* zero BSS */
>>>>>       [..]
>>>>>
>>>>>          /* zero and set up stack */
>>>>>       [..]
>>>>>
>>>>>          /* set SCTLR_EL1 to a known value */
>>>>>          ldr     x4, =INIT_SCTLR_EL1_MMU_OFF
>>>>>       [..]
>>>>>
>>>>>          /* set up exception handling */
>>>>>          bl      exceptions_init
>>>>>       [..]
>>>>>
>>>>> Where in lib/arm64/asm/sysreg.h:
>>>>>
>>>>> #define SCTLR_EL1_RES1  (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \
>>>>>                           _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29))
>>>>> #define INIT_SCTLR_EL1_MMU_OFF  \
>>>>>                          SCTLR_EL1_RES1
>>>>>
>>>>> Look like bit 23 (SPAN) should be set.
>>>>>
>>>>> How are you seeing SCTLR_EL1.SPAN unset?
>>>>
>>>> Yeah. the sctlr_el1.SPAN has always been set by the above flow. So Nadav
>>>> you can describe what you encounter with more details. Like which tests
>>>> crash you encounter, and how to reproduce it.
>>>
>>> I am using Nikos’s work to run the test using EFI, not from QEMU.
>>>
>>> So the code that you mentioned - which is supposed to initialize SCTLR -
>>> is not executed (and actually not part of the EFI image).
>>>
>>> Note that using EFI, the entry point is _start [1], and not “start”.
>>>
>>> That is also the reason lack of BSS zeroing also caused me issues with the
>>> EFI setup, which I reported before.
>>
>> Nadav,
>>
>> Would you mind reposting this along with the BSS zeroing patch, the
>> way I proposed we do that, and anything else you've discovered when
>> trying to use the EFI unit tests without QEMU? We'll call that our
>> first non-QEMU EFI support series, since the first EFI series was
>> only targeting QEMU.
> 
> Oh, and I meant to mention that, when reposting this patch, maybe we
> can consider managing sctlr in a similar way to the non-efi start path?
> 

Nadav, if you are running baremetal, it might be worth checking what EL 
you're running in as well. If HW is implementing EL2, EFI will handover 
in EL2.

I was planning to rebase an old patch (more like rewrite it) but I 
haven't found the time yet [1]. If I remember correctly, we have to 
check what EL we're running in and if it's EL2 we have to add a stub 
EL2, drop to EL1 and setup EL1. But things have change since that patch 
and with the new structure, I am not sure if we would drop to EL1 right 
at the start (crt0-efi-aarch64.S) or somewhere in setup_efi().

In general, I think, it would be easier to deal with this in QEMU 
(-machine secure=on) and before we even start thinking about real 
hardware where it is very likely that we will have to address other 
issues (such as the problem with the BSS, cache maintenance) as well.

[1]: 
https://github.com/relokin/kvm-unit-tests/commit/1468abeee7be1d85140ed92cb91a42ee27a9bf1f

Thanks,

Nikos

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nadav Amit]

Combining the answers to Andrew and Nikos.

On Jul 17, 2023, at 1:53 AM, Nikos Nikoleris <nikos.nikoleris@arm.com> wrote:
> 
>>> 
>>> Would you mind reposting this along with the BSS zeroing patch, the
>>> way I proposed we do that, and anything else you've discovered when
>>> trying to use the EFI unit tests without QEMU? We'll call that our
>>> first non-QEMU EFI support series, since the first EFI series was
>>> only targeting QEMU.

I need to rehash the solution that you proposed for BSS (if there is
anything special there). I had a different workaround for that issue,
because IIRC I had some issues with the zeroing. I’ll give it another

>> 
>> Oh, and I meant to mention that, when reposting this patch, maybe we
>> can consider managing sctlr in a similar way to the non-efi start path?
>> 

I am afraid of turning on random bits on SCTLR. Arguably, the way that
the non-efi test sets the default value of SCTLR (with no naming of the
different bits) is not very friendly.

I will have a look on the other bits of SCTLR and see if I can do something
quick and simple, but I don’t want to refactor things in a way that might
break things.

> 
> Nadav, if you are running baremetal, it might be worth checking what EL
> you're running in as well. If HW is implementing EL2, EFI will handover
> in EL2.

I don’t. I run the test on a different hypervisor. When I enabled the x86
tests to run on a different hypervisor years ago, there were many many
test and real issues that required me to run KVM-unit-tests on bare
metal - and therefore I fixed these tests to run on bare-metal as well.

With ARM, excluding the BSS and the SCTLR issue, I didn’t encounter any
additional test issues. So I don’t have the need or time to enable it
to run on bare-metal… sorry.
 

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Alexandru Elisei]

Hi,

On Mon, Jul 17, 2023 at 05:05:06PM +0000, Nadav Amit wrote:
> Combining the answers to Andrew and Nikos.
> 
> On Jul 17, 2023, at 1:53 AM, Nikos Nikoleris <nikos.nikoleris@arm.com> wrote:
> > 
> >>> 
> >>> Would you mind reposting this along with the BSS zeroing patch, the
> >>> way I proposed we do that, and anything else you've discovered when
> >>> trying to use the EFI unit tests without QEMU? We'll call that our
> >>> first non-QEMU EFI support series, since the first EFI series was
> >>> only targeting QEMU.
> 
> I need to rehash the solution that you proposed for BSS (if there is
> anything special there). I had a different workaround for that issue,
> because IIRC I had some issues with the zeroing. I’ll give it another
> 
> >> 
> >> Oh, and I meant to mention that, when reposting this patch, maybe we
> >> can consider managing sctlr in a similar way to the non-efi start path?
> >> 
> 
> I am afraid of turning on random bits on SCTLR. Arguably, the way that

What do you mean by turning on random bits on SCTLR? All the functional
bits are documented in the architecture. Same goes for RES1 (it's in the
Glossary).

> the non-efi test sets the default value of SCTLR (with no naming of the
> different bits) is not very friendly.

That's because as the architecture gets updated, what used to be a RES1 bit
becomes a functional bit. The only sane way to handle this is to disable
all the features you don't support, **and** set all the RES1 bits (and
clear RES0 bits), to disable any newly introduced features you don't know
about yet which were left enabled by software running at a higher privilege
level.

You can send a patch if you want to give a name to the bits which have
become functional since SCTLR_EL1_RES1 was introduced.

Thanks,
Alex

> 
> I will have a look on the other bits of SCTLR and see if I can do something
> quick and simple, but I don’t want to refactor things in a way that might
> break things.
> 
> > 
> > Nadav, if you are running baremetal, it might be worth checking what EL
> > you're running in as well. If HW is implementing EL2, EFI will handover
> > in EL2.
> 
> I don’t. I run the test on a different hypervisor. When I enabled the x86
> tests to run on a different hypervisor years ago, there were many many
> test and real issues that required me to run KVM-unit-tests on bare
> metal - and therefore I fixed these tests to run on bare-metal as well.
> 
> With ARM, excluding the BSS and the SCTLR issue, I didn’t encounter any
> additional test issues. So I don’t have the need or time to enable it
> to run on bare-metal… sorry.
>  

Re: [kvm-unit-tests PATCH 1/2] arm64: set sctlr_el1.SPAN

[Nadav Amit]

> On Jul 18, 2023, at 1:44 AM, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi,
> 
> On Mon, Jul 17, 2023 at 05:05:06PM +0000, Nadav Amit wrote:
>> Combining the answers to Andrew and Nikos.
>> 
>> On Jul 17, 2023, at 1:53 AM, Nikos Nikoleris <nikos.nikoleris@arm.com> wrote:
>>> 
>>>>> 
>>>>> Would you mind reposting this along with the BSS zeroing patch, the
>>>>> way I proposed we do that, and anything else you've discovered when
>>>>> trying to use the EFI unit tests without QEMU? We'll call that our
>>>>> first non-QEMU EFI support series, since the first EFI series was
>>>>> only targeting QEMU.
>> 
>> I need to rehash the solution that you proposed for BSS (if there is
>> anything special there). I had a different workaround for that issue,
>> because IIRC I had some issues with the zeroing. I’ll give it another
>> 
>>>> 
>>>> Oh, and I meant to mention that, when reposting this patch, maybe we
>>>> can consider managing sctlr in a similar way to the non-efi start path?
>>>> 
>> 
>> I am afraid of turning on random bits on SCTLR. Arguably, the way that
> 
> What do you mean by turning on random bits on SCTLR? All the functional
> bits are documented in the architecture. Same goes for RES1 (it's in the
> Glossary).
> 
>> the non-efi test sets the default value of SCTLR (with no naming of the
>> different bits) is not very friendly.
> 
> That's because as the architecture gets updated, what used to be a RES1 bit
> becomes a functional bit. The only sane way to handle this is to disable
> all the features you don't support, **and** set all the RES1 bits (and
> clear RES0 bits), to disable any newly introduced features you don't know
> about yet which were left enabled by software running at a higher privilege
> level.
> 
> You can send a patch if you want to give a name to the bits which have
> become functional since SCTLR_EL1_RES1 was introduced.

I can give it a quick shot, but I do need to clarify something. Although
I have rather deep knowledge of x86 architecture with over 20 years of
experience, my experience with ARM is limited to 2 weeks. And I don’t even
have an environment in which I can run KVM+ARM.

So I am not inclined to revamp code that was actually just added to
kvm-unit-tests. I will attempt to refactor the code to solve both the
BSS and SCTLR issues in EFI under these limitations.