| Message ID | 20171125174031.13117-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Nov 25, 2017 at 06:40:31PM +0100, Andrew Jones wrote: > Since commit 93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU > features from guests") we can hide cpu features from guests. Apply > this to a long standing issue where guests see a PMU available, but > it's not, because it was not enabled by KVM's userspace. > > Signed-off-by: Andrew Jones <drjones@redhat.com> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > arch/arm64/kvm/sys_regs.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1830ebc227d1..503144f13af0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -881,18 +881,25 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu, > } > > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > -static u64 read_id_reg(struct sys_reg_desc const *r, bool raz) > +static u64 read_id_reg(struct kvm_vcpu *vcpu, > + struct sys_reg_desc const *r, > + bool raz) > { > u32 id = sys_reg((u32)r->Op0, (u32)r->Op1, > (u32)r->CRn, (u32)r->CRm, (u32)r->Op2); > u64 val = raz ? 0 : read_sanitised_ftr_reg(id); > > - if (id == SYS_ID_AA64PFR0_EL1) { > + switch (id) { > + case SYS_ID_AA64DFR0_EL1: > + if (!kvm_arm_pmu_v3_ready(vcpu)) > + val &= ~(0xfUL << ID_AA64DFR0_PMUVER_SHIFT); > + break; > + case SYS_ID_AA64PFR0_EL1: > if (val & (0xfUL << ID_AA64PFR0_SVE_SHIFT)) > pr_err_once("kvm [%i]: SVE unsupported for guests, suppressing\n", > task_pid_nr(current)); > - > val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT); > + break; > } > > return val; > @@ -908,7 +915,7 @@ static bool __access_id_reg(struct kvm_vcpu *vcpu, > if (p->is_write) > return write_to_read_only(vcpu, p, r); > > - p->regval = read_id_reg(r, raz); > + p->regval = read_id_reg(vcpu, r, raz); > return true; > } > > @@ -937,17 +944,17 @@ static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > * are stored, and for set_id_reg() we don't allow the effective value > * to be changed. > */ > -static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > - bool raz) > +static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + void __user *uaddr, bool raz) > { > const u64 id = sys_reg_to_index(rd); > - const u64 val = read_id_reg(rd, raz); > + const u64 val = read_id_reg(vcpu, rd, raz); > > return reg_to_user(uaddr, &val, id); > } > > -static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > - bool raz) > +static int __set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + void __user *uaddr, bool raz) > { > const u64 id = sys_reg_to_index(rd); > int err; > @@ -958,7 +965,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > return err; > > /* This is what we mean by invariant: you can't change it. */ > - if (val != read_id_reg(rd, raz)) > + if (val != read_id_reg(vcpu, rd, raz)) > return -EINVAL; > > return 0; > @@ -967,25 +974,25 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr) > { > - return __get_id_reg(rd, uaddr, false); > + return __get_id_reg(vcpu, rd, uaddr, false); > } > > static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr) > { > - return __set_id_reg(rd, uaddr, false); > + return __set_id_reg(vcpu, rd, uaddr, false); > } > > static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr) > { > - return __get_id_reg(rd, uaddr, true); > + return __get_id_reg(vcpu, rd, uaddr, true); > } > > static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > const struct kvm_one_reg *reg, void __user *uaddr) > { > - return __set_id_reg(rd, uaddr, true); > + return __set_id_reg(vcpu, rd, uaddr, true); > } > > /* sys_reg_desc initialiser for known cpufeature ID registers */ > -- > 2.13.6 >
Hi Drew, On Sat, Nov 25, 2017 at 06:40:31PM +0100, Andrew Jones wrote: > Since commit 93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU > features from guests") we can hide cpu features from guests. Apply > this to a long standing issue where guests see a PMU available, but > it's not, because it was not enabled by KVM's userspace. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > arch/arm64/kvm/sys_regs.c | 35 +++++++++++++++++++++-------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1830ebc227d1..503144f13af0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -881,18 +881,25 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu, > } > > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > -static u64 read_id_reg(struct sys_reg_desc const *r, bool raz) > +static u64 read_id_reg(struct kvm_vcpu *vcpu, > + struct sys_reg_desc const *r, > + bool raz) > { > u32 id = sys_reg((u32)r->Op0, (u32)r->Op1, > (u32)r->CRn, (u32)r->CRm, (u32)r->Op2); > u64 val = raz ? 0 : read_sanitised_ftr_reg(id); > > - if (id == SYS_ID_AA64PFR0_EL1) { > + switch (id) { > + case SYS_ID_AA64DFR0_EL1: > + if (!kvm_arm_pmu_v3_ready(vcpu)) > + val &= ~(0xfUL << ID_AA64DFR0_PMUVER_SHIFT); This is actually problematic, becuase it can break migration. If user space reads the guest state at some point after the guest has been started with vPMU enabled, but then tries to restore the state before creating a pmu, then we get an error. See below... > + break; > + case SYS_ID_AA64PFR0_EL1: > if (val & (0xfUL << ID_AA64PFR0_SVE_SHIFT)) > pr_err_once("kvm [%i]: SVE unsupported for guests, suppressing\n", > task_pid_nr(current)); > - > val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT); > + break; > } > > return val; > @@ -908,7 +915,7 @@ static bool __access_id_reg(struct kvm_vcpu *vcpu, > if (p->is_write) > return write_to_read_only(vcpu, p, r); > > - p->regval = read_id_reg(r, raz); > + p->regval = read_id_reg(vcpu, r, raz); > return true; > } > > @@ -937,17 +944,17 @@ static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > * are stored, and for set_id_reg() we don't allow the effective value > * to be changed. > */ > -static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > - bool raz) > +static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + void __user *uaddr, bool raz) > { > const u64 id = sys_reg_to_index(rd); > - const u64 val = read_id_reg(rd, raz); > + const u64 val = read_id_reg(vcpu, rd, raz); > > return reg_to_user(uaddr, &val, id); > } > > -static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > - bool raz) > +static int __set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + void __user *uaddr, bool raz) > { > const u64 id = sys_reg_to_index(rd); > int err; > @@ -958,7 +965,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > return err; > > /* This is what we mean by invariant: you can't change it. */ > - if (val != read_id_reg(rd, raz)) > + if (val != read_id_reg(vcpu, rd, raz)) So since we modified read_id_reg above for the PMU, we have now broken migration, because we've introduced an implicit ordering requirement for creating the PMU. One way to handle this is to remove this check at this point and verify integrity when we're about to run a VCPU, but that changes behavior and we've been happy with the invariant checks so far. Perhaps a better approach is to let userspace write ID register values that can be hidden, and then simply mask of features when the guest is running which would allow snapshotting the id register values any time before/after adding all features/peripherals to the VCPUs. Thoughts? I'll drop this patch from queue and next for now until we have a better solution. Thanks, -Christoffer
On Mon, Jan 08, 2018 at 03:18:15PM +0100, Christoffer Dall wrote: > Hi Drew, > > On Sat, Nov 25, 2017 at 06:40:31PM +0100, Andrew Jones wrote: > > Since commit 93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU > > features from guests") we can hide cpu features from guests. Apply > > this to a long standing issue where guests see a PMU available, but > > it's not, because it was not enabled by KVM's userspace. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > arch/arm64/kvm/sys_regs.c | 35 +++++++++++++++++++++-------------- > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index 1830ebc227d1..503144f13af0 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -881,18 +881,25 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu, > > } > > > > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > > -static u64 read_id_reg(struct sys_reg_desc const *r, bool raz) > > +static u64 read_id_reg(struct kvm_vcpu *vcpu, > > + struct sys_reg_desc const *r, > > + bool raz) > > { > > u32 id = sys_reg((u32)r->Op0, (u32)r->Op1, > > (u32)r->CRn, (u32)r->CRm, (u32)r->Op2); > > u64 val = raz ? 0 : read_sanitised_ftr_reg(id); > > > > - if (id == SYS_ID_AA64PFR0_EL1) { > > + switch (id) { > > + case SYS_ID_AA64DFR0_EL1: > > + if (!kvm_arm_pmu_v3_ready(vcpu)) > > + val &= ~(0xfUL << ID_AA64DFR0_PMUVER_SHIFT); > > This is actually problematic, becuase it can break migration. If > user space reads the guest state at some point after the guest has been > started with vPMU enabled, but then tries to restore the state before > creating a pmu, then we get an error. See below... > > > + break; > > + case SYS_ID_AA64PFR0_EL1: > > if (val & (0xfUL << ID_AA64PFR0_SVE_SHIFT)) > > pr_err_once("kvm [%i]: SVE unsupported for guests, suppressing\n", > > task_pid_nr(current)); > > - > > val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT); > > + break; > > } > > > > return val; > > @@ -908,7 +915,7 @@ static bool __access_id_reg(struct kvm_vcpu *vcpu, > > if (p->is_write) > > return write_to_read_only(vcpu, p, r); > > > > - p->regval = read_id_reg(r, raz); > > + p->regval = read_id_reg(vcpu, r, raz); > > return true; > > } > > > > @@ -937,17 +944,17 @@ static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > > * are stored, and for set_id_reg() we don't allow the effective value > > * to be changed. > > */ > > -static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > > - bool raz) > > +static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > + void __user *uaddr, bool raz) > > { > > const u64 id = sys_reg_to_index(rd); > > - const u64 val = read_id_reg(rd, raz); > > + const u64 val = read_id_reg(vcpu, rd, raz); > > > > return reg_to_user(uaddr, &val, id); > > } > > > > -static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > > - bool raz) > > +static int __set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > + void __user *uaddr, bool raz) > > { > > const u64 id = sys_reg_to_index(rd); > > int err; > > @@ -958,7 +965,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > > return err; > > > > /* This is what we mean by invariant: you can't change it. */ > > - if (val != read_id_reg(rd, raz)) > > + if (val != read_id_reg(vcpu, rd, raz)) > > So since we modified read_id_reg above for the PMU, we have now broken > migration, because we've introduced an implicit ordering requirement for > creating the PMU. > > One way to handle this is to remove this check at this point and verify > integrity when we're about to run a VCPU, but that changes behavior and > we've been happy with the invariant checks so far. > > Perhaps a better approach is to let userspace write ID register values > that can be hidden, and then simply mask of features when the guest is > running which would allow snapshotting the id register values any time > before/after adding all features/peripherals to the VCPUs. > > Thoughts? > > I'll drop this patch from queue and next for now until we have a better > solution. Argh, sorry about this. I'll revisit it soon. Thanks, drew
On Mon, Jan 08, 2018 at 03:56:29PM +0100, Andrew Jones wrote: > On Mon, Jan 08, 2018 at 03:18:15PM +0100, Christoffer Dall wrote: > > Hi Drew, > > > > On Sat, Nov 25, 2017 at 06:40:31PM +0100, Andrew Jones wrote: > > > Since commit 93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU > > > features from guests") we can hide cpu features from guests. Apply > > > this to a long standing issue where guests see a PMU available, but > > > it's not, because it was not enabled by KVM's userspace. > > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > > --- > > > arch/arm64/kvm/sys_regs.c | 35 +++++++++++++++++++++-------------- > > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index 1830ebc227d1..503144f13af0 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -881,18 +881,25 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu, > > > } > > > > > > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > > > -static u64 read_id_reg(struct sys_reg_desc const *r, bool raz) > > > +static u64 read_id_reg(struct kvm_vcpu *vcpu, > > > + struct sys_reg_desc const *r, > > > + bool raz) > > > { > > > u32 id = sys_reg((u32)r->Op0, (u32)r->Op1, > > > (u32)r->CRn, (u32)r->CRm, (u32)r->Op2); > > > u64 val = raz ? 0 : read_sanitised_ftr_reg(id); > > > > > > - if (id == SYS_ID_AA64PFR0_EL1) { > > > + switch (id) { > > > + case SYS_ID_AA64DFR0_EL1: > > > + if (!kvm_arm_pmu_v3_ready(vcpu)) > > > + val &= ~(0xfUL << ID_AA64DFR0_PMUVER_SHIFT); > > > > This is actually problematic, becuase it can break migration. If > > user space reads the guest state at some point after the guest has been > > started with vPMU enabled, but then tries to restore the state before > > creating a pmu, then we get an error. See below... > > > > > + break; > > > + case SYS_ID_AA64PFR0_EL1: > > > if (val & (0xfUL << ID_AA64PFR0_SVE_SHIFT)) > > > pr_err_once("kvm [%i]: SVE unsupported for guests, suppressing\n", > > > task_pid_nr(current)); > > > - > > > val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT); > > > + break; > > > } > > > > > > return val; > > > @@ -908,7 +915,7 @@ static bool __access_id_reg(struct kvm_vcpu *vcpu, > > > if (p->is_write) > > > return write_to_read_only(vcpu, p, r); > > > > > > - p->regval = read_id_reg(r, raz); > > > + p->regval = read_id_reg(vcpu, r, raz); > > > return true; > > > } > > > > > > @@ -937,17 +944,17 @@ static u64 sys_reg_to_index(const struct sys_reg_desc *reg); > > > * are stored, and for set_id_reg() we don't allow the effective value > > > * to be changed. > > > */ > > > -static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > > > - bool raz) > > > +static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > > + void __user *uaddr, bool raz) > > > { > > > const u64 id = sys_reg_to_index(rd); > > > - const u64 val = read_id_reg(rd, raz); > > > + const u64 val = read_id_reg(vcpu, rd, raz); > > > > > > return reg_to_user(uaddr, &val, id); > > > } > > > > > > -static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > > > - bool raz) > > > +static int __set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > > > + void __user *uaddr, bool raz) > > > { > > > const u64 id = sys_reg_to_index(rd); > > > int err; > > > @@ -958,7 +965,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, > > > return err; > > > > > > /* This is what we mean by invariant: you can't change it. */ > > > - if (val != read_id_reg(rd, raz)) > > > + if (val != read_id_reg(vcpu, rd, raz)) > > > > So since we modified read_id_reg above for the PMU, we have now broken > > migration, because we've introduced an implicit ordering requirement for > > creating the PMU. > > > > One way to handle this is to remove this check at this point and verify > > integrity when we're about to run a VCPU, but that changes behavior and > > we've been happy with the invariant checks so far. > > > > Perhaps a better approach is to let userspace write ID register values > > that can be hidden, and then simply mask of features when the guest is > > running which would allow snapshotting the id register values any time > > before/after adding all features/peripherals to the VCPUs. > > > > Thoughts? > > > > I'll drop this patch from queue and next for now until we have a better > > solution. > > Argh, sorry about this. I'll revisit it soon. > No worries. Thanks for taking a look. -Christoffer
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 1830ebc227d1..503144f13af0 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -881,18 +881,25 @@ static bool access_cntp_cval(struct kvm_vcpu *vcpu, } /* Read a sanitised cpufeature ID register by sys_reg_desc */ -static u64 read_id_reg(struct sys_reg_desc const *r, bool raz) +static u64 read_id_reg(struct kvm_vcpu *vcpu, + struct sys_reg_desc const *r, + bool raz) { u32 id = sys_reg((u32)r->Op0, (u32)r->Op1, (u32)r->CRn, (u32)r->CRm, (u32)r->Op2); u64 val = raz ? 0 : read_sanitised_ftr_reg(id); - if (id == SYS_ID_AA64PFR0_EL1) { + switch (id) { + case SYS_ID_AA64DFR0_EL1: + if (!kvm_arm_pmu_v3_ready(vcpu)) + val &= ~(0xfUL << ID_AA64DFR0_PMUVER_SHIFT); + break; + case SYS_ID_AA64PFR0_EL1: if (val & (0xfUL << ID_AA64PFR0_SVE_SHIFT)) pr_err_once("kvm [%i]: SVE unsupported for guests, suppressing\n", task_pid_nr(current)); - val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT); + break; } return val; @@ -908,7 +915,7 @@ static bool __access_id_reg(struct kvm_vcpu *vcpu, if (p->is_write) return write_to_read_only(vcpu, p, r); - p->regval = read_id_reg(r, raz); + p->regval = read_id_reg(vcpu, r, raz); return true; } @@ -937,17 +944,17 @@ static u64 sys_reg_to_index(const struct sys_reg_desc *reg); * are stored, and for set_id_reg() we don't allow the effective value * to be changed. */ -static int __get_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, - bool raz) +static int __get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, + void __user *uaddr, bool raz) { const u64 id = sys_reg_to_index(rd); - const u64 val = read_id_reg(rd, raz); + const u64 val = read_id_reg(vcpu, rd, raz); return reg_to_user(uaddr, &val, id); } -static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, - bool raz) +static int __set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, + void __user *uaddr, bool raz) { const u64 id = sys_reg_to_index(rd); int err; @@ -958,7 +965,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, return err; /* This is what we mean by invariant: you can't change it. */ - if (val != read_id_reg(rd, raz)) + if (val != read_id_reg(vcpu, rd, raz)) return -EINVAL; return 0; @@ -967,25 +974,25 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr, static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, const struct kvm_one_reg *reg, void __user *uaddr) { - return __get_id_reg(rd, uaddr, false); + return __get_id_reg(vcpu, rd, uaddr, false); } static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, const struct kvm_one_reg *reg, void __user *uaddr) { - return __set_id_reg(rd, uaddr, false); + return __set_id_reg(vcpu, rd, uaddr, false); } static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, const struct kvm_one_reg *reg, void __user *uaddr) { - return __get_id_reg(rd, uaddr, true); + return __get_id_reg(vcpu, rd, uaddr, true); } static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, const struct kvm_one_reg *reg, void __user *uaddr) { - return __set_id_reg(rd, uaddr, true); + return __set_id_reg(vcpu, rd, uaddr, true); } /* sys_reg_desc initialiser for known cpufeature ID registers */
Since commit 93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests") we can hide cpu features from guests. Apply this to a long standing issue where guests see a PMU available, but it's not, because it was not enabled by KVM's userspace. Signed-off-by: Andrew Jones <drjones@redhat.com> --- arch/arm64/kvm/sys_regs.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)