Message ID | 20210817081134.2918285-12-tabba@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: arm64: Fixed features for protected VMs | expand |
On Tue, 17 Aug 2021 09:11:30 +0100, Fuad Tabba <tabba@google.com> wrote: > > Add an array of pointers to handlers for various trap reasons in > nVHE code. > > The current code selects how to fixup a guest on exit based on a > series of if/else statements. Future patches will also require > different handling for guest exists. Create an array of handlers > to consolidate them. > > No functional change intended as the array isn't populated yet. > > Acked-by: Will Deacon <will@kernel.org> > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 43 +++++++++++++++++++++++++ > arch/arm64/kvm/hyp/nvhe/switch.c | 33 +++++++++++++++++++ > 2 files changed, 76 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index a0e78a6027be..5a2b89b96c67 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -409,6 +409,46 @@ static inline bool __hyp_handle_ptrauth(struct kvm_vcpu *vcpu) > return true; > } > > +typedef int (*exit_handle_fn)(struct kvm_vcpu *); This returns an int... > + > +exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu); > + > +static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu) > +{ > + return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL; > +} > + > +/* > + * Allow the hypervisor to handle the exit with an exit handler if it has one. > + * > + * Returns true if the hypervisor handled the exit, and control should go back > + * to the guest, or false if it hasn't. > + */ > +static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu) > +{ > + bool is_handled = false; ... which you then implicitly cast as a bool. > + exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu); > + > + if (exit_handler) { > + /* > + * There's limited vcpu context here since it's not synced yet. > + * Ensure that relevant vcpu context that might be used by the > + * exit_handler is in sync before it's called and if handled. > + */ > + *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR); > + *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR); > + > + is_handled = exit_handler(vcpu); What does 'is_handled' mean here? By definition, any trap *must* be handled, one way or another. By the look of it, what you really mean is something like "I have updated the vcpu state and you'd better reload it". Is that what it means? > + > + if (is_handled) { > + write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); > + write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR); > + } > + } > + > + return is_handled; > +} All these functions really should be marked inline. Have you checked how this expands on VHE? I think some compilers could be pretty unhappy about the undefined symbol in kvm_get_hyp_exit_handler(). It is also unfortunate that we get a bunch of tests for various flavours of traps (FP, PAuth, page faults...), only to hit yet another decoding tree. Is there a way we could use this infrastructure for everything? > + > /* > * Return true when we were able to fixup the guest exit and should return to > * the guest, false when we should restore the host state and return to the > @@ -496,6 +536,9 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) > goto guest; > } > > + /* Check if there's an exit handler and allow it to handle the exit. */ > + if (kvm_hyp_handle_exit(vcpu)) > + goto guest; > exit: > /* Return to the host kernel and handle the exit */ > return false; > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > index 86f3d6482935..b7f25307a7b9 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -158,6 +158,39 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) > write_sysreg(pmu->events_host, pmcntenset_el0); > } > > +static exit_handle_fn hyp_exit_handlers[] = { > + [0 ... ESR_ELx_EC_MAX] = NULL, > + [ESR_ELx_EC_WFx] = NULL, > + [ESR_ELx_EC_CP15_32] = NULL, > + [ESR_ELx_EC_CP15_64] = NULL, > + [ESR_ELx_EC_CP14_MR] = NULL, > + [ESR_ELx_EC_CP14_LS] = NULL, > + [ESR_ELx_EC_CP14_64] = NULL, > + [ESR_ELx_EC_HVC32] = NULL, > + [ESR_ELx_EC_SMC32] = NULL, > + [ESR_ELx_EC_HVC64] = NULL, > + [ESR_ELx_EC_SMC64] = NULL, > + [ESR_ELx_EC_SYS64] = NULL, > + [ESR_ELx_EC_SVE] = NULL, > + [ESR_ELx_EC_IABT_LOW] = NULL, > + [ESR_ELx_EC_DABT_LOW] = NULL, > + [ESR_ELx_EC_SOFTSTP_LOW] = NULL, > + [ESR_ELx_EC_WATCHPT_LOW] = NULL, > + [ESR_ELx_EC_BREAKPT_LOW] = NULL, > + [ESR_ELx_EC_BKPT32] = NULL, > + [ESR_ELx_EC_BRK64] = NULL, > + [ESR_ELx_EC_FP_ASIMD] = NULL, > + [ESR_ELx_EC_PAC] = NULL, You can safely drop all these and only keep the top one for now. This will also keep the idiot robot at bay for until the next patch... ;-) > +}; > + > +exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu) > +{ > + u32 esr = kvm_vcpu_get_esr(vcpu); > + u8 esr_ec = ESR_ELx_EC(esr); > + > + return hyp_exit_handlers[esr_ec]; > +} > + > /* Switch to the guest for legacy non-VHE systems */ > int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > { Thanks, M.
Hi Fuad, On Wed, 18 Aug 2021 17:45:50 +0100, Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 17 Aug 2021 09:11:30 +0100, > Fuad Tabba <tabba@google.com> wrote: > > > > Add an array of pointers to handlers for various trap reasons in > > nVHE code. > > > > The current code selects how to fixup a guest on exit based on a > > series of if/else statements. Future patches will also require > > different handling for guest exists. Create an array of handlers > > to consolidate them. > > > > No functional change intended as the array isn't populated yet. > > > > Acked-by: Will Deacon <will@kernel.org> > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/include/hyp/switch.h | 43 +++++++++++++++++++++++++ > > arch/arm64/kvm/hyp/nvhe/switch.c | 33 +++++++++++++++++++ > > 2 files changed, 76 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index a0e78a6027be..5a2b89b96c67 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -409,6 +409,46 @@ static inline bool __hyp_handle_ptrauth(struct kvm_vcpu *vcpu) > > return true; > > } > > > > +typedef int (*exit_handle_fn)(struct kvm_vcpu *); > > This returns an int... > > > + > > +exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu); > > + > > +static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu) > > +{ > > + return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL; > > +} > > + > > +/* > > + * Allow the hypervisor to handle the exit with an exit handler if it has one. > > + * > > + * Returns true if the hypervisor handled the exit, and control should go back > > + * to the guest, or false if it hasn't. > > + */ > > +static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu) > > +{ > > + bool is_handled = false; > > ... which you then implicitly cast as a bool. > > > + exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu); > > + > > + if (exit_handler) { > > + /* > > + * There's limited vcpu context here since it's not synced yet. > > + * Ensure that relevant vcpu context that might be used by the > > + * exit_handler is in sync before it's called and if handled. > > + */ > > + *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR); > > + *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR); > > + > > + is_handled = exit_handler(vcpu); > > What does 'is_handled' mean here? By definition, any trap *must* be > handled, one way or another. By the look of it, what you really mean > is something like "I have updated the vcpu state and you'd better > reload it". Is that what it means? > > > + > > + if (is_handled) { > > + write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); > > + write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR); > > + } > > + } > > + > > + return is_handled; > > +} > > All these functions really should be marked inline. Have you checked > how this expands on VHE? I think some compilers could be pretty > unhappy about the undefined symbol in kvm_get_hyp_exit_handler(). > > It is also unfortunate that we get a bunch of tests for various > flavours of traps (FP, PAuth, page faults...), only to hit yet another > decoding tree. Is there a way we could use this infrastructure for > everything? I realised that I wasn't very forthcoming here. I've decided to put the code where my mouth is and pushed out a branch [1] with your first 10 patches, followed by my own take on this particular problem. It compiles, and even managed to boot a Debian guest on a nVHE box. As you can see, most of the early exit handling is now moved to specific handlers, unifying the handling. For the protected mode, you can provide your own handler array (just hack kvm_get_exit_handler_array() to return something else), which will do the right thing as long as you call into the existing handlers first. When it comes to the ELR/SPSR handling, it is better left to the individual handlers (which we already do in some cases, see how we skip instructions, for example). Please let me know what you think. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pkvm-fixed-features
Hi Marc, On Thu, Aug 19, 2021 at 3:36 PM Marc Zyngier <maz@kernel.org> wrote: > > Hi Fuad, > > On Wed, 18 Aug 2021 17:45:50 +0100, > Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 17 Aug 2021 09:11:30 +0100, > > Fuad Tabba <tabba@google.com> wrote: > > > > > > Add an array of pointers to handlers for various trap reasons in > > > nVHE code. > > > > > > The current code selects how to fixup a guest on exit based on a > > > series of if/else statements. Future patches will also require > > > different handling for guest exists. Create an array of handlers > > > to consolidate them. > > > > > > No functional change intended as the array isn't populated yet. > > > > > > Acked-by: Will Deacon <will@kernel.org> > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > > --- > > > arch/arm64/kvm/hyp/include/hyp/switch.h | 43 +++++++++++++++++++++++++ > > > arch/arm64/kvm/hyp/nvhe/switch.c | 33 +++++++++++++++++++ > > > 2 files changed, 76 insertions(+) > > > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > > index a0e78a6027be..5a2b89b96c67 100644 > > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > > @@ -409,6 +409,46 @@ static inline bool __hyp_handle_ptrauth(struct kvm_vcpu *vcpu) > > > return true; > > > } > > > > > > +typedef int (*exit_handle_fn)(struct kvm_vcpu *); > > > > This returns an int... > > > > > + > > > +exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu); > > > + > > > +static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu) > > > +{ > > > + return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL; > > > +} > > > + > > > +/* > > > + * Allow the hypervisor to handle the exit with an exit handler if it has one. > > > + * > > > + * Returns true if the hypervisor handled the exit, and control should go back > > > + * to the guest, or false if it hasn't. > > > + */ > > > +static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu) > > > +{ > > > + bool is_handled = false; > > > > ... which you then implicitly cast as a bool. > > > > > + exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu); > > > + > > > + if (exit_handler) { > > > + /* > > > + * There's limited vcpu context here since it's not synced yet. > > > + * Ensure that relevant vcpu context that might be used by the > > > + * exit_handler is in sync before it's called and if handled. > > > + */ > > > + *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR); > > > + *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR); > > > + > > > + is_handled = exit_handler(vcpu); > > > > What does 'is_handled' mean here? By definition, any trap *must* be > > handled, one way or another. By the look of it, what you really mean > > is something like "I have updated the vcpu state and you'd better > > reload it". Is that what it means? > > > > > + > > > + if (is_handled) { > > > + write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); > > > + write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR); > > > + } > > > + } > > > + > > > + return is_handled; > > > +} > > > > All these functions really should be marked inline. Have you checked > > how this expands on VHE? I think some compilers could be pretty > > unhappy about the undefined symbol in kvm_get_hyp_exit_handler(). > > > > It is also unfortunate that we get a bunch of tests for various > > flavours of traps (FP, PAuth, page faults...), only to hit yet another > > decoding tree. Is there a way we could use this infrastructure for > > everything? > > I realised that I wasn't very forthcoming here. I've decided to put > the code where my mouth is and pushed out a branch [1] with your first > 10 patches, followed by my own take on this particular problem. It > compiles, and even managed to boot a Debian guest on a nVHE box. > > As you can see, most of the early exit handling is now moved to > specific handlers, unifying the handling. For the protected mode, you > can provide your own handler array (just hack > kvm_get_exit_handler_array() to return something else), which will do > the right thing as long as you call into the existing handlers first. > When it comes to the ELR/SPSR handling, it is better left to the > individual handlers (which we already do in some cases, see how we > skip instructions, for example). > Please let me know what you think. Thanks a lot for this and sorry for being late to reply. I've been travelling. I think that your proposal looks great. All handling is consolidated now and handling for protected VMs can just be added on top. There are some small issues with what parameters we need (e.g., passing struct kvm to kvm_get_exit_handler_array), but I will sort them out and submit them in the next round. Cheers, /fuad > Thanks, > > M. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pkvm-fixed-features > > -- > Without deviation from the norm, progress is not possible.
Hi Fuad, On Mon, 23 Aug 2021 11:21:05 +0100, Fuad Tabba <tabba@google.com> wrote: > > Hi Marc, > > On Thu, Aug 19, 2021 at 3:36 PM Marc Zyngier <maz@kernel.org> wrote: > > I realised that I wasn't very forthcoming here. I've decided to put > > the code where my mouth is and pushed out a branch [1] with your first > > 10 patches, followed by my own take on this particular problem. It > > compiles, and even managed to boot a Debian guest on a nVHE box. > > > > As you can see, most of the early exit handling is now moved to > > specific handlers, unifying the handling. For the protected mode, you > > can provide your own handler array (just hack > > kvm_get_exit_handler_array() to return something else), which will do > > the right thing as long as you call into the existing handlers first. > > When it comes to the ELR/SPSR handling, it is better left to the > > individual handlers (which we already do in some cases, see how we > > skip instructions, for example). > > Please let me know what you think. > > Thanks a lot for this and sorry for being late to reply. I've been > travelling. No worries, it should be me who apologies for getting to this that late. > I think that your proposal looks great. All handling is consolidated > now and handling for protected VMs can just be added on top. There are > some small issues with what parameters we need (e.g., passing struct > kvm to kvm_get_exit_handler_array), but I will sort them out and > submit them in the next round. OK. Please base these changes on top of the three patches in my branch, which I will update with actual commit messages. Thanks, M.
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index a0e78a6027be..5a2b89b96c67 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -409,6 +409,46 @@ static inline bool __hyp_handle_ptrauth(struct kvm_vcpu *vcpu) return true; } +typedef int (*exit_handle_fn)(struct kvm_vcpu *); + +exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu); + +static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu) +{ + return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL; +} + +/* + * Allow the hypervisor to handle the exit with an exit handler if it has one. + * + * Returns true if the hypervisor handled the exit, and control should go back + * to the guest, or false if it hasn't. + */ +static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu) +{ + bool is_handled = false; + exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu); + + if (exit_handler) { + /* + * There's limited vcpu context here since it's not synced yet. + * Ensure that relevant vcpu context that might be used by the + * exit_handler is in sync before it's called and if handled. + */ + *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR); + *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR); + + is_handled = exit_handler(vcpu); + + if (is_handled) { + write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); + write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR); + } + } + + return is_handled; +} + /* * Return true when we were able to fixup the guest exit and should return to * the guest, false when we should restore the host state and return to the @@ -496,6 +536,9 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) goto guest; } + /* Check if there's an exit handler and allow it to handle the exit. */ + if (kvm_hyp_handle_exit(vcpu)) + goto guest; exit: /* Return to the host kernel and handle the exit */ return false; diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 86f3d6482935..b7f25307a7b9 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -158,6 +158,39 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) write_sysreg(pmu->events_host, pmcntenset_el0); } +static exit_handle_fn hyp_exit_handlers[] = { + [0 ... ESR_ELx_EC_MAX] = NULL, + [ESR_ELx_EC_WFx] = NULL, + [ESR_ELx_EC_CP15_32] = NULL, + [ESR_ELx_EC_CP15_64] = NULL, + [ESR_ELx_EC_CP14_MR] = NULL, + [ESR_ELx_EC_CP14_LS] = NULL, + [ESR_ELx_EC_CP14_64] = NULL, + [ESR_ELx_EC_HVC32] = NULL, + [ESR_ELx_EC_SMC32] = NULL, + [ESR_ELx_EC_HVC64] = NULL, + [ESR_ELx_EC_SMC64] = NULL, + [ESR_ELx_EC_SYS64] = NULL, + [ESR_ELx_EC_SVE] = NULL, + [ESR_ELx_EC_IABT_LOW] = NULL, + [ESR_ELx_EC_DABT_LOW] = NULL, + [ESR_ELx_EC_SOFTSTP_LOW] = NULL, + [ESR_ELx_EC_WATCHPT_LOW] = NULL, + [ESR_ELx_EC_BREAKPT_LOW] = NULL, + [ESR_ELx_EC_BKPT32] = NULL, + [ESR_ELx_EC_BRK64] = NULL, + [ESR_ELx_EC_FP_ASIMD] = NULL, + [ESR_ELx_EC_PAC] = NULL, +}; + +exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu) +{ + u32 esr = kvm_vcpu_get_esr(vcpu); + u8 esr_ec = ESR_ELx_EC(esr); + + return hyp_exit_handlers[esr_ec]; +} + /* Switch to the guest for legacy non-VHE systems */ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) {