Hi Marc,
On Sun, Mar 17, 2024 at 01:09:01PM +0000, Marc Zyngier wrote:
> On Thu, 14 Mar 2024 20:25:43 +0000,
> Pierre-Clément Tosi <ptosi@xxxxxxxxxx> wrote:
> >
> > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> > index b0c23e7d6595..281e352a4c94 100644
> > --- a/arch/arm64/include/asm/esr.h
> > +++ b/arch/arm64/include/asm/esr.h
> > @@ -397,6 +397,12 @@ static inline bool esr_is_data_abort(unsigned long esr)
> > return ec == ESR_ELx_EC_DABT_LOW || ec == ESR_ELx_EC_DABT_CUR;
> > }
> >
> > +static inline bool esr_is_cfi_brk(unsigned long esr)
> > +{
> > + return ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
> > + (esr_comment(esr) & ~CFI_BRK_IMM_MASK) == CFI_BRK_IMM_BASE;
> > +}
> > +
>
> nit: since there is a single user, please place this helper in handle_exit.c.
I've placed this here as I'm introducing a second user in a following patch of
this series (in the VHE code) and wanted to avoid adding code then immediately
moving it around.
I've therefore kept this part unchanged in v2 but let me know if you prefer the
commits to add-then-move and I'll update that for v3.
> > static inline bool esr_fsc_is_translation_fault(unsigned long esr)
> > {
> > return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index ffa67ac6656c..9b6574e50b13 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -383,6 +383,15 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
> > kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
> > }
> >
> > +static void kvm_nvhe_report_cfi_failure(u64 panic_addr)
> > +{
> > + kvm_err("nVHE hyp CFI failure at: [<%016llx>] %pB!\n", panic_addr,
> > + (void *)(panic_addr + kaslr_offset()));
> > +
> > + if (IS_ENABLED(CONFIG_CFI_PERMISSIVE))
> > + kvm_err(" (CONFIG_CFI_PERMISSIVE ignored for hyp failures)\n");
> > +}
> > +
> > void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
> > u64 elr_virt, u64 elr_phys,
> > u64 par, uintptr_t vcpu,
> > @@ -413,6 +422,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
> > else
> > kvm_err("nVHE hyp BUG at: [<%016llx>] %pB!\n", panic_addr,
> > (void *)(panic_addr + kaslr_offset()));
> > + } else if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr)) {
>
> It would seem logical to move the IS_ENABLED() into the ESR check helper.
I suppose it makes sense for a static function but, given that I've kept the
helper in a shared header and as it essentially is a straightforward
shift-mask-compare (like the existing helpers in <asm/esr.h>), wouldn't it be
confusing for its result to depend on a Kconfig flag?
Anyway, same as above; left unchanged in v2 but happy to update this in v3.
--
Pierre
