| Message ID | 20191220143025.33853-12-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: KVM: add SPE profiling support | expand |
On Fri, Dec 20, 2019 at 02:30:18PM +0000, Andrew Murray wrote: > As we now save/restore the profiler state there is no need to trap > accesses to the statistical profiling controls. Let's unset the > _TPMS bit. > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > --- > arch/arm64/kvm/debug.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index 43487f035385..07ca783e7d9e 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) > * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > * - Debug ROM Address (MDCR_EL2_TDRA) > * - OS related registers (MDCR_EL2_TDOSA) > - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) > * > * Additionally, KVM only traps guest accesses to the debug registers if > * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY > @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > */ > vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK; > vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | > - MDCR_EL2_TPMS | > MDCR_EL2_TPMCR | > MDCR_EL2_TDRA | > MDCR_EL2_TDOSA); I think that this should be conditional on some vcpu feature flag. If nothing else, this could break existing migration cases otherwise. Thanks, Mark.
On Fri, 20 Dec 2019 14:30:18 +0000, Andrew Murray <andrew.murray@arm.com> wrote: > > As we now save/restore the profiler state there is no need to trap > accesses to the statistical profiling controls. Let's unset the > _TPMS bit. > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > --- > arch/arm64/kvm/debug.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index 43487f035385..07ca783e7d9e 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) > * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > * - Debug ROM Address (MDCR_EL2_TDRA) > * - OS related registers (MDCR_EL2_TDOSA) > - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) > * > * Additionally, KVM only traps guest accesses to the debug registers if > * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY > @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > */ > vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK; > vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | > - MDCR_EL2_TPMS | No. This is an *optional* feature (the guest could not be presented with the SPE feature, or the the support simply not be compiled in). If the guest is not allowed to see the feature, for whichever reason, the traps *must* be enabled and handled. M.
On Sun, Dec 22, 2019 at 10:42:05AM +0000, Marc Zyngier wrote: > On Fri, 20 Dec 2019 14:30:18 +0000, > Andrew Murray <andrew.murray@arm.com> wrote: > > > > As we now save/restore the profiler state there is no need to trap > > accesses to the statistical profiling controls. Let's unset the > > _TPMS bit. > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > --- > > arch/arm64/kvm/debug.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > index 43487f035385..07ca783e7d9e 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) > > * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > > * - Debug ROM Address (MDCR_EL2_TDRA) > > * - OS related registers (MDCR_EL2_TDOSA) > > - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) > > * > > * Additionally, KVM only traps guest accesses to the debug registers if > > * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY > > @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > */ > > vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK; > > vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | > > - MDCR_EL2_TPMS | > > No. This is an *optional* feature (the guest could not be presented > with the SPE feature, or the the support simply not be compiled in). > > If the guest is not allowed to see the feature, for whichever reason, > the traps *must* be enabled and handled. I'll update this (and similar) to trap such registers when we don't support SPE in the guest. My original concern in the cover letter was in how to prevent the guest from attempting to use these registers in the first place - I think the solution I was looking for is to trap-and-emulate ID_AA64DFR0_EL1 such that the PMSVer bits indicate that SPE is not emulated. Thanks, Andrew Murray > > M. > > -- > Jazz is not dead, it just smells funny.
On 2019-12-23 11:56, Andrew Murray wrote: > On Sun, Dec 22, 2019 at 10:42:05AM +0000, Marc Zyngier wrote: >> On Fri, 20 Dec 2019 14:30:18 +0000, >> Andrew Murray <andrew.murray@arm.com> wrote: >> > >> > As we now save/restore the profiler state there is no need to trap >> > accesses to the statistical profiling controls. Let's unset the >> > _TPMS bit. >> > >> > Signed-off-by: Andrew Murray <andrew.murray@arm.com> >> > --- >> > arch/arm64/kvm/debug.c | 2 -- >> > 1 file changed, 2 deletions(-) >> > >> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c >> > index 43487f035385..07ca783e7d9e 100644 >> > --- a/arch/arm64/kvm/debug.c >> > +++ b/arch/arm64/kvm/debug.c >> > @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu >> *vcpu) >> > * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) >> > * - Debug ROM Address (MDCR_EL2_TDRA) >> > * - OS related registers (MDCR_EL2_TDOSA) >> > - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) >> > * >> > * Additionally, KVM only traps guest accesses to the debug >> registers if >> > * the guest is not actively using them (see the >> KVM_ARM64_DEBUG_DIRTY >> > @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu >> *vcpu) >> > */ >> > vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & >> MDCR_EL2_HPMN_MASK; >> > vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | >> > - MDCR_EL2_TPMS | >> >> No. This is an *optional* feature (the guest could not be presented >> with the SPE feature, or the the support simply not be compiled in). >> >> If the guest is not allowed to see the feature, for whichever >> reason, >> the traps *must* be enabled and handled. > > I'll update this (and similar) to trap such registers when we don't > support > SPE in the guest. > > My original concern in the cover letter was in how to prevent the > guest > from attempting to use these registers in the first place - I think > the > solution I was looking for is to trap-and-emulate ID_AA64DFR0_EL1 > such that > the PMSVer bits indicate that SPE is not emulated. That, and active trapping of the SPE system registers resulting in injection of an UNDEF into the offending guest. Thanks, M.
On Mon, Dec 23, 2019 at 12:05:12PM +0000, Marc Zyngier wrote: > On 2019-12-23 11:56, Andrew Murray wrote: > > On Sun, Dec 22, 2019 at 10:42:05AM +0000, Marc Zyngier wrote: > > > On Fri, 20 Dec 2019 14:30:18 +0000, > > > Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > > > As we now save/restore the profiler state there is no need to trap > > > > accesses to the statistical profiling controls. Let's unset the > > > > _TPMS bit. > > > > > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > > > --- > > > > arch/arm64/kvm/debug.c | 2 -- > > > > 1 file changed, 2 deletions(-) > > > > > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > > > index 43487f035385..07ca783e7d9e 100644 > > > > --- a/arch/arm64/kvm/debug.c > > > > +++ b/arch/arm64/kvm/debug.c > > > > @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu > > > *vcpu) > > > > * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > > > > * - Debug ROM Address (MDCR_EL2_TDRA) > > > > * - OS related registers (MDCR_EL2_TDOSA) > > > > - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) > > > > * > > > > * Additionally, KVM only traps guest accesses to the debug > > > registers if > > > > * the guest is not actively using them (see the > > > KVM_ARM64_DEBUG_DIRTY > > > > @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu > > > *vcpu) > > > > */ > > > > vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & > > > MDCR_EL2_HPMN_MASK; > > > > vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | > > > > - MDCR_EL2_TPMS | > > > > > > No. This is an *optional* feature (the guest could not be presented > > > with the SPE feature, or the the support simply not be compiled in). > > > > > > If the guest is not allowed to see the feature, for whichever > > > reason, > > > the traps *must* be enabled and handled. > > > > I'll update this (and similar) to trap such registers when we don't > > support > > SPE in the guest. > > > > My original concern in the cover letter was in how to prevent the guest > > from attempting to use these registers in the first place - I think the > > solution I was looking for is to trap-and-emulate ID_AA64DFR0_EL1 such > > that > > the PMSVer bits indicate that SPE is not emulated. > > That, and active trapping of the SPE system registers resulting in injection > of an UNDEF into the offending guest. Yes that's no problem. Thanks, Andrew Murray > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On Mon, Dec 23, 2019 at 12:10:42PM +0000, Andrew Murray wrote: > On Mon, Dec 23, 2019 at 12:05:12PM +0000, Marc Zyngier wrote: > > On 2019-12-23 11:56, Andrew Murray wrote: > > > On Sun, Dec 22, 2019 at 10:42:05AM +0000, Marc Zyngier wrote: > > > > On Fri, 20 Dec 2019 14:30:18 +0000, > > > > Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > > > > > As we now save/restore the profiler state there is no need to trap > > > > > accesses to the statistical profiling controls. Let's unset the > > > > > _TPMS bit. > > > > > > > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > > > > --- > > > > > arch/arm64/kvm/debug.c | 2 -- > > > > > 1 file changed, 2 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > > > > index 43487f035385..07ca783e7d9e 100644 > > > > > --- a/arch/arm64/kvm/debug.c > > > > > +++ b/arch/arm64/kvm/debug.c > > > > > @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu > > > > *vcpu) > > > > > * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) > > > > > * - Debug ROM Address (MDCR_EL2_TDRA) > > > > > * - OS related registers (MDCR_EL2_TDOSA) > > > > > - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) > > > > > * > > > > > * Additionally, KVM only traps guest accesses to the debug > > > > registers if > > > > > * the guest is not actively using them (see the > > > > KVM_ARM64_DEBUG_DIRTY > > > > > @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu > > > > *vcpu) > > > > > */ > > > > > vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & > > > > MDCR_EL2_HPMN_MASK; > > > > > vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | > > > > > - MDCR_EL2_TPMS | > > > > > > > > No. This is an *optional* feature (the guest could not be presented > > > > with the SPE feature, or the the support simply not be compiled in). > > > > > > > > If the guest is not allowed to see the feature, for whichever > > > > reason, > > > > the traps *must* be enabled and handled. > > > > > > I'll update this (and similar) to trap such registers when we don't > > > support > > > SPE in the guest. > > > > > > My original concern in the cover letter was in how to prevent the guest > > > from attempting to use these registers in the first place - I think the > > > solution I was looking for is to trap-and-emulate ID_AA64DFR0_EL1 such > > > that > > > the PMSVer bits indicate that SPE is not emulated. > > > > That, and active trapping of the SPE system registers resulting in injection > > of an UNDEF into the offending guest. > > Yes that's no problem. The spec says that 'direct access to [these registers] are UNDEFINED' - is it not more correct to handle this with trap_raz_wi than an undefined instruction? Thanks, Andrew Murray > > Thanks, > > Andrew Murray > > > > > Thanks, > > > > M. > > -- > > Jazz is not dead. It just smells funny... > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Hi Andrew, On Thu, Jan 09, 2020 at 05:25:12PM +0000, Andrew Murray wrote: > On Mon, Dec 23, 2019 at 12:10:42PM +0000, Andrew Murray wrote: > > On Mon, Dec 23, 2019 at 12:05:12PM +0000, Marc Zyngier wrote: > > > On 2019-12-23 11:56, Andrew Murray wrote: > > > > My original concern in the cover letter was in how to prevent > > > > the guest from attempting to use these registers in the first > > > > place - I think the solution I was looking for is to > > > > trap-and-emulate ID_AA64DFR0_EL1 such that the PMSVer bits > > > > indicate that SPE is not emulated. > > > > > > That, and active trapping of the SPE system registers resulting in injection > > > of an UNDEF into the offending guest. > > > > Yes that's no problem. > > The spec says that 'direct access to [these registers] are UNDEFINED' - is it > not more correct to handle this with trap_raz_wi than an undefined instruction? The term UNDEFINED specifically means treated as an undefined instruction. The Glossary in ARM DDI 0487E.a says for UNDEFINED: | Indicates cases where an attempt to execute a particular encoding bit | pattern generates an exception, that is taken to the current Exception | level, or to the default Exception level for taking exceptions if the | UNDEFINED encoding was executed at EL0. This applies to: | | * Any encoding that is not allocated to any instruction. | | * Any encoding that is defined as never accessible at the current | Exception level. | | * Some cases where an enable, disable, or trap control means an | encoding is not accessible at the current Exception level. So these should trigger an UNDEFINED exception rather than behaving as RAZ/WI. Thanks, Mark.
On Thu, Jan 09, 2020 at 05:42:51PM +0000, Mark Rutland wrote: > Hi Andrew, > > On Thu, Jan 09, 2020 at 05:25:12PM +0000, Andrew Murray wrote: > > On Mon, Dec 23, 2019 at 12:10:42PM +0000, Andrew Murray wrote: > > > On Mon, Dec 23, 2019 at 12:05:12PM +0000, Marc Zyngier wrote: > > > > On 2019-12-23 11:56, Andrew Murray wrote: > > > > > My original concern in the cover letter was in how to prevent > > > > > the guest from attempting to use these registers in the first > > > > > place - I think the solution I was looking for is to > > > > > trap-and-emulate ID_AA64DFR0_EL1 such that the PMSVer bits > > > > > indicate that SPE is not emulated. > > > > > > > > That, and active trapping of the SPE system registers resulting in injection > > > > of an UNDEF into the offending guest. > > > > > > Yes that's no problem. > > > > The spec says that 'direct access to [these registers] are UNDEFINED' - is it > > not more correct to handle this with trap_raz_wi than an undefined instruction? > > The term UNDEFINED specifically means treated as an undefined > instruction. The Glossary in ARM DDI 0487E.a says for UNDEFINED: > > | Indicates cases where an attempt to execute a particular encoding bit > | pattern generates an exception, that is taken to the current Exception > | level, or to the default Exception level for taking exceptions if the > | UNDEFINED encoding was executed at EL0. This applies to: > | > | * Any encoding that is not allocated to any instruction. > | > | * Any encoding that is defined as never accessible at the current > | Exception level. > | > | * Some cases where an enable, disable, or trap control means an > | encoding is not accessible at the current Exception level. > > So these should trigger an UNDEFINED exception rather than behaving as > RAZ/WI. OK thanks for the clarification - I'll leave it as an undefined instruction. Thanks, Andrew Murray > > Thanks, > Mark.
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index 43487f035385..07ca783e7d9e 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -88,7 +88,6 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) * - Performance monitors (MDCR_EL2_TPM/MDCR_EL2_TPMCR) * - Debug ROM Address (MDCR_EL2_TDRA) * - OS related registers (MDCR_EL2_TDOSA) - * - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB) * * Additionally, KVM only traps guest accesses to the debug registers if * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY @@ -111,7 +110,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) */ vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK; vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | - MDCR_EL2_TPMS | MDCR_EL2_TPMCR | MDCR_EL2_TDRA | MDCR_EL2_TDOSA);
As we now save/restore the profiler state there is no need to trap accesses to the statistical profiling controls. Let's unset the _TPMS bit. Signed-off-by: Andrew Murray <andrew.murray@arm.com> --- arch/arm64/kvm/debug.c | 2 -- 1 file changed, 2 deletions(-)