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

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

You seem to have accidentally dropped xen-devel. Re-adding.

On 13/02/2025 12:04, Volodymyr Babchuk wrote:
> 
> 
> Hi Michal,
> 
> "Orzel, Michal" <michal.orzel@xxxxxxx> writes:
> 
>> Hi Volodymyr,
>>
>> NIT: s/EL/IL/ in commit title
> 
> Sure, thanks.
> 
>> 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()
> 
> You are right, I'll do this in the second version. It is strange that
> compiler didn't warn me about unused parameter, though...
You would need to explicitly set -Wunused-parameter in EXTRA_CFLAGS_XEN_CORE.
There are other issues there though.

> 
>> BTW. Are you going to also look at simplifying e.g. inject_iabt_exception() 
>> for
>> which IL is also 1?
> 
> Yes, I'll add a separate patch in the next version.
> 
> --
> WBR, Volodymyr

~Michal