Re: [PATCH] arch: arm64: always set EL=1 when injecting undefined exception

["Orzel, Michal" <michal.orzel@xxxxxxx>]

Hi Volodymyr,

NIT: s/EL/IL/ in commit title

One remark below.

On 12/02/2025 23:03, Stefano Stabellini wrote:
> 
> 
> On Wed, 12 Feb 2025, Volodymyr Babchuk wrote:
>> ARM Architecture Reference Manual states that EL field of ESR_EL1
>> register should be 1 when EC is 0b000000 aka HSR_EC_UNKNOWN.
>>
>> Section D24.2.40, page D24-7337 of ARM DDI 0487L:
>>
>>   IL, bit [25]
>>   Instruction Length for synchronous exceptions. Possible values of this bit 
>> are:
>>
>>   [...]
>>
>>   0b1 - 32-bit instruction trapped.
>>   This value is also used when the exception is one of the following:
>>   [...]
>>    - An exception reported using EC value 0b000000.
>>
>> To align code with the specification, set .len field to 1 in
>> inject_undef64_exception() and remove unneeded second parameter.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> 
>> ---
>>  xen/arch/arm/arm64/vsysreg.c           |  8 ++++----
>>  xen/arch/arm/include/asm/arm64/traps.h |  2 +-
>>  xen/arch/arm/include/asm/traps.h       |  2 +-
>>  xen/arch/arm/p2m.c                     |  2 +-
>>  xen/arch/arm/traps.c                   | 24 ++++++++++++------------
>>  xen/arch/arm/vcpreg.c                  | 24 ++++++++++++------------
>>  xen/arch/arm/vsmc.c                    |  6 ++----
>>  7 files changed, 33 insertions(+), 35 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
>> index c73b2c95ce..29622a8593 100644
>> --- a/xen/arch/arm/arm64/vsysreg.c
>> +++ b/xen/arch/arm/arm64/vsysreg.c
>> @@ -95,7 +95,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>       */
>>      case HSR_SYSREG_ACTLR_EL1:
>>          if ( regs_mode_is_user(regs) )
>> -            return inject_undef_exception(regs, hsr);
>> +            return inject_undef_exception(regs);
>>          if ( hsr.sysreg.read )
>>              set_user_reg(regs, regidx, v->arch.actlr);
>>          break;
>> @@ -267,7 +267,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>      case HSR_SYSREG_CNTP_TVAL_EL0:
>>      case HSR_SYSREG_CNTP_CVAL_EL0:
>>          if ( !vtimer_emulate(regs, hsr) )
>> -            return inject_undef_exception(regs, hsr);
>> +            return inject_undef_exception(regs);
>>          break;
>>
>>      /*
>> @@ -280,7 +280,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>      case HSR_SYSREG_ICC_SGI0R_EL1:
>>
>>          if ( !vgic_emulate(regs, hsr) )
>> -            return inject_undef64_exception(regs, hsr.len);
>> +            return inject_undef64_exception(regs);
>>          break;
>>
>>      /*
>> @@ -440,7 +440,7 @@ void do_sysreg(struct cpu_user_regs *regs,
>>      gdprintk(XENLOG_ERR,
>>               "unhandled 64-bit sysreg access %#"PRIregister"\n",
>>               hsr.bits & HSR_SYSREG_REGS_MASK);
>> -    inject_undef_exception(regs, hsr);
>> +    inject_undef_exception(regs);
>>  }
>>
>>  /*
>> diff --git a/xen/arch/arm/include/asm/arm64/traps.h 
>> b/xen/arch/arm/include/asm/arm64/traps.h
>> index a347cb13d6..3be2fa69ee 100644
>> --- a/xen/arch/arm/include/asm/arm64/traps.h
>> +++ b/xen/arch/arm/include/asm/arm64/traps.h
>> @@ -1,7 +1,7 @@
>>  #ifndef __ASM_ARM64_TRAPS__
>>  #define __ASM_ARM64_TRAPS__
>>
>> -void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
>> +void inject_undef64_exception(struct cpu_user_regs *regs);
>>
>>  void do_sysreg(struct cpu_user_regs *regs,
>>                 const union hsr hsr);
>> diff --git a/xen/arch/arm/include/asm/traps.h 
>> b/xen/arch/arm/include/asm/traps.h
>> index 9a60dbf70e..3b40afe262 100644
>> --- a/xen/arch/arm/include/asm/traps.h
>> +++ b/xen/arch/arm/include/asm/traps.h
>> @@ -44,7 +44,7 @@ int check_conditional_instr(struct cpu_user_regs *regs, 
>> const union hsr hsr);
>>
>>  void advance_pc(struct cpu_user_regs *regs, const union hsr hsr);
>>
>> -void inject_undef_exception(struct cpu_user_regs *regs, const union hsr 
>> hsr);
>> +void inject_undef_exception(struct cpu_user_regs *regs);
>>
>>  /* read as zero and write ignore */
>>  void handle_raz_wi(struct cpu_user_regs *regs, int regidx, bool read,
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 65b70955e3..6196cad0d4 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -438,7 +438,7 @@ void p2m_set_way_flush(struct vcpu *v, struct 
>> cpu_user_regs *regs,
>>      {
>>          gprintk(XENLOG_ERR,
>>                  "The cache should be flushed by VA rather than by 
>> set/way.\n");
>> -        inject_undef_exception(regs, hsr);
>> +        inject_undef_exception(regs);
There's nothing using hsr anymore, so you should drop hsr parameter from
p2m_set_way_flush()

BTW. Are you going to also look at simplifying e.g. inject_iabt_exception() for
which IL is also 1?

~Michal