| Message ID | 20240813142835.77180-1-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make the exposed feature bits in AA64DFR0_EL1 writable from userspace | expand |
On Tue, 13 Aug 2024 15:28:35 +0100, Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > > KVM exposes the OS double lock feature bit to Guests but returns > RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between > systems where this feature support differ. Add support to make this > feature writable from userspace by setting the mask bit. While at it, > set the mask bits for other exposed features in the AA64DFR0_EL1 > register as well. > > Also update the selftest to cover these fields. > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > This is based on the discussion here(Thanks to Oliver), > https://lore.kernel.org/all/ZrVSlbVwnaMDShah@linux.dev/ > --- > arch/arm64/kvm/sys_regs.c | 6 +++++- > tools/testing/selftests/kvm/aarch64/set_id_regs.c | 4 ++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index c90324060436..adb49d681052 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -2376,7 +2376,11 @@ static const struct sys_reg_desc sys_reg_descs[] = { > .get_user = get_id_reg, > .set_user = set_id_aa64dfr0_el1, > .reset = read_sanitised_id_aa64dfr0_el1, > - .val = ID_AA64DFR0_EL1_PMUVer_MASK | > + .val = ID_AA64DFR0_EL1_DoubleLock_MASK | > + ID_AA64DFR0_EL1_CTX_CMPs_MASK | > + ID_AA64DFR0_EL1_WRPs_MASK | > + ID_AA64DFR0_EL1_BRPs_MASK | I think this is going to cause some troubles. The issue is that context-aware breakpoints are the highest-numbered breakpoints, right after the normal breakpoints (D2.8.3 "Breakpoint types and linking of breakpoints"). So if you reduce the number of normal breakpoints, you shift the context-aware ones down, and everything breaks. I really don't see how you can safely do that without completely changing the way we handle the debug registers. Thanks, M.
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: Tuesday, August 13, 2024 7:21 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: kvmarm@lists.linux.dev; linux-arm-kernel@lists.infradead.org; > will@kernel.org; catalin.marinas@arm.com; oliver.upton@linux.dev; > james.morse@arm.com; suzuki.poulose@arm.com; yuzenghui > <yuzenghui@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>; > Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH] KVM: arm64: Make the exposed feature bits in > AA64DFR0_EL1 writable from userspace > > On Tue, 13 Aug 2024 15:28:35 +0100, > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > > > > KVM exposes the OS double lock feature bit to Guests but returns > > RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between > > systems where this feature support differ. Add support to make this > > feature writable from userspace by setting the mask bit. While at it, > > set the mask bits for other exposed features in the AA64DFR0_EL1 > > register as well. > > > > Also update the selftest to cover these fields. > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > > --- > > This is based on the discussion here(Thanks to Oliver), > > https://lore.kernel.org/all/ZrVSlbVwnaMDShah@linux.dev/ > > --- > > arch/arm64/kvm/sys_regs.c | 6 +++++- > > tools/testing/selftests/kvm/aarch64/set_id_regs.c | 4 ++++ > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index c90324060436..adb49d681052 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -2376,7 +2376,11 @@ static const struct sys_reg_desc sys_reg_descs[] > = { > > .get_user = get_id_reg, > > .set_user = set_id_aa64dfr0_el1, > > .reset = read_sanitised_id_aa64dfr0_el1, > > - .val = ID_AA64DFR0_EL1_PMUVer_MASK | > > + .val = ID_AA64DFR0_EL1_DoubleLock_MASK | > > + ID_AA64DFR0_EL1_CTX_CMPs_MASK | > > + ID_AA64DFR0_EL1_WRPs_MASK | > > + ID_AA64DFR0_EL1_BRPs_MASK | > > > I think this is going to cause some troubles. > > The issue is that context-aware breakpoints are the highest-numbered > breakpoints, right after the normal breakpoints (D2.8.3 "Breakpoint > types and linking of breakpoints"). So if you reduce the number of > normal breakpoints, you shift the context-aware ones down, and > everything breaks. Thanks Marc for explaining this. I was not aware of this one. > I really don't see how you can safely do that without completely > changing the way we handle the debug registers. Looks like Reji has attempted to do this a while back, https://lore.kernel.org/kvm/20220419065544.3616948-13-reijiw@google.com/ I guess that one is trying to address the problem you described above, right? Though, not clear to me what happened afterwards to these patches in the series. Coming back to this patch, we don't have a requirement now to make the breakpoints writable for migration. The only concern is OS Double lock feature. Not sure anyone has a high priority requirement to make the other features writable or not. Will it be acceptable if I resent this patch with just OS Double Lock being writable?(Sorry If I sound selfish, but at least some progress can be made soon). Thanks, Shameer
On Wed, 14 Aug 2024 10:17:10 +0100, Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote: > > > > > -----Original Message----- > > From: Marc Zyngier <maz@kernel.org> > > Sent: Tuesday, August 13, 2024 7:21 PM > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > > Cc: kvmarm@lists.linux.dev; linux-arm-kernel@lists.infradead.org; > > will@kernel.org; catalin.marinas@arm.com; oliver.upton@linux.dev; > > james.morse@arm.com; suzuki.poulose@arm.com; yuzenghui > > <yuzenghui@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>; > > Linuxarm <linuxarm@huawei.com> > > Subject: Re: [PATCH] KVM: arm64: Make the exposed feature bits in > > AA64DFR0_EL1 writable from userspace > > > > On Tue, 13 Aug 2024 15:28:35 +0100, > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > > > > > > KVM exposes the OS double lock feature bit to Guests but returns > > > RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between > > > systems where this feature support differ. Add support to make this > > > feature writable from userspace by setting the mask bit. While at it, > > > set the mask bits for other exposed features in the AA64DFR0_EL1 > > > register as well. > > > > > > Also update the selftest to cover these fields. > > > > > > Signed-off-by: Shameer Kolothum > > <shameerali.kolothum.thodi@huawei.com> > > > --- > > > This is based on the discussion here(Thanks to Oliver), > > > https://lore.kernel.org/all/ZrVSlbVwnaMDShah@linux.dev/ > > > --- > > > arch/arm64/kvm/sys_regs.c | 6 +++++- > > > tools/testing/selftests/kvm/aarch64/set_id_regs.c | 4 ++++ > > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > > index c90324060436..adb49d681052 100644 > > > --- a/arch/arm64/kvm/sys_regs.c > > > +++ b/arch/arm64/kvm/sys_regs.c > > > @@ -2376,7 +2376,11 @@ static const struct sys_reg_desc sys_reg_descs[] > > = { > > > .get_user = get_id_reg, > > > .set_user = set_id_aa64dfr0_el1, > > > .reset = read_sanitised_id_aa64dfr0_el1, > > > - .val = ID_AA64DFR0_EL1_PMUVer_MASK | > > > + .val = ID_AA64DFR0_EL1_DoubleLock_MASK | > > > + ID_AA64DFR0_EL1_CTX_CMPs_MASK | > > > + ID_AA64DFR0_EL1_WRPs_MASK | > > > + ID_AA64DFR0_EL1_BRPs_MASK | > > > > > > I think this is going to cause some troubles. > > > > The issue is that context-aware breakpoints are the highest-numbered > > breakpoints, right after the normal breakpoints (D2.8.3 "Breakpoint > > types and linking of breakpoints"). So if you reduce the number of > > normal breakpoints, you shift the context-aware ones down, and > > everything breaks. > > Thanks Marc for explaining this. I was not aware of this one. Yeah, that's a pretty annoying shortcoming of the architecture. There is an effort to try and address it, but not sure when that will be fixed. > > > I really don't see how you can safely do that without completely > > changing the way we handle the debug registers. > > Looks like Reji has attempted to do this a while back, > https://lore.kernel.org/kvm/20220419065544.3616948-13-reijiw@google.com/ > > I guess that one is trying to address the problem you described > above, right? Though, not clear to me what happened afterwards to > these patches in the series. > > Coming back to this patch, we don't have a requirement now to make > the breakpoints writable for migration. The only concern is OS > Double lock feature. Not sure anyone has a high priority > requirement to make the other features writable or not. Will it be > acceptable if I resent this patch with just OS Double Lock being > writable?(Sorry If I sound selfish, but at least some progress can > be made soon). I think you can keep all the other two fields, as they are independent. You could add a comment indicating why we can't just let userspace change this field. Thanks, M.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index c90324060436..adb49d681052 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2376,7 +2376,11 @@ static const struct sys_reg_desc sys_reg_descs[] = { .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, .reset = read_sanitised_id_aa64dfr0_el1, - .val = ID_AA64DFR0_EL1_PMUVer_MASK | + .val = ID_AA64DFR0_EL1_DoubleLock_MASK | + ID_AA64DFR0_EL1_CTX_CMPs_MASK | + ID_AA64DFR0_EL1_WRPs_MASK | + ID_AA64DFR0_EL1_BRPs_MASK | + ID_AA64DFR0_EL1_PMUVer_MASK | ID_AA64DFR0_EL1_DebugVer_MASK, }, ID_SANITISED(ID_AA64DFR1_EL1), ID_UNALLOCATED(5,2), diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c index d20981663831..1e6b9594daf8 100644 --- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c @@ -68,6 +68,10 @@ struct test_feature_reg { } static const struct reg_ftr_bits ftr_id_aa64dfr0_el1[] = { + S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DoubleLock, 0), + REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, CTX_CMPs, 0), + REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, WRPs, 0), + REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, BRPs, 0), S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, PMUVer, 0), REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DebugVer, ID_AA64DFR0_EL1_DebugVer_IMP), REG_FTR_END,
KVM exposes the OS double lock feature bit to Guests but returns RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between systems where this feature support differ. Add support to make this feature writable from userspace by setting the mask bit. While at it, set the mask bits for other exposed features in the AA64DFR0_EL1 register as well. Also update the selftest to cover these fields. Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- This is based on the discussion here(Thanks to Oliver), https://lore.kernel.org/all/ZrVSlbVwnaMDShah@linux.dev/ --- arch/arm64/kvm/sys_regs.c | 6 +++++- tools/testing/selftests/kvm/aarch64/set_id_regs.c | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-)