[kvm-unit-tests PATCH] arm64: Make vector_table and vector_stub weak symbols

[kvm-unit-tests PATCH] arm64: Make vector_table and vector_stub weak symbols

[Nikos Nikoleris]

This changes allows a test to define and override the declared symbols,
taking control of the whole vector_table or a vector_stub.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 arm/cstart64.S | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arm/cstart64.S b/arm/cstart64.S
index e4ab7d06..eda0daa5 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -275,8 +275,11 @@ exceptions_init:
 /*
  * Vector stubs
  * Adapted from arch/arm64/kernel/entry.S
+ * Declare as weak to allow external tests to redefine and override a
+ * vector_stub.
  */
 .macro vector_stub, name, vec
+.weak \name
 \name:
 	stp	 x0,  x1, [sp, #-S_FRAME_SIZE]!
 	stp	 x2,  x3, [sp,  #16]
@@ -369,7 +372,13 @@ vector_stub	el0_error_32, 15
 	b	\label
 .endm
 
+
+/*
+ * Declare as weak to allow external tests to redefine and override the
+ * default vector table.
+ */
 .align 11
+.weak vector_table
 vector_table:
 	ventry	el1t_sync			// Synchronous EL1t
 	ventry	el1t_irq			// IRQ EL1t
-- 
2.25.1

Re: [kvm-unit-tests PATCH] arm64: Make vector_table and vector_stub weak symbols

[Andrew Jones]

On Mon, May 15, 2023 at 11:15:17PM +0100, Nikos Nikoleris wrote:
> This changes allows a test to define and override the declared symbols,
> taking control of the whole vector_table or a vector_stub.

Hi Nikos,

Can you add some motivation for this change to the commit message or
submit it along with some test that needs it?

Thanks,
drew

> 
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  arm/cstart64.S | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arm/cstart64.S b/arm/cstart64.S
> index e4ab7d06..eda0daa5 100644
> --- a/arm/cstart64.S
> +++ b/arm/cstart64.S
> @@ -275,8 +275,11 @@ exceptions_init:
>  /*
>   * Vector stubs
>   * Adapted from arch/arm64/kernel/entry.S
> + * Declare as weak to allow external tests to redefine and override a
> + * vector_stub.
>   */
>  .macro vector_stub, name, vec
> +.weak \name
>  \name:
>  	stp	 x0,  x1, [sp, #-S_FRAME_SIZE]!
>  	stp	 x2,  x3, [sp,  #16]
> @@ -369,7 +372,13 @@ vector_stub	el0_error_32, 15
>  	b	\label
>  .endm
>  
> +
> +/*
> + * Declare as weak to allow external tests to redefine and override the
> + * default vector table.
> + */
>  .align 11
> +.weak vector_table
>  vector_table:
>  	ventry	el1t_sync			// Synchronous EL1t
>  	ventry	el1t_irq			// IRQ EL1t
> -- 
> 2.25.1
> 

Re: [kvm-unit-tests PATCH] arm64: Make vector_table and vector_stub weak symbols

[Nikos Nikoleris]

On 18/05/2023 17:06, Andrew Jones wrote:
> On Mon, May 15, 2023 at 11:15:17PM +0100, Nikos Nikoleris wrote:
>> This changes allows a test to define and override the declared symbols,
>> taking control of the whole vector_table or a vector_stub.
> 
> Hi Nikos,
> 
> Can you add some motivation for this change to the commit message or
> submit it along with some test that needs it?
> 

Hi Drew,

Thanks for reviewing this.

What do you think about adding the following to the commit message?

> With the ability to override specific exception handlers, litmus7
> [1] a tool used to generate c sources for a given memory model
> litmus test, can override the el1h_sync symbol to implement tests
> with explicit exception handlers. For example:
> 
> AArch64 LDRv0+I2V-dsb.ishst
 > { >   [PTE(x)]=(oa:PA(x),valid:0);
>   x=1;
> 
>   0:X1=x;
>   0:X3=PTE(x); >   0:X2=(oa:PA(x),valid:1);
 > }>  P0          | P0.F         ;
> L0:          | ADD X8,X8,#1 ;
>  LDR W0,[X1] | STR X2,[X3]  ;
>              | DSB ISHST    ;
>              | ERET         ; > exists(0:X0=0 \/ 0:X8!=1)
> 
> In this test, a thread running in core P0 executes a load to a memory
> location x. The PTE of the virtual address x is initially invalid.
> The execution of the load causes a synchronous EL1 exception which is
> handled by the code in P0.F. P0.F increments a counter which is
> maintained in X8, updates the PTE of x and makes it valid, executes a
> DSB ISHST and calls ERET which is expected to return and retry the
> execution of the load in P0:L0.
> 
> The postcondition checks if there is any execution where the load 
> wasn't executed (X0 its destination register is not update), or that 
> the P0.F > handler was invoked more than once (the counter X8 is not 
> 1).
> 
> For this tests, litmus7 needs to control the el1h_sync. Calling 
> install_exception_handler() would be suboptimal because the 
> vector_stub would wrap around the code of P0.F and disturb the test.
> 
> [1]: https://diy.inria.fr/doc/litmus.html
If you think this is sufficient, I will update the patch. If not I could 
look into adding a test in KUT but generally the code of these tests are 
generated by a tool and the coding style (if one can call it a style) is 
very different.

Thanks,

Nikos

> Thanks,
> drew
> 
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>>   arm/cstart64.S | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arm/cstart64.S b/arm/cstart64.S
>> index e4ab7d06..eda0daa5 100644
>> --- a/arm/cstart64.S
>> +++ b/arm/cstart64.S
>> @@ -275,8 +275,11 @@ exceptions_init:
>>   /*
>>    * Vector stubs
>>    * Adapted from arch/arm64/kernel/entry.S
>> + * Declare as weak to allow external tests to redefine and override a
>> + * vector_stub.
>>    */
>>   .macro vector_stub, name, vec
>> +.weak \name
>>   \name:
>>   	stp	 x0,  x1, [sp, #-S_FRAME_SIZE]!
>>   	stp	 x2,  x3, [sp,  #16]
>> @@ -369,7 +372,13 @@ vector_stub	el0_error_32, 15
>>   	b	\label
>>   .endm
>>   
>> +
>> +/*
>> + * Declare as weak to allow external tests to redefine and override the
>> + * default vector table.
>> + */
>>   .align 11
>> +.weak vector_table
>>   vector_table:
>>   	ventry	el1t_sync			// Synchronous EL1t
>>   	ventry	el1t_irq			// IRQ EL1t
>> -- 
>> 2.25.1
>>

Re: [kvm-unit-tests PATCH] arm64: Make vector_table and vector_stub weak symbols

[Andrew Jones]

On Mon, May 22, 2023 at 02:54:14PM +0100, Nikos Nikoleris wrote:
> On 18/05/2023 17:06, Andrew Jones wrote:
> > On Mon, May 15, 2023 at 11:15:17PM +0100, Nikos Nikoleris wrote:
> > > This changes allows a test to define and override the declared symbols,
> > > taking control of the whole vector_table or a vector_stub.
> > 
> > Hi Nikos,
> > 
> > Can you add some motivation for this change to the commit message or
> > submit it along with some test that needs it?
> > 
> 
> Hi Drew,
> 
> Thanks for reviewing this.
> 
> What do you think about adding the following to the commit message?
> 
> > With the ability to override specific exception handlers, litmus7
> > [1] a tool used to generate c sources for a given memory model
> > litmus test, can override the el1h_sync symbol to implement tests
> > with explicit exception handlers. For example:
> > 
> > AArch64 LDRv0+I2V-dsb.ishst
> > { >   [PTE(x)]=(oa:PA(x),valid:0);
> >   x=1;
> > 
> >   0:X1=x;
> >   0:X3=PTE(x); >   0:X2=(oa:PA(x),valid:1);
> > }>  P0          | P0.F         ;
> > L0:          | ADD X8,X8,#1 ;
> >  LDR W0,[X1] | STR X2,[X3]  ;
> >              | DSB ISHST    ;
> >              | ERET         ; > exists(0:X0=0 \/ 0:X8!=1)
> > 
> > In this test, a thread running in core P0 executes a load to a memory
> > location x. The PTE of the virtual address x is initially invalid.
> > The execution of the load causes a synchronous EL1 exception which is
> > handled by the code in P0.F. P0.F increments a counter which is
> > maintained in X8, updates the PTE of x and makes it valid, executes a
> > DSB ISHST and calls ERET which is expected to return and retry the
> > execution of the load in P0:L0.
> > 
> > The postcondition checks if there is any execution where the load wasn't
> > executed (X0 its destination register is not update), or that the P0.F >
> > handler was invoked more than once (the counter X8 is not 1).
> > 
> > For this tests, litmus7 needs to control the el1h_sync. Calling
> > install_exception_handler() would be suboptimal because the vector_stub
> > would wrap around the code of P0.F and disturb the test.
> > 
> > [1]: https://diy.inria.fr/doc/litmus.html
> If you think this is sufficient, I will update the patch.

The above works for me.

(Unrelated: Sorry I haven't had a chance to give your latest efi branch
a test drive. I think you can probably go ahead and post the next version
of the series, though. That'll help bring it to the forefront for me to
prioritize.)

Thanks,
drew

Re: [kvm-unit-tests PATCH] arm64: Make vector_table and vector_stub weak symbols

[Nikos Nikoleris]

On 23/05/2023 09:56, Andrew Jones wrote:
> On Mon, May 22, 2023 at 02:54:14PM +0100, Nikos Nikoleris wrote:
>> On 18/05/2023 17:06, Andrew Jones wrote:
>>> On Mon, May 15, 2023 at 11:15:17PM +0100, Nikos Nikoleris wrote:
>>>> This changes allows a test to define and override the declared symbols,
>>>> taking control of the whole vector_table or a vector_stub.
>>>
>>> Hi Nikos,
>>>
>>> Can you add some motivation for this change to the commit message or
>>> submit it along with some test that needs it?
>>>
>>
>> Hi Drew,
>>
>> Thanks for reviewing this.
>>
>> What do you think about adding the following to the commit message?
>>
>>> With the ability to override specific exception handlers, litmus7
>>> [1] a tool used to generate c sources for a given memory model
>>> litmus test, can override the el1h_sync symbol to implement tests
>>> with explicit exception handlers. For example:
>>>
>>> AArch64 LDRv0+I2V-dsb.ishst
>>> { >   [PTE(x)]=(oa:PA(x),valid:0);
>>>    x=1;
>>>
>>>    0:X1=x;
>>>    0:X3=PTE(x); >   0:X2=(oa:PA(x),valid:1);
>>> }>  P0          | P0.F         ;
>>> L0:          | ADD X8,X8,#1 ;
>>>   LDR W0,[X1] | STR X2,[X3]  ;
>>>               | DSB ISHST    ;
>>>               | ERET         ; > exists(0:X0=0 \/ 0:X8!=1)
>>>
>>> In this test, a thread running in core P0 executes a load to a memory
>>> location x. The PTE of the virtual address x is initially invalid.
>>> The execution of the load causes a synchronous EL1 exception which is
>>> handled by the code in P0.F. P0.F increments a counter which is
>>> maintained in X8, updates the PTE of x and makes it valid, executes a
>>> DSB ISHST and calls ERET which is expected to return and retry the
>>> execution of the load in P0:L0.
>>>
>>> The postcondition checks if there is any execution where the load wasn't
>>> executed (X0 its destination register is not update), or that the P0.F >
>>> handler was invoked more than once (the counter X8 is not 1).
>>>
>>> For this tests, litmus7 needs to control the el1h_sync. Calling
>>> install_exception_handler() would be suboptimal because the vector_stub
>>> would wrap around the code of P0.F and disturb the test.
>>>
>>> [1]: https://diy.inria.fr/doc/litmus.html
>> If you think this is sufficient, I will update the patch.
> 
> The above works for me.
> 
> (Unrelated: Sorry I haven't had a chance to give your latest efi branch
> a test drive. I think you can probably go ahead and post the next version
> of the series, though. That'll help bring it to the forefront for me to
> prioritize.)
> 

Thanks Drew, I'll send a new revision of this patch and the EFI series.

Nikos