| Message ID | 20230127150727.612594-7-jean-philippe@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: Run Arm CCA VMs with KVM | expand |
On 1/27/23 05:07, Jean-Philippe Brucker wrote: > The target code calls kvm_arm_vcpu_init() to mark the vCPU as part of a > realm. RME support does not use the register lists, because the host can > only set the boot PC and registers x0-x7. The rest is private to the > Realm and saved/restored by the RMM. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > --- > target/arm/cpu.h | 3 ++ > target/arm/kvm_arm.h | 1 + > target/arm/helper.c | 8 ++++++ > target/arm/kvm-rme.c | 10 +++++++ > target/arm/kvm.c | 12 ++++++++ > target/arm/kvm64.c | 65 ++++++++++++++++++++++++++++++++++++++++++-- > 6 files changed, 97 insertions(+), 2 deletions(-) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 9aeed3c848..7d8397985f 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -937,6 +937,9 @@ struct ArchCPU { > /* KVM steal time */ > OnOffAuto kvm_steal_time; > > + /* Realm Management Extension */ > + bool kvm_rme; > + > /* Uniprocessor system with MP extensions */ > bool mp_is_up; > > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h > index 00d3df8cac..e4dc7fbb8d 100644 > --- a/target/arm/kvm_arm.h > +++ b/target/arm/kvm_arm.h > @@ -373,6 +373,7 @@ int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp); > int kvm_arm_rme_vm_type(MachineState *ms); > > bool kvm_arm_rme_enabled(void); > +int kvm_arm_rme_vcpu_init(CPUState *cs); > > #else > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index d8c8223ec3..52360ae2ff 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -126,6 +126,10 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync) > int i; > bool ok = true; > > + if (cpu->kvm_rme) { > + return ok; > + } > + > for (i = 0; i < cpu->cpreg_array_len; i++) { > uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]); > const ARMCPRegInfo *ri; > @@ -171,6 +175,10 @@ bool write_list_to_cpustate(ARMCPU *cpu) > int i; > bool ok = true; > > + if (cpu->kvm_rme) { > + return ok; > + } > + > for (i = 0; i < cpu->cpreg_array_len; i++) { > uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]); > uint64_t v = cpu->cpreg_values[i]; > diff --git a/target/arm/kvm-rme.c b/target/arm/kvm-rme.c > index d7cdca1cbf..3833b187f9 100644 > --- a/target/arm/kvm-rme.c > +++ b/target/arm/kvm-rme.c > @@ -118,6 +118,16 @@ int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp) > return 0; > } > > +int kvm_arm_rme_vcpu_init(CPUState *cs) > +{ > + ARMCPU *cpu = ARM_CPU(cs); > + > + if (kvm_arm_rme_enabled()) { > + cpu->kvm_rme = true; > + } > + return 0; > +} > + > int kvm_arm_rme_vm_type(MachineState *ms) > { > if (cgs_to_rme(ms->cgs)) { > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index f022c644d2..fcddead4fe 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -449,6 +449,10 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu) > int i, ret, arraylen; > CPUState *cs = CPU(cpu); > > + if (cpu->kvm_rme) { > + return 0; > + } > + > rl.n = 0; > ret = kvm_vcpu_ioctl(cs, KVM_GET_REG_LIST, &rl); > if (ret != -E2BIG) { > @@ -521,6 +525,10 @@ bool write_kvmstate_to_list(ARMCPU *cpu) > int i; > bool ok = true; > > + if (cpu->kvm_rme) { > + return ok; > + } > + > for (i = 0; i < cpu->cpreg_array_len; i++) { > struct kvm_one_reg r; > uint64_t regidx = cpu->cpreg_indexes[i]; > @@ -557,6 +565,10 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level) > int i; > bool ok = true; > > + if (cpu->kvm_rme) { > + return ok; > + } I don't think that simply returning "ok" is best. We shouldn't be calling this function at all with rme enabled. r~
On 1/27/23 05:07, Jean-Philippe Brucker wrote: > +static int kvm_arm_rme_get_core_regs(CPUState *cs) > +{ > + int i, ret; > + struct kvm_one_reg reg; > + ARMCPU *cpu = ARM_CPU(cs); > + CPUARMState *env = &cpu->env; > + > + for (i = 0; i < 8; i++) { > + reg.id = AARCH64_CORE_REG(regs.regs[i]); > + reg.addr = (uintptr_t) &env->xregs[i]; > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > + if (ret) { > + return ret; > + } > + } > + > + return 0; > +} Wow, this is quite the restriction. I get that this is just enough to seed the guest for boot, and take SMC traps, but I'm concerned that we can't do much with the machine underneath, when it comes to other things like "info registers" or gdbstub will be silently unusable. I would prefer if we can somehow make this loudly unusable. Pardon if I'm jumping the gun and you handle this later. r~
On 1/27/23 05:07, Jean-Philippe Brucker wrote: > --- a/target/arm/kvm-rme.c > +++ b/target/arm/kvm-rme.c > @@ -118,6 +118,16 @@ int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp) > return 0; > } > > +int kvm_arm_rme_vcpu_init(CPUState *cs) > +{ > + ARMCPU *cpu = ARM_CPU(cs); > + > + if (kvm_arm_rme_enabled()) { > + cpu->kvm_rme = true; > + } > + return 0; > +} So... harking back to patch 3, how may times are we going to query the cast of machine->cgs to TYPE_RME_GUEST before we save this value? r~
On Fri, Jan 27, 2023 at 12:37:12PM -1000, Richard Henderson wrote: > On 1/27/23 05:07, Jean-Philippe Brucker wrote: > > +static int kvm_arm_rme_get_core_regs(CPUState *cs) > > +{ > > + int i, ret; > > + struct kvm_one_reg reg; > > + ARMCPU *cpu = ARM_CPU(cs); > > + CPUARMState *env = &cpu->env; > > + > > + for (i = 0; i < 8; i++) { > > + reg.id = AARCH64_CORE_REG(regs.regs[i]); > > + reg.addr = (uintptr_t) &env->xregs[i]; > > + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); > > + if (ret) { > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > Wow, this is quite the restriction. > > I get that this is just enough to seed the guest for boot, and take SMC > traps, but I'm concerned that we can't do much with the machine underneath, > when it comes to other things like "info registers" or gdbstub will be > silently unusable. I would prefer if we can somehow make this loudly > unusable. For "info registers", which currently displays zero values for all regs, we can instead return an error message in aarch64_cpu_dump_state(). For gdbstub, I suspect we should disable it entirely since it seems fundamentally incompatible with confidential VMs, but I need to spend more time on this. Thanks, Jean
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 9aeed3c848..7d8397985f 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -937,6 +937,9 @@ struct ArchCPU { /* KVM steal time */ OnOffAuto kvm_steal_time; + /* Realm Management Extension */ + bool kvm_rme; + /* Uniprocessor system with MP extensions */ bool mp_is_up; diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h index 00d3df8cac..e4dc7fbb8d 100644 --- a/target/arm/kvm_arm.h +++ b/target/arm/kvm_arm.h @@ -373,6 +373,7 @@ int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp); int kvm_arm_rme_vm_type(MachineState *ms); bool kvm_arm_rme_enabled(void); +int kvm_arm_rme_vcpu_init(CPUState *cs); #else diff --git a/target/arm/helper.c b/target/arm/helper.c index d8c8223ec3..52360ae2ff 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -126,6 +126,10 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync) int i; bool ok = true; + if (cpu->kvm_rme) { + return ok; + } + for (i = 0; i < cpu->cpreg_array_len; i++) { uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]); const ARMCPRegInfo *ri; @@ -171,6 +175,10 @@ bool write_list_to_cpustate(ARMCPU *cpu) int i; bool ok = true; + if (cpu->kvm_rme) { + return ok; + } + for (i = 0; i < cpu->cpreg_array_len; i++) { uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]); uint64_t v = cpu->cpreg_values[i]; diff --git a/target/arm/kvm-rme.c b/target/arm/kvm-rme.c index d7cdca1cbf..3833b187f9 100644 --- a/target/arm/kvm-rme.c +++ b/target/arm/kvm-rme.c @@ -118,6 +118,16 @@ int kvm_arm_rme_init(ConfidentialGuestSupport *cgs, Error **errp) return 0; } +int kvm_arm_rme_vcpu_init(CPUState *cs) +{ + ARMCPU *cpu = ARM_CPU(cs); + + if (kvm_arm_rme_enabled()) { + cpu->kvm_rme = true; + } + return 0; +} + int kvm_arm_rme_vm_type(MachineState *ms) { if (cgs_to_rme(ms->cgs)) { diff --git a/target/arm/kvm.c b/target/arm/kvm.c index f022c644d2..fcddead4fe 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -449,6 +449,10 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu) int i, ret, arraylen; CPUState *cs = CPU(cpu); + if (cpu->kvm_rme) { + return 0; + } + rl.n = 0; ret = kvm_vcpu_ioctl(cs, KVM_GET_REG_LIST, &rl); if (ret != -E2BIG) { @@ -521,6 +525,10 @@ bool write_kvmstate_to_list(ARMCPU *cpu) int i; bool ok = true; + if (cpu->kvm_rme) { + return ok; + } + for (i = 0; i < cpu->cpreg_array_len; i++) { struct kvm_one_reg r; uint64_t regidx = cpu->cpreg_indexes[i]; @@ -557,6 +565,10 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level) int i; bool ok = true; + if (cpu->kvm_rme) { + return ok; + } + for (i = 0; i < cpu->cpreg_array_len; i++) { struct kvm_one_reg r; uint64_t regidx = cpu->cpreg_indexes[i]; diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 55191496f3..b6320672b2 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -887,6 +887,11 @@ int kvm_arch_init_vcpu(CPUState *cs) return ret; } + ret = kvm_arm_rme_vcpu_init(cs); + if (ret) { + return ret; + } + if (cpu_isar_feature(aa64_sve, cpu)) { ret = kvm_arm_sve_set_vls(cs); if (ret) { @@ -1080,6 +1085,35 @@ static int kvm_arch_put_sve(CPUState *cs) return 0; } +static int kvm_arm_rme_put_core_regs(CPUState *cs, int level) +{ + int i, ret; + struct kvm_one_reg reg; + ARMCPU *cpu = ARM_CPU(cs); + CPUARMState *env = &cpu->env; + + /* + * The RME ABI only allows us to set 8 GPRs and the PC + */ + for (i = 0; i < 8; i++) { + reg.id = AARCH64_CORE_REG(regs.regs[i]); + reg.addr = (uintptr_t) &env->xregs[i]; + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret) { + return ret; + } + } + + reg.id = AARCH64_CORE_REG(regs.pc); + reg.addr = (uintptr_t) &env->pc; + ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®); + if (ret) { + return ret; + } + + return 0; +} + static int kvm_arm_put_core_regs(CPUState *cs) { struct kvm_one_reg reg; @@ -1208,7 +1242,11 @@ int kvm_arch_put_registers(CPUState *cs, int level) int ret; ARMCPU *cpu = ARM_CPU(cs); - ret = kvm_arm_put_core_regs(cs); + if (cpu->kvm_rme) { + ret = kvm_arm_rme_put_core_regs(cs, level); + } else { + ret = kvm_arm_put_core_regs(cs); + } if (ret) { return ret; } @@ -1306,6 +1344,25 @@ static int kvm_arch_get_sve(CPUState *cs) return 0; } +static int kvm_arm_rme_get_core_regs(CPUState *cs) +{ + int i, ret; + struct kvm_one_reg reg; + ARMCPU *cpu = ARM_CPU(cs); + CPUARMState *env = &cpu->env; + + for (i = 0; i < 8; i++) { + reg.id = AARCH64_CORE_REG(regs.regs[i]); + reg.addr = (uintptr_t) &env->xregs[i]; + ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®); + if (ret) { + return ret; + } + } + + return 0; +} + static int kvm_arm_get_core_regs(CPUState *cs) { struct kvm_one_reg reg; @@ -1434,7 +1491,11 @@ int kvm_arch_get_registers(CPUState *cs) int ret; ARMCPU *cpu = ARM_CPU(cs); - ret = kvm_arm_get_core_regs(cs); + if (cpu->kvm_rme) { + ret = kvm_arm_rme_get_core_regs(cs); + } else { + ret = kvm_arm_get_core_regs(cs); + } if (ret) { return ret; }
The target code calls kvm_arm_vcpu_init() to mark the vCPU as part of a realm. RME support does not use the register lists, because the host can only set the boot PC and registers x0-x7. The rest is private to the Realm and saved/restored by the RMM. Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> --- target/arm/cpu.h | 3 ++ target/arm/kvm_arm.h | 1 + target/arm/helper.c | 8 ++++++ target/arm/kvm-rme.c | 10 +++++++ target/arm/kvm.c | 12 ++++++++ target/arm/kvm64.c | 65 ++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 97 insertions(+), 2 deletions(-)