| Message ID | 20220421100547.873761-6-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Treat ESR_ELx as a 64-bit register | expand |
On Thu, 21 Apr 2022 11:05:47 +0100, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 93d92130d36c..fd5b6773e3a2 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -121,6 +121,8 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > > run->exit_reason = KVM_EXIT_DEBUG; > run->debug.arch.hsr = lower_32_bits(esr); > + run->debug.arch.hsr_high = upper_32_bits(esr); > + run->flags |= KVM_DEBUG_ARCH_HSR_HIGH_VALID; Who will eventually clear this flag? I'm concerned that it could be misinterpreted by other userspace paths, as once you get a debug exit on this vcpu, it will always be set. Probably only a matter of clearing flags on all the other exit paths. Also, please document the flag in the API file (only a couple of x86 flags are there so far). Thanks, M.
Hi, On Thu, Apr 21, 2022 at 01:58:52PM +0100, Marc Zyngier wrote: > On Thu, 21 Apr 2022 11:05:47 +0100, > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > > index 93d92130d36c..fd5b6773e3a2 100644 > > --- a/arch/arm64/kvm/handle_exit.c > > +++ b/arch/arm64/kvm/handle_exit.c > > @@ -121,6 +121,8 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > > > > run->exit_reason = KVM_EXIT_DEBUG; > > run->debug.arch.hsr = lower_32_bits(esr); > > + run->debug.arch.hsr_high = upper_32_bits(esr); > > + run->flags |= KVM_DEBUG_ARCH_HSR_HIGH_VALID; > > Who will eventually clear this flag? I'm concerned that it could be > misinterpreted by other userspace paths, as once you get a debug exit > on this vcpu, it will always be set. > > Probably only a matter of clearing flags on all the other exit paths. I missed this part, I was under the impression that kvm_run->flags was already cleared on every exit (that's why it's bitwise OR'ed with the flag). kvm_arch_vcpu_ioctl_run() always sets exit_reason = KVM_EXIT_UNKNOWN, so I guess if we want to be consistent, kvm_run->flags should be cleared at the same time. Unless you want KVM to clear flags for all exit reasons *except* KVM_EXIT_UNKNOWN. I prefer clearing flags in kvm_arch_vcpu_ioctl_run() as that looks to me like the least error prone way to do it, and if in the future an exit reason wants to preserve flags across KVM_RUN ioctls we can add a check for that particular situation. > > Also, please document the flag in the API file (only a couple of x86 > flags are there so far). Sure thing, will do. Thanks, Alex
On Thu, 21 Apr 2022 16:22:00 +0100, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi, > > On Thu, Apr 21, 2022 at 01:58:52PM +0100, Marc Zyngier wrote: > > On Thu, 21 Apr 2022 11:05:47 +0100, > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > > > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > > > index 93d92130d36c..fd5b6773e3a2 100644 > > > --- a/arch/arm64/kvm/handle_exit.c > > > +++ b/arch/arm64/kvm/handle_exit.c > > > @@ -121,6 +121,8 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > > > > > > run->exit_reason = KVM_EXIT_DEBUG; > > > run->debug.arch.hsr = lower_32_bits(esr); > > > + run->debug.arch.hsr_high = upper_32_bits(esr); > > > + run->flags |= KVM_DEBUG_ARCH_HSR_HIGH_VALID; > > > > Who will eventually clear this flag? I'm concerned that it could be > > misinterpreted by other userspace paths, as once you get a debug exit > > on this vcpu, it will always be set. > > > > Probably only a matter of clearing flags on all the other exit paths. > > I missed this part, I was under the impression that kvm_run->flags was > already cleared on every exit (that's why it's bitwise OR'ed with the > flag). > > kvm_arch_vcpu_ioctl_run() always sets exit_reason = KVM_EXIT_UNKNOWN, so I > guess if we want to be consistent, kvm_run->flags should be cleared at the > same time. Unless you want KVM to clear flags for all exit reasons *except* > KVM_EXIT_UNKNOWN. > > I prefer clearing flags in kvm_arch_vcpu_ioctl_run() as that looks to me > like the least error prone way to do it, and if in the future an exit > reason wants to preserve flags across KVM_RUN ioctls we can add a check for > that particular situation. It seems entirely reasonable to reset exit_reason and flags together. > > > > > Also, please document the flag in the API file (only a couple of x86 > > flags are there so far). > > Sure thing, will do. Thanks, M.
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h index c1b6ddc02d2f..695324b6f92e 100644 --- a/arch/arm64/include/uapi/asm/kvm.h +++ b/arch/arm64/include/uapi/asm/kvm.h @@ -139,8 +139,10 @@ struct kvm_guest_debug_arch { __u64 dbg_wvr[KVM_ARM_MAX_DBG_REGS]; }; +#define KVM_DEBUG_ARCH_HSR_HIGH_VALID 1 struct kvm_debug_exit_arch { __u32 hsr; + __u32 hsr_high; /* ESR_EL2[61:32] */ __u64 far; /* used for watchpoints */ }; diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 93d92130d36c..fd5b6773e3a2 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -121,6 +121,8 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) run->exit_reason = KVM_EXIT_DEBUG; run->debug.arch.hsr = lower_32_bits(esr); + run->debug.arch.hsr_high = upper_32_bits(esr); + run->flags |= KVM_DEBUG_ARCH_HSR_HIGH_VALID; if (ESR_ELx_EC(esr) == ESR_ELx_EC_WATCHPT_LOW) run->debug.arch.far = vcpu->arch.fault.far_el2;
When userspace is debugging a VM, the kvm_debug_exit_arch part of the kvm_run struct contains arm64 specific debug information: the ESR_EL2 value, encoded in the field "hsr", and the address of the instruction that caused the exception, encoded in the field "far". Linux has moved to treating ESR_EL2 as a 64-bit register, but unfortunately kvm_debug_exit_arch.hsr cannot be changed because that would change the memory layout of the struct on big endian machines: Current layout: | Layout with "hsr" extended to 64 bits: | offset 0: ESR_EL2[31:0] (hsr) | offset 0: ESR_EL2[61:32] (hsr[61:32]) offset 4: padding | offset 4: ESR_EL2[31:0] (hsr[31:0]) offset 8: FAR_EL2[61:0] (far) | offset 8: FAR_EL2[61:0] (far) which breaks existing code. The padding is inserted by the compiler because the "far" field must be aligned to 8 bytes (each field must be naturally aligned - aapcs64 [1], page 18), and the struct itself must be aligned to 8 bytes (the struct must be aligned to the maximum alignment of its fields - aapcs64, page 18), which means that "hsr" must be aligned to 8 bytes as it is the first field in the struct. To avoid changing the struct size and layout for the existing fields, add a new field, "hsr_high", which replaces the existing padding. "hsr_high" will be used to hold the ESR_EL2[61:32] bits of the register. The memory layout, both on big and little endian machine, becomes: offset 0: ESR_EL2[31:0] (hsr) offset 4: ESR_EL2[61:32] (hsr_high) offset 8: FAR_EL2[61:0] (far) The padding that the compiler inserts for the current struct layout is unitialized. To prevent an updated userspace running on an old kernel mistaking the padding for a valid "hsr_high" value, add a new flag, KVM_DEBUG_ARCH_HSR_HIGH_VALID, to kvm_run->flags to let userspace know that "hsr_high" holds a valid ESR_EL2[61:32] value. [1] https://github.com/ARM-software/abi-aa/releases/download/2021Q3/aapcs64.pdf Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arch/arm64/include/uapi/asm/kvm.h | 2 ++ arch/arm64/kvm/handle_exit.c | 2 ++ 2 files changed, 4 insertions(+)