You seem to have accidentally dropped xen-devel. Re-adding.
On 13/02/2025 12:04, Volodymyr Babchuk wrote:
>
>
> Hi Michal,
>
> "Orzel, Michal" <michal.or...@amd.com> 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_babc...@epam.com>
>>>
>>> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
>>>
>>>
>>>> ---
>>>> 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
