| Message ID | 20250214150258.464798-2-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fix initializing HCRX_EL2 and other traps in pKVM | expand |
Hi Fuad, Series LGTM overall, one comment: On Fri, Feb 14, 2025 at 03:02:56PM +0000, Fuad Tabba wrote: > Initialize and set the traps controlled by the HCRX_EL2 in pKVM > when the register is supported by the system. > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/kvm/hyp/nvhe/pkvm.c | 46 ++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c > index 3927fe52a3dd..668ebec27f1b 100644 > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c > @@ -58,6 +58,30 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu) > vcpu->arch.hcr_el2 |= HCR_ATA; > } > > +static void pkvm_vcpu_reset_hcrx(struct pkvm_hyp_vcpu *hyp_vcpu) > +{ > + struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > + struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu; > + > + if (!cpus_have_final_cap(ARM64_HAS_HCX)) > + return; > + > + /* > + * In general, all HCRX_EL2 bits are gated by a feature. > + * The only reason we can set SMPME without checking any > + * feature is that its effects are not directly observable > + * from the guest. > + */ > + vcpu->arch.hcrx_el2 = HCRX_EL2_SMPME; > + The comment isn't wrong, but we don't support SME at all in KVM at this point. Any objection to dropping this bit? I can fix it when applying the series, no need to respin. Thanks, Oliver
On Wed, 26 Feb 2025 10:07:56 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Fuad, > > Series LGTM overall, one comment: > > On Fri, Feb 14, 2025 at 03:02:56PM +0000, Fuad Tabba wrote: > > Initialize and set the traps controlled by the HCRX_EL2 in pKVM > > when the register is supported by the system. > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/pkvm.c | 46 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > index 3927fe52a3dd..668ebec27f1b 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c > > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > @@ -58,6 +58,30 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu) > > vcpu->arch.hcr_el2 |= HCR_ATA; > > } > > > > +static void pkvm_vcpu_reset_hcrx(struct pkvm_hyp_vcpu *hyp_vcpu) > > +{ > > + struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > > + struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu; > > + > > + if (!cpus_have_final_cap(ARM64_HAS_HCX)) > > + return; > > + > > + /* > > + * In general, all HCRX_EL2 bits are gated by a feature. > > + * The only reason we can set SMPME without checking any > > + * feature is that its effects are not directly observable > > + * from the guest. > > + */ > > + vcpu->arch.hcrx_el2 = HCRX_EL2_SMPME; > > + > > The comment isn't wrong, but we don't support SME at all in KVM at this > point. This is a copy/paste of what we have in kvm_calculate_traps(), and the result of the removal of the dreaded HCRX_GUEST_FLAGS. > Any objection to dropping this bit? I can fix it when applying the > series, no need to respin. Whatever we do, I think we should keep the two side of the trap configuration in sync, as this is otherwise a cause of bugs. Thanks, M.
Hi Oliver, On Wed, 26 Feb 2025 at 02:08, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hi Fuad, > > Series LGTM overall, one comment: > > On Fri, Feb 14, 2025 at 03:02:56PM +0000, Fuad Tabba wrote: > > Initialize and set the traps controlled by the HCRX_EL2 in pKVM > > when the register is supported by the system. > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/pkvm.c | 46 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > index 3927fe52a3dd..668ebec27f1b 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c > > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > @@ -58,6 +58,30 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu) > > vcpu->arch.hcr_el2 |= HCR_ATA; > > } > > > > +static void pkvm_vcpu_reset_hcrx(struct pkvm_hyp_vcpu *hyp_vcpu) > > +{ > > + struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > > + struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu; > > + > > + if (!cpus_have_final_cap(ARM64_HAS_HCX)) > > + return; > > + > > + /* > > + * In general, all HCRX_EL2 bits are gated by a feature. > > + * The only reason we can set SMPME without checking any > > + * feature is that its effects are not directly observable > > + * from the guest. > > + */ > > + vcpu->arch.hcrx_el2 = HCRX_EL2_SMPME; > > + > > The comment isn't wrong, but we don't support SME at all in KVM at this > point. > > Any objection to dropping this bit? I can fix it when applying the > series, no need to respin. No objections to dropping it. Regarding respinning though, it depends on what I need to do to address Marc's comment. On that in my reply to his email. Thanks, /fuad > Thanks, > Oliver
Hi Marc, On Wed, 26 Feb 2025 at 02:45, Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 26 Feb 2025 10:07:56 +0000, > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > Hi Fuad, > > > > Series LGTM overall, one comment: > > > > On Fri, Feb 14, 2025 at 03:02:56PM +0000, Fuad Tabba wrote: > > > Initialize and set the traps controlled by the HCRX_EL2 in pKVM > > > when the register is supported by the system. > > > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > > --- > > > arch/arm64/kvm/hyp/nvhe/pkvm.c | 46 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 46 insertions(+) > > > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > > index 3927fe52a3dd..668ebec27f1b 100644 > > > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c > > > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > > @@ -58,6 +58,30 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu) > > > vcpu->arch.hcr_el2 |= HCR_ATA; > > > } > > > > > > +static void pkvm_vcpu_reset_hcrx(struct pkvm_hyp_vcpu *hyp_vcpu) > > > +{ > > > + struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > > > + struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu; > > > + > > > + if (!cpus_have_final_cap(ARM64_HAS_HCX)) > > > + return; > > > + > > > + /* > > > + * In general, all HCRX_EL2 bits are gated by a feature. > > > + * The only reason we can set SMPME without checking any > > > + * feature is that its effects are not directly observable > > > + * from the guest. > > > + */ > > > + vcpu->arch.hcrx_el2 = HCRX_EL2_SMPME; > > > + > > > > The comment isn't wrong, but we don't support SME at all in KVM at this > > point. > > This is a copy/paste of what we have in kvm_calculate_traps(), and the > result of the removal of the dreaded HCRX_GUEST_FLAGS. > > > Any objection to dropping this bit? I can fix it when applying the > > series, no need to respin. > > Whatever we do, I think we should keep the two side of the trap > configuration in sync, as this is otherwise a cause of bugs. I could take the initialization of hcrx part from kvm_calculate_traps() and place it in an inline function in kvm_emulate.h. This then would be called by kvm_calculate_traps() and pkvm_vcpu_init_traps(). What do you think? Thanks, /fuad > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. >
On Wed, 26 Feb 2025 12:44:49 +0000, Fuad Tabba <tabba@google.com> wrote: > > Hi Marc, > > On Wed, 26 Feb 2025 at 02:45, Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 26 Feb 2025 10:07:56 +0000, > > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > > > Hi Fuad, > > > > > > Series LGTM overall, one comment: > > > > > > On Fri, Feb 14, 2025 at 03:02:56PM +0000, Fuad Tabba wrote: > > > > Initialize and set the traps controlled by the HCRX_EL2 in pKVM > > > > when the register is supported by the system. > > > > > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > > > --- > > > > arch/arm64/kvm/hyp/nvhe/pkvm.c | 46 ++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 46 insertions(+) > > > > > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > > > index 3927fe52a3dd..668ebec27f1b 100644 > > > > --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c > > > > +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c > > > > @@ -58,6 +58,30 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu) > > > > vcpu->arch.hcr_el2 |= HCR_ATA; > > > > } > > > > > > > > +static void pkvm_vcpu_reset_hcrx(struct pkvm_hyp_vcpu *hyp_vcpu) > > > > +{ > > > > + struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; > > > > + struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu; > > > > + > > > > + if (!cpus_have_final_cap(ARM64_HAS_HCX)) > > > > + return; > > > > + > > > > + /* > > > > + * In general, all HCRX_EL2 bits are gated by a feature. > > > > + * The only reason we can set SMPME without checking any > > > > + * feature is that its effects are not directly observable > > > > + * from the guest. > > > > + */ > > > > + vcpu->arch.hcrx_el2 = HCRX_EL2_SMPME; > > > > + > > > > > > The comment isn't wrong, but we don't support SME at all in KVM at this > > > point. > > > > This is a copy/paste of what we have in kvm_calculate_traps(), and the > > result of the removal of the dreaded HCRX_GUEST_FLAGS. > > > > > Any objection to dropping this bit? I can fix it when applying the > > > series, no need to respin. > > > > Whatever we do, I think we should keep the two side of the trap > > configuration in sync, as this is otherwise a cause of bugs. > > I could take the initialization of hcrx part from > kvm_calculate_traps() and place it in an inline function in > kvm_emulate.h. This then would be called by kvm_calculate_traps() and > pkvm_vcpu_init_traps(). > > What do you think? That could be a good option indeed, irrespective of the fate of SMPME. Thanks, M.
Hey, On Wed, Feb 26, 2025 at 03:28:22PM +0000, Marc Zyngier wrote: > On Wed, 26 Feb 2025 12:44:49 +0000, Fuad Tabba <tabba@google.com> wrote: > > On Wed, 26 Feb 2025 at 02:45, Marc Zyngier <maz@kernel.org> wrote: > > > This is a copy/paste of what we have in kvm_calculate_traps(), and the > > > result of the removal of the dreaded HCRX_GUEST_FLAGS. Ah, I missed that. We probably should've just tossed it at that point, doing configuration for features we don't support is a bit confusing. > > > > Any objection to dropping this bit? I can fix it when applying the > > > > series, no need to respin. > > > > > > Whatever we do, I think we should keep the two side of the trap > > > configuration in sync, as this is otherwise a cause of bugs. > > > > I could take the initialization of hcrx part from > > kvm_calculate_traps() and place it in an inline function in > > kvm_emulate.h. This then would be called by kvm_calculate_traps() and > > pkvm_vcpu_init_traps(). > > > > What do you think? > > That could be a good option indeed, irrespective of the fate of SMPME. > Agreed. Fuad, would you mind taking another pass at this with the shared helper? Thanks, Oliver
Hi, On Wed, 26 Feb 2025 at 10:53, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hey, > > On Wed, Feb 26, 2025 at 03:28:22PM +0000, Marc Zyngier wrote: > > On Wed, 26 Feb 2025 12:44:49 +0000, Fuad Tabba <tabba@google.com> wrote: > > > On Wed, 26 Feb 2025 at 02:45, Marc Zyngier <maz@kernel.org> wrote: > > > > This is a copy/paste of what we have in kvm_calculate_traps(), and the > > > > result of the removal of the dreaded HCRX_GUEST_FLAGS. > > Ah, I missed that. > > We probably should've just tossed it at that point, doing configuration > for features we don't support is a bit confusing. > > > > > > Any objection to dropping this bit? I can fix it when applying the > > > > > series, no need to respin. > > > > > > > > Whatever we do, I think we should keep the two side of the trap > > > > configuration in sync, as this is otherwise a cause of bugs. > > > > > > I could take the initialization of hcrx part from > > > kvm_calculate_traps() and place it in an inline function in > > > kvm_emulate.h. This then would be called by kvm_calculate_traps() and > > > pkvm_vcpu_init_traps(). > > > > > > What do you think? > > > > That could be a good option indeed, irrespective of the fate of SMPME. > > > > Agreed. Fuad, would you mind taking another pass at this with the shared > helper? I'm on it! Cheers, /fuad > Thanks, > Oliver >
diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c index 3927fe52a3dd..668ebec27f1b 100644 --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c @@ -58,6 +58,30 @@ static void pkvm_vcpu_reset_hcr(struct kvm_vcpu *vcpu) vcpu->arch.hcr_el2 |= HCR_ATA; } +static void pkvm_vcpu_reset_hcrx(struct pkvm_hyp_vcpu *hyp_vcpu) +{ + struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu; + struct kvm_vcpu *vcpu = &hyp_vcpu->vcpu; + + if (!cpus_have_final_cap(ARM64_HAS_HCX)) + return; + + /* + * In general, all HCRX_EL2 bits are gated by a feature. + * The only reason we can set SMPME without checking any + * feature is that its effects are not directly observable + * from the guest. + */ + vcpu->arch.hcrx_el2 = HCRX_EL2_SMPME; + + /* + * For non-protected VMs, the host is responsible for the guest's + * features, so use the remaining host HCRX_EL2 bits. + */ + if ((!pkvm_hyp_vcpu_is_protected(hyp_vcpu))) + vcpu->arch.hcrx_el2 |= host_vcpu->arch.hcrx_el2; +} + static void pvm_init_traps_hcr(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -92,6 +116,26 @@ static void pvm_init_traps_hcr(struct kvm_vcpu *vcpu) vcpu->arch.hcr_el2 = val; } +static void pvm_init_traps_hcrx(struct kvm_vcpu *vcpu) +{ + struct kvm *kvm = vcpu->kvm; + u64 hcrx_set = 0; + + if (!cpus_have_final_cap(ARM64_HAS_HCX)) + return; + + if (kvm_has_feat(kvm, ID_AA64ISAR2_EL1, MOPS, IMP)) + hcrx_set |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2); + + if (kvm_has_feat(kvm, ID_AA64MMFR3_EL1, TCRX, IMP)) + hcrx_set |= HCRX_EL2_TCR2En; + + if (kvm_has_fpmr(kvm)) + hcrx_set |= HCRX_EL2_EnFPM; + + vcpu->arch.hcrx_el2 |= hcrx_set; +} + static void pvm_init_traps_mdcr(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -165,6 +209,7 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu) vcpu->arch.mdcr_el2 = 0; pkvm_vcpu_reset_hcr(vcpu); + pkvm_vcpu_reset_hcrx(hyp_vcpu); if ((!pkvm_hyp_vcpu_is_protected(hyp_vcpu))) return 0; @@ -174,6 +219,7 @@ static int pkvm_vcpu_init_traps(struct pkvm_hyp_vcpu *hyp_vcpu) return ret; pvm_init_traps_hcr(vcpu); + pvm_init_traps_hcrx(vcpu); pvm_init_traps_mdcr(vcpu); return 0;
Initialize and set the traps controlled by the HCRX_EL2 in pKVM when the register is supported by the system. Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/kvm/hyp/nvhe/pkvm.c | 46 ++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)