Re: [PATCH v3 04/41] KVM: arm64: Rework hyp_panic for VHE and non-VHE
On 05/02/18 18:04, Julien Grall wrote:
On 12/01/18 12:07, Christoffer Dall wrote:
@@ -436,37 +446,31 @@ static void __hyp_text __hyp_call_panic_nvhe(u64
spsr, u64 elr, u64 par,
read_sysreg(hpfar_el2), par, vcpu);
}
-static void __hyp_text __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
- struct kvm_vcpu *vcpu)
+static void __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
+ struct kvm_cpu_context *host_ctxt)
{
+ struct kvm_vcpu *vcpu;
+ vcpu = host_ctxt->__hyp_running_vcpu;
+
+ __deactivate_traps(vcpu);
+ __sysreg_restore_host_state(host_ctxt);
I was about to ask why you keep this function around as it does nothing
in VHE case. But I see that this will actually restore some values in a
later patch.
Actually, I just misread the code. Sorry for the noise.
Cheers,
--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 04/41] KVM: arm64: Rework hyp_panic for VHE and non-VHE
Hi Christoffer,
On 12/01/18 12:07, Christoffer Dall wrote:
VHE actually doesn't rely on clearing the VTTBR when returning to the
host kernel, and that is the current key mechanism of hyp_panic to
figure out how to attempt to return to a state good enough to print a
panic statement.
Therefore, we split the hyp_panic function into two functions, a VHE and
a non-VHE, keeping the non-VHE version intact, but changing the VHE
behavior.
The vttbr_el2 check on VHE doesn't really make that much sense, because
the only situation where we can get here on VHE is when the hypervisor
assembly code actually called into hyp_panic, which only happens when
VBAR_EL2 has been set to the KVM exception vectors. On VHE, we can
always safely disable the traps and restore the host registers at this
point, so we simply do that unconditionally and call into the panic
function directly.
Acked-by: Marc Zyngier
Signed-off-by: Christoffer Dall
---
arch/arm64/kvm/hyp/switch.c | 42 +++---
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 6fcb37e220b5..71700ecee308 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -419,10 +419,20 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
static const char __hyp_panic_string[] = "HYP panic:\nPS:%08llx PC:%016llx
ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n";
static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par,
-struct kvm_vcpu *vcpu)
+struct kvm_cpu_context
*__host_ctxt)
{
+ struct kvm_vcpu *vcpu;
unsigned long str_va;
+ vcpu = __host_ctxt->__hyp_running_vcpu;
+
+ if (read_sysreg(vttbr_el2)) {
+ __timer_disable_traps(vcpu);
+ __deactivate_traps(vcpu);
+ __deactivate_vm(vcpu);
+ __sysreg_restore_host_state(__host_ctxt);
+ }
+
/*
* Force the panic string to be loaded from the literal pool,
* making sure it is a kernel address and not a PC-relative
@@ -436,37 +446,31 @@ static void __hyp_text __hyp_call_panic_nvhe(u64 spsr,
u64 elr, u64 par,
read_sysreg(hpfar_el2), par, vcpu);
}
-static void __hyp_text __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
- struct kvm_vcpu *vcpu)
+static void __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
+struct kvm_cpu_context *host_ctxt)
{
+ struct kvm_vcpu *vcpu;
+ vcpu = host_ctxt->__hyp_running_vcpu;
+
+ __deactivate_traps(vcpu);
+ __sysreg_restore_host_state(host_ctxt);
I was about to ask why you keep this function around as it does nothing
in VHE case. But I see that this will actually restore some values in a
later patch.
+
panic(__hyp_panic_string,
spsr, elr,
read_sysreg_el2(esr), read_sysreg_el2(far),
read_sysreg(hpfar_el2), par, vcpu);
}
-static hyp_alternate_select(__hyp_call_panic,
- __hyp_call_panic_nvhe, __hyp_call_panic_vhe,
- ARM64_HAS_VIRT_HOST_EXTN);
Out of interest, any specific rather to remove hyp_alternate_select and
"open-code" it?
-
void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt)
{
- struct kvm_vcpu *vcpu = NULL;
-
u64 spsr = read_sysreg_el2(spsr);
u64 elr = read_sysreg_el2(elr);
u64 par = read_sysreg(par_el1);
- if (read_sysreg(vttbr_el2)) {
- vcpu = host_ctxt->__hyp_running_vcpu;
- __timer_disable_traps(vcpu);
- __deactivate_traps(vcpu);
- __deactivate_vm(vcpu);
- __sysreg_restore_host_state(host_ctxt);
- }
-
- /* Call panic for real */
- __hyp_call_panic()(spsr, elr, par, vcpu);
+ if (!has_vhe())
+ __hyp_call_panic_nvhe(spsr, elr, par, host_ctxt);
+ else
+ __hyp_call_panic_vhe(spsr, elr, par, host_ctxt);
unreachable();
}
Cheers,
--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 03/41] KVM: arm64: Avoid storing the vcpu pointer on the stack
Hi Christoffer, On 12/01/18 12:07, Christoffer Dall wrote: diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 048f5db120f3..6ce0b428a4db 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -350,10 +350,15 @@ int kvm_perf_teardown(void); struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr); +extern void __kvm_set_tpidr_el2(u64 tpidr_el2); NIT: The rest of the file seem to declare prototype without extern. [...] diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index 71bf088f1e4b..612021dce84f 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -135,6 +135,7 @@ int main(void) DEFINE(CPU_FP_REGS, offsetof(struct kvm_regs, fp_regs)); DEFINE(VCPU_FPEXC32_EL2,offsetof(struct kvm_vcpu, arch.ctxt.sys_regs[FPEXC32_EL2])); DEFINE(VCPU_HOST_CONTEXT, offsetof(struct kvm_vcpu, arch.host_cpu_context)); + DEFINE(HOST_CONTEXT_VCPU,offsetof(struct kvm_cpu_context, __hyp_running_vcpu)); #endif #ifdef CONFIG_CPU_PM DEFINE(CPU_SUSPEND_SZ, sizeof(struct cpu_suspend_ctx)); diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S index 9a8ab59e..a360ac6e89e9 100644 --- a/arch/arm64/kvm/hyp/entry.S +++ b/arch/arm64/kvm/hyp/entry.S @@ -62,9 +62,6 @@ ENTRY(__guest_enter) // Store the host regs save_callee_saved_regs x1 - // Store host_ctxt and vcpu for use at exit time - stp x1, x0, [sp, #-16]! - add x18, x0, #VCPU_CONTEXT // Restore guest regs x0-x17 @@ -118,8 +115,7 @@ ENTRY(__guest_exit) // Store the guest regs x19-x29, lr save_callee_saved_regs x1 - // Restore the host_ctxt from the stack - ldr x2, [sp], #16 + get_host_ctxt x2, x3 // Now restore the host regs restore_callee_saved_regs x2 diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S index e4f37b9dd47c..71b4cc92895e 100644 --- a/arch/arm64/kvm/hyp/hyp-entry.S +++ b/arch/arm64/kvm/hyp/hyp-entry.S @@ -56,18 +56,15 @@ ENDPROC(__vhe_hyp_call) el1_sync: // Guest trapped into EL2 stp x0, x1, [sp, #-16]! -alternative_if_not ARM64_HAS_VIRT_HOST_EXTN - mrs x1, esr_el2 -alternative_else - mrs x1, esr_el1 -alternative_endif - lsr x0, x1, #ESR_ELx_EC_SHIFT + mrs x1, vttbr_el2 // If vttbr is valid, this is a trap + cbnzx1, el1_trap// from the guest - cmp x0, #ESR_ELx_EC_HVC64 - b.neel1_trap - - mrs x1, vttbr_el2 // If vttbr is valid, the 64bit guest - cbnzx1, el1_trap// called HVC +#ifdef CONFIG_DEBUG + mrs x0, esr_el2 + lsr x0, x0, #ESR_ELx_EC_SHIFT + cmp x0, #ESR_ELx_EC_HVC64 + b.ne__hyp_panic +#endif FWIW, I noticed that Mark's series about Spectre is also touching this code (see https://patchwork.kernel.org/patch/10190297/). Cheers, -- Julien Grall ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v2 REPOST] arm64: KVM: KVM API extensions for SVE
Hi Dave,
On Fri, Jan 26, 2018 at 05:28:49PM +, Dave Martin wrote:
> New feature KVM_ARM_VCPU_SVE:
>
> * enables exposure of SVE to the guest
>
> * enables visibility of / access to KVM_REG_ARM_SVE_*() via KVM reg
>ioctls. The main purposes of this are a) is to allow userspace to hide
>weird-sized registers that it doesn't understand how to deal with,
>and b) allow SVE to be hidden from the VM so that it can migrate to
>nodes that don't support SVE.
>
>ZCR_EL1 is not specifically hidden, since it is "just a system register"
>and does not have a weird size or semantics etc.
I think you want to hide ZCR_EL1 if SVE is not enabled, since presenting
it to a legacy userspace will prevent migration onto a non-SVE enabled
kernel/machine.
>
>
> Registers:
>
> * A new register size is defined KVM_REG_SIZE_U2048 (which can be
>encoded sensibly using the next unused value for the reg size field
>in the reg ID) (grep KVM_REG_SIZE_).
>
> * Reg IDs for the SVE regs will be defined as "coproc" 0x14
>(i.e., 0x14 << KVM_REG_ARM_COPROC_SHIFT)
>
>KVM_REG_ARM_SVE_Z(n, i) is slice i of Zn (each slice is 2048 bits)
>KVM_REG_ARM_SVE_P(n, i) is slice i of Pn (each slice is 256 bits)
>KVM_REG_ARM_FFR(i) is slice i of FFR (each slice is 256 bits)
>
Just so I understand; slice 0 covers all the SVE state as the SVE spec
is today. Additional slices is something we can use in the future in
case the SVE spec is expanded to allow even larger vector lengths? In
which case, for a hypothetical 4096 bits register, slice 0 of Zn will be
the lower 2048 bits and slice 1 will be the upper 2048 bits?
>The slice sizes allow each register to be read/written in exactly
>one slice for SVE.
>
>Surplus bits (beyond the maximum VL supported by the vcpu) will
>be read-as-zero write-ignore.
This implies that there are some ordering fun to be had between
accessing SVE registers and changing the maximum vector length for the
vcpu. Why not loosen the API slightly and say that anything written to
the surplus bits is not guaranteed to be preserved when read back? (In
other words, we make no guarantees).
>
>Reading/writing surplus slices will probably be forbidden, and the
>surplus slices would not be reported via KVM_GET_REG_LIST.
>(We could make these RAZ/WI too, but I'm not sure if it's worth it,
>or why it would be useful.)
>
>Future extensions to the architecture might grow the registers up
>to 32 slices: this may or may not actually happen, but SVE keeps the
>possibilty open. I've tried to design for it.
>
> * KVM_REG_ARM_SVE_Z(n, 0) bits [127:0] alias Vn in
>KVM_REG_ARM_CORE(fp_regs.v[n]) .. KVM_REG_ARM_CORE(fp_regs.v[n])+3.
nit: KVM_REG_ARM_CORE_REG doesn't specify the size, so I think this
would be more clearly stated as
"aliases KVM_REG_SIZE_U128 | KVM_REG_ARM_CORE(fp_regs.vregs[n])" or as
"aliases kvm_regs.regs.fp_regs.vregs[n]".
>
>It's simplest for userspace if the two views always appear to be
>in sync, but it's unclear whether this is really useful. Perhaps
>this can be relaxed if it's a big deal for the KVM implementation;
>I don't know yet.
It's not going to be a big deal, and it's the only sensible thing to do;
otherwise we'll have no clear semantics of where the values are. If KVM
chooses to duplicate the 128 bits of state in memory, then it must also
know how to syncrhonize that duplicated state when running the guest,
and therefore can also do this in software on SET/GET.
>
>
> Vector length control:
>
> Some means is needed to determine the set of vector lengths visible
> to guest software running on a vcpu.
>
> When a vcpu is created, the set would be defaulted to the maximal set
> that can be supported while permitting each vcpu to run on any host
> CPU. SVE has some virtualisation quirks which mean that this set may
> exclude some vector lengths that are available for host userspace
> applications. The common case should be that the sets are the same
> however.
>
> * New ioctl KVM_ARM_VCPU_{SET,GET}_SVE_VLS to set or retrieve the set of
>vector lengths available to the guest.
>
>Adding random vcpu ioctls
>
>To configure a non-default set of vector lengths,
>KVM_ARM_VCPU_SET_SVE_VLS can be called: this would only be permitted
>before the vcpu is first run.
>
>This is primarily intended for supporting migration, by providing a
>robust check that the destination node will run the vcpu correctly.
>In a cluster with non-uniform SVE implementation across nodes, this
>also allows a specific set of VLs to be requested that the caller
>knows is usable across the whole cluster.
>
>For migration purposes, userspace would need to do
>KVM_ARM_VCPU_GET_SVE_VLS at the origin node and store the returned
>set as VM metadata: on the destination node,
>KVM_ARM_VCPU_SET_SVE_VLS should be used to request that exact set of
>VLs: if the de
Re: [PATCH v3 02/41] KVM: arm/arm64: Move vcpu_load call after kvm_vcpu_first_run_init
Hi Christoffer,
On 12/01/18 12:07, Christoffer Dall wrote:
Moving the call to vcpu_load() in kvm_arch_vcpu_ioctl_run() to after
we've called kvm_vcpu_first_run_init() simplifies some of the vgic and
there is also no need to do vcpu_load() for things such as handling the
immediate_exit flag.
Signed-off-by: Christoffer Dall
Reviewed-by: Julien Grall
Cheers,
---
virt/kvm/arm/arch_timer.c | 7 ---
virt/kvm/arm/arm.c| 22 --
virt/kvm/arm/vgic/vgic-init.c | 11 ---
3 files changed, 8 insertions(+), 32 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index cfcd0323deab..c09c701fd68e 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -834,14 +834,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
return ret;
no_vgic:
- preempt_disable();
timer->enabled = 1;
- if (!irqchip_in_kernel(vcpu->kvm))
- kvm_timer_vcpu_load_user(vcpu);
- else
- kvm_timer_vcpu_load_vgic(vcpu);
- preempt_enable();
-
return 0;
}
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 5e3c149a6e28..360df72692ee 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -631,27 +631,22 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct
kvm_run *run)
if (unlikely(!kvm_vcpu_initialized(vcpu)))
return -ENOEXEC;
- vcpu_load(vcpu);
-
ret = kvm_vcpu_first_run_init(vcpu);
if (ret)
- goto out;
+ return ret;
if (run->exit_reason == KVM_EXIT_MMIO) {
ret = kvm_handle_mmio_return(vcpu, vcpu->run);
if (ret)
- goto out;
- if (kvm_arm_handle_step_debug(vcpu, vcpu->run)) {
- ret = 0;
- goto out;
- }
-
+ return ret;
+ if (kvm_arm_handle_step_debug(vcpu, vcpu->run))
+ return 0;
}
- if (run->immediate_exit) {
- ret = -EINTR;
- goto out;
- }
+ if (run->immediate_exit)
+ return -EINTR;
+
+ vcpu_load(vcpu);
kvm_sigset_activate(vcpu);
@@ -803,7 +798,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
kvm_sigset_deactivate(vcpu);
-out:
vcpu_put(vcpu);
return ret;
}
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 62310122ee78..a0688ef52ad7 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -300,17 +300,6 @@ int vgic_init(struct kvm *kvm)
dist->initialized = true;
- /*
-* If we're initializing GICv2 on-demand when first running the VCPU
-* then we need to load the VGIC state onto the CPU. We can detect
-* this easily by checking if we are in between vcpu_load and vcpu_put
-* when we just initialized the VGIC.
-*/
- preempt_disable();
- vcpu = kvm_arm_get_running_vcpu();
- if (vcpu)
- kvm_vgic_load(vcpu);
- preempt_enable();
out:
return ret;
}
--
Julien Grall
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 41/41] KVM: arm/arm64: Avoid VGICv3 save/restore on VHE with no IRQs
Hi Christoffer,
On 12.01.2018 13:07, Christoffer Dall wrote:
We can finally get completely rid of any calls to the VGICv3
save/restore functions when the AP lists are empty on VHE systems. This
requires carefully factoring out trap configuration from saving and
restoring state, and carefully choosing what to do on the VHE and
non-VHE path.
One of the challenges is that we cannot save/restore the VMCR lazily
because we can only write the VMCR when ICC_SRE_EL1.SRE is cleared when
emulating a GICv2-on-GICv3, since otherwise all Group-0 interrupts end
up being delivered as FIQ.
To solve this problem, and still provide fast performance in the fast
path of exiting a VM when no interrupts are pending (which also
optimized the latency for actually delivering virtual interrupts coming
from physical interrupts), we orchestrate a dance of only doing the
activate/deactivate traps in vgic load/put for VHE systems (which can
have ICC_SRE_EL1.SRE cleared when running in the host), and doing the
configuration on every round-trip on non-VHE systems.
Signed-off-by: Christoffer Dall
---
arch/arm/include/asm/kvm_hyp.h | 2 +
arch/arm/kvm/hyp/switch.c| 8 ++-
arch/arm64/include/asm/kvm_hyp.h | 2 +
arch/arm64/kvm/hyp/switch.c | 8 ++-
virt/kvm/arm/hyp/vgic-v3-sr.c| 121 +--
virt/kvm/arm/vgic/vgic-v3.c | 6 ++
virt/kvm/arm/vgic/vgic.c | 7 +--
7 files changed, 103 insertions(+), 51 deletions(-)
diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
index b3dd4f4304f5..d01676e5b816 100644
--- a/arch/arm/include/asm/kvm_hyp.h
+++ b/arch/arm/include/asm/kvm_hyp.h
@@ -109,6 +109,8 @@ void __sysreg_restore_state(struct kvm_cpu_context *ctxt);
void __vgic_v3_save_state(struct kvm_vcpu *vcpu);
void __vgic_v3_restore_state(struct kvm_vcpu *vcpu);
+void __vgic_v3_activate_traps(struct kvm_vcpu *vcpu);
+void __vgic_v3_deactivate_traps(struct kvm_vcpu *vcpu);
void __vgic_v3_save_aprs(struct kvm_vcpu *vcpu);
void __vgic_v3_restore_aprs(struct kvm_vcpu *vcpu);
diff --git a/arch/arm/kvm/hyp/switch.c b/arch/arm/kvm/hyp/switch.c
index 214187446e63..337c76230885 100644
--- a/arch/arm/kvm/hyp/switch.c
+++ b/arch/arm/kvm/hyp/switch.c
@@ -89,14 +89,18 @@ static void __hyp_text __deactivate_vm(struct kvm_vcpu
*vcpu)
static void __hyp_text __vgic_save_state(struct kvm_vcpu *vcpu)
{
- if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
+ if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) {
__vgic_v3_save_state(vcpu);
+ __vgic_v3_deactivate_traps(vcpu);
+ }
}
static void __hyp_text __vgic_restore_state(struct kvm_vcpu *vcpu)
{
- if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
+ if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) {
+ __vgic_v3_activate_traps(vcpu);
__vgic_v3_restore_state(vcpu);
+ }
}
static bool __hyp_text __populate_fault_info(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 693d29f0036d..af7cf0faf58f 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -125,6 +125,8 @@ int __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
void __vgic_v3_save_state(struct kvm_vcpu *vcpu);
void __vgic_v3_restore_state(struct kvm_vcpu *vcpu);
+void __vgic_v3_activate_traps(struct kvm_vcpu *vcpu);
+void __vgic_v3_deactivate_traps(struct kvm_vcpu *vcpu);
void __vgic_v3_save_aprs(struct kvm_vcpu *vcpu);
void __vgic_v3_restore_aprs(struct kvm_vcpu *vcpu);
int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 9187afca181a..901a111fb509 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -194,14 +194,18 @@ static void __hyp_text __deactivate_vm(struct kvm_vcpu
*vcpu)
static void __hyp_text __vgic_save_state(struct kvm_vcpu *vcpu)
{
- if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
+ if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) {
__vgic_v3_save_state(vcpu);
+ __vgic_v3_deactivate_traps(vcpu);
+ }
}
static void __hyp_text __vgic_restore_state(struct kvm_vcpu *vcpu)
{
- if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
+ if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) {
+ __vgic_v3_activate_traps(vcpu);
__vgic_v3_restore_state(vcpu);
+ }
}
static bool __hyp_text __true_value(void)
diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index 811b42c8441d..e5f3bc7582b6 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -208,15 +208,15 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu
*vcpu)
{
Re: [PATCH v3 01/41] KVM: arm/arm64: Avoid vcpu_load for other vcpu ioctls than KVM_RUN
Hi Christoffer, On 12/01/18 12:07, Christoffer Dall wrote: Calling vcpu_load() registers preempt notifiers for this vcpu and calls kvm_arch_vcpu_load(). The latter will soon be doing a lot of heavy lifting on arm/arm64 and will try to do things such as enabling the virtual timer and setting us up to handle interrupts from the timer hardware. Loading state onto hardware registers and enabling hardware to signal interrupts can be problematic when we're not actually about to run the VCPU, because it makes it difficult to establish the right context when handling interrupts from the timer, and it makes the register access code difficult to reason about. Luckily, now when we call vcpu_load in each ioctl implementation, we can simply remove the call from the non-KVM_RUN vcpu ioctls, and our kvm_arch_vcpu_load() is only used for loading vcpu content to the physical CPU when we're actually going to run the vcpu. Signed-off-by: Christoffer Dall Reviewed-by: Julien Grall Cheers, -- Julien Grall ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v9 3/7] acpi: apei: Add SEI notification type support for ARMv8
[...]
>
> > Yes, I know you are dong that. Your serial's patch will consider all above
> things, right?
>
> Assuming I got it right, yes. It currently makes the race Xie XiuQi spotted
> worse,
> which I want to fix too. (details on the cover letter)
Ok.
>
>
> > If your patch can be consider that, this patch can based on your patchset.
> thanks.
>
> I'd like to pick these patches onto the end of that series, but first I want
> to
> know what NOTIFY_SEI means for any OS. The ACPI spec doesn't say, and
> because its asynchronous, route-able and mask-able, there are many more
> corners than NOTFIY_SEA.
>
> This thing is a notification using an emulated SError exception. (emulated
> because physical-SError must be routed to EL3 for firmware-first, and
> virtual-SError belongs to EL2).
>
> Does your firmware emulate SError exactly as the TakeException() pseudo code
> in the Arm-Arm?
Yes, it is.
> Is the emulated SError routed following the routing rules for HCR_EL2.{AMO,
> TGE}?
Yes, it is.
> What does your firmware do when it wants to emulate SError but its masked?
> (e.g.1: The physical-SError interrupted EL2 and the SPSR shows EL2 had
> PSTATE.A set.
> e.g.2: The physical-SError interrupted EL2 but HCR_EL2 indicates the
> emulated SError should go to EL1. This effectively masks SError.)
Currently we does not consider much about the mask status(SPSR).
I remember that you ever suggested firmware should reboot if the mask status is
set(SPSR), right?
I ever suggest our firmware team to evaluate that, but there is no response.
I CC "liu jun" who is our UEFI firmware Architect, if
you have firmware requirements, you can
raise again.
>
> Answers to these let us determine whether a bug is in the firmware or the
> kernel. If firmware is expecting the OS to do something special, I'd like to
> know
> about it from the beginning!
I know your meaning, thanks for raising it again.
>
>
> >>> Expose API ghes_notify_sei() to external users. External modules can
> >>> call this exposed API to parse APEI table and handle the SEI
> >>> notification.
> >>
> >> external modules? You mean called by the arch code when it gets this
> NOTIFY_SEI?
>
> > yes, called by kernel ARCH code, such as below, I remember I have discussed
> with you.
>
> Sure. The phrase 'external modules' usually means the '.ko' files that live in
> /lib/modules, nothing outside the kernel tree should be doing this stuff.
I will rename 'external modules' to other name. Thanks.
>
>
> Thanks,
>
> James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On 05/02/18 10:50, Christoffer Dall wrote: > On Mon, Feb 05, 2018 at 10:42:44AM +, Marc Zyngier wrote: >> On 05/02/18 09:58, Andrew Jones wrote: >>> On Mon, Feb 05, 2018 at 09:24:33AM +, Marc Zyngier wrote: On 04/02/18 12:37, Christoffer Dall wrote: >> >> [...] >> > Given the urgency of adding mitigation towards variant 2 which is the > driver for this work, I think we should drop the compat functionality in > this series and work this out later on if needed. I think we can just > tweak the previous patch to enable PSCI 1.0 by default and drop this > patch for the current merge window. I'd be fine with that, as long as we have a clear agreement on the impact of such a move. >>> >>> Yeah, that's what I was trying to figure out with my fancy tables. I might >>> be coming around more to your approach now, though. Ensuring the new->old >>> migration fails is a nice feature of this series. It would be good if >>> we could preserve that behavior without committing to a new userspace >>> interface, but I'm not sure how. Maybe I should just apologize for the >>> noise, and this patch be left as is... >> >> How about we don't decide now? >> >> I can remove this patch from the series so that the core stuff can make >> it into the arm64 tree ASAP (I think Catalin wants to queue something >> early this week so that it can hit Linus' tree before the end of the >> merge window), and then repost this single patch on its own (with fixes >> for the things that Christoffer found in his review) after -rc1. >> >> It leaves us time to haggle over the userspace ABI (which is >> realistically not going to affect anyone), and we get the core stuff in >> place for SoC vendors to start updating their firmware. >> > I agree, that's what I tried to suggest in my e-mail as well. Just > remember to tweak the previous patch to actually enable PSCI 1.0 by > default. Yup. I'll move the KVM_ARM_PSCI_LATEST hunk to that patch, and return it unconditionally from kvm_psci_version. 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
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On Mon, Feb 05, 2018 at 10:42:44AM +, Marc Zyngier wrote: > On 05/02/18 09:58, Andrew Jones wrote: > > On Mon, Feb 05, 2018 at 09:24:33AM +, Marc Zyngier wrote: > >> On 04/02/18 12:37, Christoffer Dall wrote: > > [...] > > >>> Given the urgency of adding mitigation towards variant 2 which is the > >>> driver for this work, I think we should drop the compat functionality in > >>> this series and work this out later on if needed. I think we can just > >>> tweak the previous patch to enable PSCI 1.0 by default and drop this > >>> patch for the current merge window. > >> > >> I'd be fine with that, as long as we have a clear agreement on the > >> impact of such a move. > > > > Yeah, that's what I was trying to figure out with my fancy tables. I might > > be coming around more to your approach now, though. Ensuring the new->old > > migration fails is a nice feature of this series. It would be good if > > we could preserve that behavior without committing to a new userspace > > interface, but I'm not sure how. Maybe I should just apologize for the > > noise, and this patch be left as is... > > How about we don't decide now? > > I can remove this patch from the series so that the core stuff can make > it into the arm64 tree ASAP (I think Catalin wants to queue something > early this week so that it can hit Linus' tree before the end of the > merge window), and then repost this single patch on its own (with fixes > for the things that Christoffer found in his review) after -rc1. > > It leaves us time to haggle over the userspace ABI (which is > realistically not going to affect anyone), and we get the core stuff in > place for SoC vendors to start updating their firmware. > I agree, that's what I tried to suggest in my e-mail as well. Just remember to tweak the previous patch to actually enable PSCI 1.0 by default. Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On 05/02/18 09:58, Andrew Jones wrote: > On Mon, Feb 05, 2018 at 09:24:33AM +, Marc Zyngier wrote: >> On 04/02/18 12:37, Christoffer Dall wrote: [...] >>> Given the urgency of adding mitigation towards variant 2 which is the >>> driver for this work, I think we should drop the compat functionality in >>> this series and work this out later on if needed. I think we can just >>> tweak the previous patch to enable PSCI 1.0 by default and drop this >>> patch for the current merge window. >> >> I'd be fine with that, as long as we have a clear agreement on the >> impact of such a move. > > Yeah, that's what I was trying to figure out with my fancy tables. I might > be coming around more to your approach now, though. Ensuring the new->old > migration fails is a nice feature of this series. It would be good if > we could preserve that behavior without committing to a new userspace > interface, but I'm not sure how. Maybe I should just apologize for the > noise, and this patch be left as is... How about we don't decide now? I can remove this patch from the series so that the core stuff can make it into the arm64 tree ASAP (I think Catalin wants to queue something early this week so that it can hit Linus' tree before the end of the merge window), and then repost this single patch on its own (with fixes for the things that Christoffer found in his review) after -rc1. It leaves us time to haggle over the userspace ABI (which is realistically not going to affect anyone), and we get the core stuff in place for SoC vendors to start updating their firmware. Thoughts? 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
Re: [PATCH v3 12/18] arm64: KVM: Add SMCCC_ARCH_WORKAROUND_1 fast handling
On Mon, Feb 05, 2018 at 09:08:31AM +, Marc Zyngier wrote: > On 04/02/18 18:39, Christoffer Dall wrote: > > On Thu, Feb 01, 2018 at 11:46:51AM +, Marc Zyngier wrote: > >> We want SMCCC_ARCH_WORKAROUND_1 to be fast. As fast as possible. > >> So let's intercept it as early as we can by testing for the > >> function call number as soon as we've identified a HVC call > >> coming from the guest. > > > > Hmmm. How often is this expected to happen and what is the expected > > extra cost of doing the early-exit handling in the C code vs. here? > > Pretty often. On each context switch of a Linux guest, for example. It > is almost as bad as if we were trapping all VM ops. Moving it to C is > definitely visible on something like hackbench (I remember something > like a 10-12% degradation on Seattle, but I'd need to rerun the tests to > give you something accurate). If it's that easily visible (although hackbench is clearly the pathological case here), then we should try to optimize it. Let's hope we don't have to add too many of these workarounds in the future. > It is the whole GPR save/restore dance > that costs us a lot (31 registers for the guest, 12 for the host), plus > some the extra SError synchronization that doesn't come for free either. > Fair enough. > > I think we'd be better off if we only had a single early-exit path (and > > we should move the FP/SIMD trap to that path as well), but if there's a > > measurable benefit of having this logic in assembly as opposed to in the > > C code, then I'm ok with this as well. > > I agree that the multiplication of "earlier than early" paths is > becoming annoying. Moving the FP/SIMD stuff to C would be less > problematic, as we have patches to move some of that to load/put, and > we'd only take the trap once per time slice (as opposed to once per > entry at the moment). Yes, and we can even improve on that (see separate discussions around KVM support for SVE with Dave). > > Here, we're trying hard to do exactly nothing, because each instruction > is just an extra overhead (we've already nuked the BP). I even > considered inserting that code as part of the per-CPU-type vectors (and > leave the rest of the KVM code alone), but it felt like a step too far. > We can always look at adjusting this more in the future if we want. Reviewed-by: Christoffer Dall ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 33/41] KVM: arm64: Configure FPSIMD traps on vcpu load/put
Hi Tomasz,
On Wed, Jan 31, 2018 at 01:17:36PM +0100, Tomasz Nowicki wrote:
> On 12.01.2018 13:07, Christoffer Dall wrote:
> >There is no need to enable/disable traps to FP registers on every switch
> >to/from the VM, because the host kernel does not use this resource
> >without calling vcpu_put. We can therefore move things around enough
> >that we still always write FPEXC32_EL2 before programming CPTR_EL2 but
> >only program these during vcpu load/put.
> >
> >Signed-off-by: Christoffer Dall
> >---
> > arch/arm64/include/asm/kvm_hyp.h | 6 +
> > arch/arm64/kvm/hyp/switch.c | 51
> > +---
> > arch/arm64/kvm/hyp/sysreg-sr.c | 12 --
> > 3 files changed, 53 insertions(+), 16 deletions(-)
> >
> >diff --git a/arch/arm64/include/asm/kvm_hyp.h
> >b/arch/arm64/include/asm/kvm_hyp.h
> >index 3f54c55f77a1..ffd62e31f134 100644
> >--- a/arch/arm64/include/asm/kvm_hyp.h
> >+++ b/arch/arm64/include/asm/kvm_hyp.h
> >@@ -148,6 +148,12 @@ void __fpsimd_save_state(struct user_fpsimd_state
> >*fp_regs);
> > void __fpsimd_restore_state(struct user_fpsimd_state *fp_regs);
> > bool __fpsimd_enabled(void);
> >+void __activate_traps_nvhe_load(struct kvm_vcpu *vcpu);
> >+void __deactivate_traps_nvhe_put(void);
> >+
> >+void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
> >+void deactivate_traps_vhe_put(void);
> >+
> > u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context
> > *host_ctxt);
> > void __noreturn __hyp_do_panic(unsigned long, ...);
> >diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> >index c01bcfc3fb52..d14ab9650f81 100644
> >--- a/arch/arm64/kvm/hyp/switch.c
> >+++ b/arch/arm64/kvm/hyp/switch.c
> >@@ -24,22 +24,25 @@
> > #include
> > #include
> >-static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
> >+static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
> > {
> > /*
> >- * We are about to set CPTR_EL2.TFP to trap all floating point
> >- * register accesses to EL2, however, the ARM ARM clearly states that
> >- * traps are only taken to EL2 if the operation would not otherwise
> >- * trap to EL1. Therefore, always make sure that for 32-bit guests,
> >- * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> >- * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
> >- * it will cause an exception.
> >+ * We are about to trap all floating point register accesses to EL2,
> >+ * however, traps are only taken to EL2 if the operation would not
> >+ * otherwise trap to EL1. Therefore, always make sure that for 32-bit
> >+ * guests, we set FPEXC.EN to prevent traps to EL1, when setting the
> >+ * TFP bit. If FP/ASIMD is not implemented, FPEXC is UNDEFINED and
> >+ * any access to it will cause an exception.
> > */
> > if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd() &&
> > !vcpu->arch.guest_vfp_loaded) {
> > write_sysreg(1 << 30, fpexc32_el2);
> > isb();
> > }
> >+}
> >+
> >+static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
> >+{
> > write_sysreg(vcpu->arch.hcr_el2, hcr_el2);
> > /* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */
> >@@ -61,10 +64,12 @@ static void __hyp_text __deactivate_traps_common(void)
> > write_sysreg(0, pmuserenr_el0);
> > }
> >-static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu)
> >+void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
> > {
> > u64 val;
> >+__activate_traps_fpsimd32(vcpu);
> >+
> > val = read_sysreg(cpacr_el1);
> > val |= CPACR_EL1_TTA;
> > val &= ~CPACR_EL1_ZEN;
> >@@ -73,14 +78,26 @@ static void __hyp_text __activate_traps_vhe(struct
> >kvm_vcpu *vcpu)
> > else
> > val &= ~CPACR_EL1_FPEN;
> > write_sysreg(val, cpacr_el1);
>
> Giving that you move this code to kvm_vcpu_load_sysregs() I am wondering if
> we have to deactivate FPEN trap here. IIUC, we call
> kvm_vcpu_load_sysregs()->activate_traps_vhe_load() and then
> kvm_vcpu_put_sysregs() by design. So vcpu->arch.guest_vfp_loaded should be
> always 0 here since it is zeroed in kvm_vcpu_put_sysregs(). The same for
> nvhe case below.
>
You're absolutely right, we can enable the trapping unconditionally on
this path.
Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On Mon, Feb 05, 2018 at 09:24:33AM +, Marc Zyngier wrote: > On 04/02/18 12:37, Christoffer Dall wrote: > > On Sat, Feb 03, 2018 at 11:59:32AM +, Marc Zyngier wrote: > >> On Fri, 2 Feb 2018 21:17:06 +0100 > >> Andrew Jones wrote: > >> > >>> On Thu, Feb 01, 2018 at 11:46:47AM +, Marc Zyngier wrote: > Although we've implemented PSCI 1.0 and 1.1, nothing can select them > Since all the new PSCI versions are backward compatible, we decide to > default to the latest version of the PSCI implementation. This is no > different from doing a firmware upgrade on KVM. > > But in order to give a chance to hypothetical badly implemented guests > that would have a fit by discovering something other than PSCI 0.2, > let's provide a new API that allows userspace to pick one particular > version of the API. > > This is implemented as a new class of "firmware" registers, where > we expose the PSCI version. This allows the PSCI version to be > save/restored as part of a guest migration, and also set to > any supported version if the guest requires it. > > Signed-off-by: Marc Zyngier > --- > Documentation/virtual/kvm/api.txt | 3 +- > Documentation/virtual/kvm/arm/psci.txt | 30 +++ > arch/arm/include/asm/kvm_host.h| 3 ++ > arch/arm/include/uapi/asm/kvm.h| 6 +++ > arch/arm/kvm/guest.c | 13 +++ > arch/arm64/include/asm/kvm_host.h | 3 ++ > arch/arm64/include/uapi/asm/kvm.h | 6 +++ > arch/arm64/kvm/guest.c | 14 ++- > include/kvm/arm_psci.h | 9 + > virt/kvm/arm/psci.c| 68 > +- > 10 files changed, 151 insertions(+), 4 deletions(-) > create mode 100644 Documentation/virtual/kvm/arm/psci.txt > > diff --git a/Documentation/virtual/kvm/api.txt > b/Documentation/virtual/kvm/api.txt > index 57d3ee9e4bde..334905202141 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -2493,7 +2493,8 @@ Possible features: > and execute guest code when KVM_RUN is called. > - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. > Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). > -- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. > +- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision > + backward compatible with v0.2) for the CPU. > Depends on KVM_CAP_ARM_PSCI_0_2. > - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. > Depends on KVM_CAP_ARM_PMU_V3. > diff --git a/Documentation/virtual/kvm/arm/psci.txt > b/Documentation/virtual/kvm/arm/psci.txt > new file mode 100644 > index ..aafdab887b04 > --- /dev/null > +++ b/Documentation/virtual/kvm/arm/psci.txt > @@ -0,0 +1,30 @@ > +KVM implements the PSCI (Power State Coordination Interface) > +specification in order to provide services such as CPU on/off, reset > +and power-off to the guest. > + > +The PSCI specification is regularly updated to provide new features, > +and KVM implements these updates if they make sense from a > virtualization > +point of view. > + > +This means that a guest booted on two different versions of KVM can > +observe two different "firmware" revisions. This could cause issues if > +a given guest is tied to a particular PSCI revision (unlikely), or if > +a migration causes a different PSCI version to be exposed out of the > +blue to an unsuspecting guest. > + > +In order to remedy this situation, KVM exposes a set of "firmware > +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG > +interface. These registers can be saved/restored by userspace, and set > +to a convenient value if required. > + > +The following register is defined: > + > +* KVM_REG_ARM_PSCI_VERSION: > + > + - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set > +(and thus has already been initialized) > + - Returns the current PSCI version on GET_ONE_REG (defaulting to the > +highest PSCI version implemented by KVM and compatible with v0.2) > + - Allows any PSCI version implemented by KVM and compatible with > +v0.2 to be set with SET_ONE_REG > + - Affects the whole VM (even if the register view is per-vcpu) > >>> > >> > >> Hi Drew, > >> > >> Thanks for looking into this, and for the exhaustive data. > >> > >>> > >>> I've put some more thought and experimentation into this. I think we > >>> should change to a vcpu feature bit. The feature bit would be used to > >>> force compat mode, v0.2, so KVM would still enable
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On Sun, Feb 04, 2018 at 01:37:01PM +0100, Christoffer Dall wrote: > On Sat, Feb 03, 2018 at 11:59:32AM +, Marc Zyngier wrote: > > On Fri, 2 Feb 2018 21:17:06 +0100 > > Andrew Jones wrote: > > > > > On Thu, Feb 01, 2018 at 11:46:47AM +, Marc Zyngier wrote: > > > > Although we've implemented PSCI 1.0 and 1.1, nothing can select them > > > > Since all the new PSCI versions are backward compatible, we decide to > > > > default to the latest version of the PSCI implementation. This is no > > > > different from doing a firmware upgrade on KVM. > > > > > > > > But in order to give a chance to hypothetical badly implemented guests > > > > that would have a fit by discovering something other than PSCI 0.2, > > > > let's provide a new API that allows userspace to pick one particular > > > > version of the API. > > > > > > > > This is implemented as a new class of "firmware" registers, where > > > > we expose the PSCI version. This allows the PSCI version to be > > > > save/restored as part of a guest migration, and also set to > > > > any supported version if the guest requires it. > > > > > > > > Signed-off-by: Marc Zyngier > > > > --- > > > > Documentation/virtual/kvm/api.txt | 3 +- > > > > Documentation/virtual/kvm/arm/psci.txt | 30 +++ > > > > arch/arm/include/asm/kvm_host.h| 3 ++ > > > > arch/arm/include/uapi/asm/kvm.h| 6 +++ > > > > arch/arm/kvm/guest.c | 13 +++ > > > > arch/arm64/include/asm/kvm_host.h | 3 ++ > > > > arch/arm64/include/uapi/asm/kvm.h | 6 +++ > > > > arch/arm64/kvm/guest.c | 14 ++- > > > > include/kvm/arm_psci.h | 9 + > > > > virt/kvm/arm/psci.c| 68 > > > > +- > > > > 10 files changed, 151 insertions(+), 4 deletions(-) > > > > create mode 100644 Documentation/virtual/kvm/arm/psci.txt > > > > > > > > diff --git a/Documentation/virtual/kvm/api.txt > > > > b/Documentation/virtual/kvm/api.txt > > > > index 57d3ee9e4bde..334905202141 100644 > > > > --- a/Documentation/virtual/kvm/api.txt > > > > +++ b/Documentation/virtual/kvm/api.txt > > > > @@ -2493,7 +2493,8 @@ Possible features: > > > > and execute guest code when KVM_RUN is called. > > > > - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. > > > > Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). > > > > - - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. > > > > + - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision > > > > + backward compatible with v0.2) for the CPU. > > > > Depends on KVM_CAP_ARM_PSCI_0_2. > > > > - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. > > > > Depends on KVM_CAP_ARM_PMU_V3. > > > > diff --git a/Documentation/virtual/kvm/arm/psci.txt > > > > b/Documentation/virtual/kvm/arm/psci.txt > > > > new file mode 100644 > > > > index ..aafdab887b04 > > > > --- /dev/null > > > > +++ b/Documentation/virtual/kvm/arm/psci.txt > > > > @@ -0,0 +1,30 @@ > > > > +KVM implements the PSCI (Power State Coordination Interface) > > > > +specification in order to provide services such as CPU on/off, reset > > > > +and power-off to the guest. > > > > + > > > > +The PSCI specification is regularly updated to provide new features, > > > > +and KVM implements these updates if they make sense from a > > > > virtualization > > > > +point of view. > > > > + > > > > +This means that a guest booted on two different versions of KVM can > > > > +observe two different "firmware" revisions. This could cause issues if > > > > +a given guest is tied to a particular PSCI revision (unlikely), or if > > > > +a migration causes a different PSCI version to be exposed out of the > > > > +blue to an unsuspecting guest. > > > > + > > > > +In order to remedy this situation, KVM exposes a set of "firmware > > > > +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG > > > > +interface. These registers can be saved/restored by userspace, and set > > > > +to a convenient value if required. > > > > + > > > > +The following register is defined: > > > > + > > > > +* KVM_REG_ARM_PSCI_VERSION: > > > > + > > > > + - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set > > > > +(and thus has already been initialized) > > > > + - Returns the current PSCI version on GET_ONE_REG (defaulting to the > > > > +highest PSCI version implemented by KVM and compatible with v0.2) > > > > + - Allows any PSCI version implemented by KVM and compatible with > > > > +v0.2 to be set with SET_ONE_REG > > > > + - Affects the whole VM (even if the register view is per-vcpu) > > > > > > > Hi Drew, > > > > Thanks for looking into this, and for the exhaustive data. > > > > > > > > I've put some more thought and experimentation into this. I think we > > > should change to a vcpu feature bit. The feature bit would be used to > > > force compat mod
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On 04/02/18 12:38, Christoffer Dall wrote: > Hi Marc, > > [ I know we're discussing the overall approach in parallel, but here are > some comments on the specifics of this patch, should it end up being > used in some capacity ] > > On Thu, Feb 01, 2018 at 11:46:47AM +, Marc Zyngier wrote: >> Although we've implemented PSCI 1.0 and 1.1, nothing can select them >> Since all the new PSCI versions are backward compatible, we decide to >> default to the latest version of the PSCI implementation. This is no >> different from doing a firmware upgrade on KVM. >> >> But in order to give a chance to hypothetical badly implemented guests >> that would have a fit by discovering something other than PSCI 0.2, >> let's provide a new API that allows userspace to pick one particular >> version of the API. >> >> This is implemented as a new class of "firmware" registers, where >> we expose the PSCI version. This allows the PSCI version to be >> save/restored as part of a guest migration, and also set to >> any supported version if the guest requires it. >> >> Signed-off-by: Marc Zyngier >> --- >> Documentation/virtual/kvm/api.txt | 3 +- >> Documentation/virtual/kvm/arm/psci.txt | 30 +++ >> arch/arm/include/asm/kvm_host.h| 3 ++ >> arch/arm/include/uapi/asm/kvm.h| 6 +++ >> arch/arm/kvm/guest.c | 13 +++ >> arch/arm64/include/asm/kvm_host.h | 3 ++ >> arch/arm64/include/uapi/asm/kvm.h | 6 +++ >> arch/arm64/kvm/guest.c | 14 ++- >> include/kvm/arm_psci.h | 9 + >> virt/kvm/arm/psci.c| 68 >> +- >> 10 files changed, 151 insertions(+), 4 deletions(-) >> create mode 100644 Documentation/virtual/kvm/arm/psci.txt >> >> diff --git a/Documentation/virtual/kvm/api.txt >> b/Documentation/virtual/kvm/api.txt >> index 57d3ee9e4bde..334905202141 100644 >> --- a/Documentation/virtual/kvm/api.txt >> +++ b/Documentation/virtual/kvm/api.txt >> @@ -2493,7 +2493,8 @@ Possible features: >>and execute guest code when KVM_RUN is called. >> - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. >>Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). >> - - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. >> + - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision >> + backward compatible with v0.2) for the CPU. >>Depends on KVM_CAP_ARM_PSCI_0_2. >> - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. >>Depends on KVM_CAP_ARM_PMU_V3. > > Can we add this to api.txt as well ?: > > 8><-- > > diff --git a/Documentation/virtual/kvm/api.txt > b/Documentation/virtual/kvm/api.txt > index fc3ae951bc07..c88aa04bcbe8 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -1959,6 +1959,8 @@ arm64 CCSIDR registers are demultiplexed by CSSELR > value: > arm64 system registers have the following id bit patterns: >0x6030 0013 > > +ARM/arm64 firmware pseudo-registers have the following bit pattern: > + 0x4030 0014 > > MIPS registers are mapped using the lower 32 bits. The upper 16 of that is > the register group type: > > 8><-- Ah, I never realised we actually documented this. Neat! > >> diff --git a/Documentation/virtual/kvm/arm/psci.txt >> b/Documentation/virtual/kvm/arm/psci.txt >> new file mode 100644 >> index ..aafdab887b04 >> --- /dev/null >> +++ b/Documentation/virtual/kvm/arm/psci.txt >> @@ -0,0 +1,30 @@ >> +KVM implements the PSCI (Power State Coordination Interface) >> +specification in order to provide services such as CPU on/off, reset >> +and power-off to the guest. >> + >> +The PSCI specification is regularly updated to provide new features, >> +and KVM implements these updates if they make sense from a virtualization >> +point of view. >> + >> +This means that a guest booted on two different versions of KVM can >> +observe two different "firmware" revisions. This could cause issues if >> +a given guest is tied to a particular PSCI revision (unlikely), or if >> +a migration causes a different PSCI version to be exposed out of the >> +blue to an unsuspecting guest. >> + >> +In order to remedy this situation, KVM exposes a set of "firmware >> +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG >> +interface. These registers can be saved/restored by userspace, and set >> +to a convenient value if required. >> + >> +The following register is defined: >> + >> +* KVM_REG_ARM_PSCI_VERSION: >> + >> + - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set >> +(and thus has already been initialized) >> + - Returns the current PSCI version on GET_ONE_REG (defaulting to the >> +highest PSCI version implemented by KVM and compatible with v0.2) >> + - Allows any PSCI version implemented by KVM and compatible with >> +v0.2 to be set with SET_ONE_REG >> + - Affects the whole VM (even if
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On Sat, Feb 03, 2018 at 11:59:32AM +, Marc Zyngier wrote: > On Fri, 2 Feb 2018 21:17:06 +0100 > Andrew Jones wrote: > > > On Thu, Feb 01, 2018 at 11:46:47AM +, Marc Zyngier wrote: > > > Although we've implemented PSCI 1.0 and 1.1, nothing can select them > > > Since all the new PSCI versions are backward compatible, we decide to > > > default to the latest version of the PSCI implementation. This is no > > > different from doing a firmware upgrade on KVM. > > > > > > But in order to give a chance to hypothetical badly implemented guests > > > that would have a fit by discovering something other than PSCI 0.2, > > > let's provide a new API that allows userspace to pick one particular > > > version of the API. > > > > > > This is implemented as a new class of "firmware" registers, where > > > we expose the PSCI version. This allows the PSCI version to be > > > save/restored as part of a guest migration, and also set to > > > any supported version if the guest requires it. > > > > > > Signed-off-by: Marc Zyngier > > > --- > > > Documentation/virtual/kvm/api.txt | 3 +- > > > Documentation/virtual/kvm/arm/psci.txt | 30 +++ > > > arch/arm/include/asm/kvm_host.h| 3 ++ > > > arch/arm/include/uapi/asm/kvm.h| 6 +++ > > > arch/arm/kvm/guest.c | 13 +++ > > > arch/arm64/include/asm/kvm_host.h | 3 ++ > > > arch/arm64/include/uapi/asm/kvm.h | 6 +++ > > > arch/arm64/kvm/guest.c | 14 ++- > > > include/kvm/arm_psci.h | 9 + > > > virt/kvm/arm/psci.c| 68 > > > +- > > > 10 files changed, 151 insertions(+), 4 deletions(-) > > > create mode 100644 Documentation/virtual/kvm/arm/psci.txt > > > > > > diff --git a/Documentation/virtual/kvm/api.txt > > > b/Documentation/virtual/kvm/api.txt > > > index 57d3ee9e4bde..334905202141 100644 > > > --- a/Documentation/virtual/kvm/api.txt > > > +++ b/Documentation/virtual/kvm/api.txt > > > @@ -2493,7 +2493,8 @@ Possible features: > > > and execute guest code when KVM_RUN is called. > > > - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. > > > Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). > > > - - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. > > > + - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision > > > + backward compatible with v0.2) for the CPU. > > > Depends on KVM_CAP_ARM_PSCI_0_2. > > > - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. > > > Depends on KVM_CAP_ARM_PMU_V3. > > > diff --git a/Documentation/virtual/kvm/arm/psci.txt > > > b/Documentation/virtual/kvm/arm/psci.txt > > > new file mode 100644 > > > index ..aafdab887b04 > > > --- /dev/null > > > +++ b/Documentation/virtual/kvm/arm/psci.txt > > > @@ -0,0 +1,30 @@ > > > +KVM implements the PSCI (Power State Coordination Interface) > > > +specification in order to provide services such as CPU on/off, reset > > > +and power-off to the guest. > > > + > > > +The PSCI specification is regularly updated to provide new features, > > > +and KVM implements these updates if they make sense from a virtualization > > > +point of view. > > > + > > > +This means that a guest booted on two different versions of KVM can > > > +observe two different "firmware" revisions. This could cause issues if > > > +a given guest is tied to a particular PSCI revision (unlikely), or if > > > +a migration causes a different PSCI version to be exposed out of the > > > +blue to an unsuspecting guest. > > > + > > > +In order to remedy this situation, KVM exposes a set of "firmware > > > +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG > > > +interface. These registers can be saved/restored by userspace, and set > > > +to a convenient value if required. > > > + > > > +The following register is defined: > > > + > > > +* KVM_REG_ARM_PSCI_VERSION: > > > + > > > + - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set > > > +(and thus has already been initialized) > > > + - Returns the current PSCI version on GET_ONE_REG (defaulting to the > > > +highest PSCI version implemented by KVM and compatible with v0.2) > > > + - Allows any PSCI version implemented by KVM and compatible with > > > +v0.2 to be set with SET_ONE_REG > > > + - Affects the whole VM (even if the register view is per-vcpu) > > > > Hi Drew, > > Thanks for looking into this, and for the exhaustive data. > > > > > I've put some more thought and experimentation into this. I think we > > should change to a vcpu feature bit. The feature bit would be used to > > force compat mode, v0.2, so KVM would still enable the new PSCI > > version by default. Below are two tables describing why I think we > > should switch to something other than a new sysreg, and below those > > tables are notes as to why I think we should use a vcpu feature. The > > asterisks in the tables point out behaviors that ar
Re: [PATCH v3 08/18] arm/arm64: KVM: Add PSCI version selection API
On 04/02/18 12:37, Christoffer Dall wrote: > On Sat, Feb 03, 2018 at 11:59:32AM +, Marc Zyngier wrote: >> On Fri, 2 Feb 2018 21:17:06 +0100 >> Andrew Jones wrote: >> >>> On Thu, Feb 01, 2018 at 11:46:47AM +, Marc Zyngier wrote: Although we've implemented PSCI 1.0 and 1.1, nothing can select them Since all the new PSCI versions are backward compatible, we decide to default to the latest version of the PSCI implementation. This is no different from doing a firmware upgrade on KVM. But in order to give a chance to hypothetical badly implemented guests that would have a fit by discovering something other than PSCI 0.2, let's provide a new API that allows userspace to pick one particular version of the API. This is implemented as a new class of "firmware" registers, where we expose the PSCI version. This allows the PSCI version to be save/restored as part of a guest migration, and also set to any supported version if the guest requires it. Signed-off-by: Marc Zyngier --- Documentation/virtual/kvm/api.txt | 3 +- Documentation/virtual/kvm/arm/psci.txt | 30 +++ arch/arm/include/asm/kvm_host.h| 3 ++ arch/arm/include/uapi/asm/kvm.h| 6 +++ arch/arm/kvm/guest.c | 13 +++ arch/arm64/include/asm/kvm_host.h | 3 ++ arch/arm64/include/uapi/asm/kvm.h | 6 +++ arch/arm64/kvm/guest.c | 14 ++- include/kvm/arm_psci.h | 9 + virt/kvm/arm/psci.c| 68 +- 10 files changed, 151 insertions(+), 4 deletions(-) create mode 100644 Documentation/virtual/kvm/arm/psci.txt diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt index 57d3ee9e4bde..334905202141 100644 --- a/Documentation/virtual/kvm/api.txt +++ b/Documentation/virtual/kvm/api.txt @@ -2493,7 +2493,8 @@ Possible features: and execute guest code when KVM_RUN is called. - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode. Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only). - - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU. + - KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision + backward compatible with v0.2) for the CPU. Depends on KVM_CAP_ARM_PSCI_0_2. - KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU. Depends on KVM_CAP_ARM_PMU_V3. diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt new file mode 100644 index ..aafdab887b04 --- /dev/null +++ b/Documentation/virtual/kvm/arm/psci.txt @@ -0,0 +1,30 @@ +KVM implements the PSCI (Power State Coordination Interface) +specification in order to provide services such as CPU on/off, reset +and power-off to the guest. + +The PSCI specification is regularly updated to provide new features, +and KVM implements these updates if they make sense from a virtualization +point of view. + +This means that a guest booted on two different versions of KVM can +observe two different "firmware" revisions. This could cause issues if +a given guest is tied to a particular PSCI revision (unlikely), or if +a migration causes a different PSCI version to be exposed out of the +blue to an unsuspecting guest. + +In order to remedy this situation, KVM exposes a set of "firmware +pseudo-registers" that can be manipulated using the GET/SET_ONE_REG +interface. These registers can be saved/restored by userspace, and set +to a convenient value if required. + +The following register is defined: + +* KVM_REG_ARM_PSCI_VERSION: + + - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set +(and thus has already been initialized) + - Returns the current PSCI version on GET_ONE_REG (defaulting to the +highest PSCI version implemented by KVM and compatible with v0.2) + - Allows any PSCI version implemented by KVM and compatible with +v0.2 to be set with SET_ONE_REG + - Affects the whole VM (even if the register view is per-vcpu) >>> >> >> Hi Drew, >> >> Thanks for looking into this, and for the exhaustive data. >> >>> >>> I've put some more thought and experimentation into this. I think we >>> should change to a vcpu feature bit. The feature bit would be used to >>> force compat mode, v0.2, so KVM would still enable the new PSCI >>> version by default. Below are two tables describing why I think we >>> should switch to something other than a new sysreg, and below those >>> tables are notes as to why I think we should use a vcpu feature. The >>> asterisks in the tables point out behaviors that aren't what we want. >>> While bot
Re: [PATCH v3 12/18] arm64: KVM: Add SMCCC_ARCH_WORKAROUND_1 fast handling
On 04/02/18 18:39, Christoffer Dall wrote: > On Thu, Feb 01, 2018 at 11:46:51AM +, Marc Zyngier wrote: >> We want SMCCC_ARCH_WORKAROUND_1 to be fast. As fast as possible. >> So let's intercept it as early as we can by testing for the >> function call number as soon as we've identified a HVC call >> coming from the guest. > > Hmmm. How often is this expected to happen and what is the expected > extra cost of doing the early-exit handling in the C code vs. here? Pretty often. On each context switch of a Linux guest, for example. It is almost as bad as if we were trapping all VM ops. Moving it to C is definitely visible on something like hackbench (I remember something like a 10-12% degradation on Seattle, but I'd need to rerun the tests to give you something accurate). It is the whole GPR save/restore dance that costs us a lot (31 registers for the guest, 12 for the host), plus some the extra SError synchronization that doesn't come for free either. > I think we'd be better off if we only had a single early-exit path (and > we should move the FP/SIMD trap to that path as well), but if there's a > measurable benefit of having this logic in assembly as opposed to in the > C code, then I'm ok with this as well. I agree that the multiplication of "earlier than early" paths is becoming annoying. Moving the FP/SIMD stuff to C would be less problematic, as we have patches to move some of that to load/put, and we'd only take the trap once per time slice (as opposed to once per entry at the moment). Here, we're trying hard to do exactly nothing, because each instruction is just an extra overhead (we've already nuked the BP). I even considered inserting that code as part of the per-CPU-type vectors (and leave the rest of the KVM code alone), but it felt like a step too far. > The code in this patch looks fine otherwise. 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
Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
On Sun, Feb 04, 2018 at 09:57:49PM +0100, Arnd Bergmann wrote:
> On Sun, Feb 4, 2018 at 7:45 PM, Christoffer Dall
> wrote:
> > Hi Arnd,
> >
> > On Fri, Feb 02, 2018 at 04:07:34PM +0100, Arnd Bergmann wrote:
> >> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
> >> statement to allow compilation of a multi-CPU kernel for ARMv6
> >> and older ARMv7-A that don't normally support access to the banked
> >> registers.
> >>
> >> This is considered to be a programming error by the gcc developers
> >> and will no longer work in gcc-8, where we now get a build error:
> >>
> >> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this
> >> architecture. -- `mrs r3,SP_usr'
> >> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this
> >> architecture. -- `mrs r3,ELR_hyp'
> >> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this
> >> architecture. -- `mrs r3,SP_svc'
> >> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this
> >> architecture. -- `mrs r3,LR_svc'
> >> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this
> >> architecture. -- `mrs r3,SPSR_svc'
> >> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this
> >> architecture. -- `mrs r3,SP_abt'
> >>
> >> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
> >> we know the functions won't ever be called on pre-ARMv7VE machines.
> >> Unfortunately, older compiler versions (4.8 and earlier) do not understand
> >> that flag, so we still need to keep the asm around.
> >
> > Does "not understand" mean "ignores" or do we get an error?
>
> We get an error, which is why I used the $(call cc-option) Makefile
> helper to check if the compiler supports it.
>
Right.
> >> Backporting to stable kernels (4.6+) is needed to allow those to be built
> >> with future compilers as well.
> >
> > This builds on the toolchains I have on my machine, so:
> >
> > Acked-by: Christoffer Dall
> >
> > Are you applying this via a tree with other fixes or would you like me
> > to carry it in the kvmarm tree?
>
> Please pick it up in your tree.
>
Will do.
Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
