| Message ID | 20240718035017.434996-3-shahuang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow userspace to change ID_AA64PFR1_EL1 | expand |
On Wed, Jul 17, 2024 at 11:50:15PM -0400, Shaoqin Huang wrote: > Allow userspace to change the guest-visible value of the register with > different way of handling: > > - Since the RAS and MPAM is not writable in the ID_AA64PFR0_EL1 > register, RAS_frac and MPAM_frac are also not writable in the > ID_AA64PFR1_EL1 register. > > - The MTE is controlled by an internal flag (KVM_ARCH_FLAG_MTE_ENABLED), > so it's not writable. The flag isn't the relevant part, what's important about MTE is that it already has a separate UAPI for controlling it (KVM_CAP_ARM_MTE). > - For those fields which KVM doesn't know how to handle, they have > are not exposed to the guest (being disabled in the register read > accessor), those fields value will always be 0. Allow those fields > writable is fine, since the userspace can only write 0 into those > fields. Maybe in the future KVM know how to handle some of the > fields, then they can be written into other value. > So let them writable. > Those fields include SME, RNDR_trap, NMI, GCS, THE, DF2, PFAR, > MTE_frac, MTEX. This doesn't seem right. We're committing to a UAPI behavior the moment these fields are advertised to userspace, which is rather difficult to do for features that we don't even implement. Please only advertise the fields known to KVM and leave the others unadvertised. > - The BT, SSBS, CSV2_frac don't introduce any new registers which KVM > doesn't know how to handle, they can be written without ill effect. > So let them writable. I think the handling of ARM_SMCCC_ARCH_WORKAROUND_2 needs to be updated to consider the presence of FEAT_SSBS in the guest's ID registers. Otherwise we'll wind up returning NOT_SUPPORTED and the guest will conclude it is in a vulnerable state.
Hi Oliver, On 7/18/24 14:35, Oliver Upton wrote: > On Wed, Jul 17, 2024 at 11:50:15PM -0400, Shaoqin Huang wrote: >> Allow userspace to change the guest-visible value of the register with >> different way of handling: >> >> - Since the RAS and MPAM is not writable in the ID_AA64PFR0_EL1 >> register, RAS_frac and MPAM_frac are also not writable in the >> ID_AA64PFR1_EL1 register. >> >> - The MTE is controlled by an internal flag (KVM_ARCH_FLAG_MTE_ENABLED), >> so it's not writable. > > The flag isn't the relevant part, what's important about MTE is that it > already has a separate UAPI for controlling it (KVM_CAP_ARM_MTE). I'm not quite understand why KVM_ARCH_FLAG_MTE_ENABLED isn't the relevant part. I see this capability, when enable the KVM_CAP_ARM_MTE, it set the KVM_ARCH_FLAG_MTE_ENABLED in the kvm->arch.flags. And do you mean we should update it like "The MTE is controlled by a UAPI (KVM_CAP_ARM_MTE)"? > >> - For those fields which KVM doesn't know how to handle, they have >> are not exposed to the guest (being disabled in the register read >> accessor), those fields value will always be 0. Allow those fields >> writable is fine, since the userspace can only write 0 into those >> fields. Maybe in the future KVM know how to handle some of the >> fields, then they can be written into other value. >> So let them writable. >> Those fields include SME, RNDR_trap, NMI, GCS, THE, DF2, PFAR, >> MTE_frac, MTEX. > > This doesn't seem right. We're committing to a UAPI behavior the moment > these fields are advertised to userspace, which is rather difficult to > do for features that we don't even implement. > > Please only advertise the fields known to KVM and leave the others > unadvertised. Thanks a lot for pointing this out. Now I get the point, for those not implemented feature, they should not writable, so they're not advertised to userspace. > >> - The BT, SSBS, CSV2_frac don't introduce any new registers which KVM >> doesn't know how to handle, they can be written without ill effect. >> So let them writable. > > I think the handling of ARM_SMCCC_ARCH_WORKAROUND_2 needs to be updated > to consider the presence of FEAT_SSBS in the guest's ID registers. > Otherwise we'll wind up returning NOT_SUPPORTED and the guest will > conclude it is in a vulnerable state. I see that line of code, in the case ARM_SMCCC_ARCH_WORKAROUND_2: if (cpus_have_final_cap(ARM64_SSBS)) break; I guess we should update it with something like if (SYS_FIELD_GET(ID_AA64PFR1_EL1, SSBS, IDREG(kvm, SYS_ID_AA64PFR1_EL1)) != 0) Oliver, Is there any proper function or macro implement the checking like above? I don't find the similar checking in the code. Thanks, Shaoqin >
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4508288b9d38..26b38165a8be 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2314,7 +2314,10 @@ static const struct sys_reg_desc sys_reg_descs[] = { ID_AA64PFR0_EL1_GIC | ID_AA64PFR0_EL1_AdvSIMD | ID_AA64PFR0_EL1_FP), }, - ID_SANITISED(ID_AA64PFR1_EL1), + ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RES0 | + ID_AA64PFR1_EL1_MPAM_frac | + ID_AA64PFR1_EL1_RAS_frac | + ID_AA64PFR1_EL1_MTE)), ID_UNALLOCATED(4,2), ID_UNALLOCATED(4,3), ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
Allow userspace to change the guest-visible value of the register with different way of handling: - Since the RAS and MPAM is not writable in the ID_AA64PFR0_EL1 register, RAS_frac and MPAM_frac are also not writable in the ID_AA64PFR1_EL1 register. - The MTE is controlled by an internal flag (KVM_ARCH_FLAG_MTE_ENABLED), so it's not writable. - For those fields which KVM doesn't know how to handle, they have are not exposed to the guest (being disabled in the register read accessor), those fields value will always be 0. Allow those fields writable is fine, since the userspace can only write 0 into those fields. Maybe in the future KVM know how to handle some of the fields, then they can be written into other value. So let them writable. Those fields include SME, RNDR_trap, NMI, GCS, THE, DF2, PFAR, MTE_frac, MTEX. - The BT, SSBS, CSV2_frac don't introduce any new registers which KVM doesn't know how to handle, they can be written without ill effect. So let them writable. Besides, we don't do the crosscheck in KVM about the CSV2_frac even if it depends on the value of CSV2, it should be made sure by the VMM instead of KVM. Signed-off-by: Shaoqin Huang <shahuang@redhat.com> --- arch/arm64/kvm/sys_regs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)