Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Thu, 29 Feb 2024 12:50:24 +0000 | From | Mark Rutland <> | Subject | Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions |
| |
Hi Suzuki,
On Thu, Feb 29, 2024 at 11:45:08AM +0000, Suzuki K Poulose wrote: > On 27/02/2024 11:13, Anshuman Khandual wrote: > > On 2/27/24 15:34, Mark Rutland wrote: > > > On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote: > > > > On 2/21/24 19:31, Mark Rutland wrote: > > > > > On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote: > > > > > > Currently BRBE feature is not supported in a guest environment. This hides > > > > > > BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field. > > > > > > > > > > Does that means that a guest can currently see BRBE advertised in the > > > > > ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code > > > > > today? > > > > > > > > IIRC it is hidden, but will have to double check. When experimenting for BRBE > > > > guest support enablement earlier, following changes were need for the feature > > > > to be visible in ID_AA64DFR0_EL1. > > > > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > > > index 646591c67e7a..f258568535a8 100644 > > > > --- a/arch/arm64/kernel/cpufeature.c > > > > +++ b/arch/arm64/kernel/cpufeature.c > > > > @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = { > > > > }; > > > > static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = { > > > > + S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP), > > > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0), > > > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0), > > > > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0), > > > > > > > > Should we add the following entry - explicitly hiding BRBE from the guest > > > > as a prerequisite patch ? > > This has nothing to do with the Guest visibility of the BRBE. This is > specifically for host "userspace" (via MRS emulation). > > > > > > > > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI) > > > > > > Is it visbile currently, or is it hidden currently? > > > > > > * If it is visible before this patch, that's a latent bug that we need to go > > > fix first, and that'll require more coordination. > > > > > > * If it is not visible before this patch, there's no problem in the code, but > > > the commit message needs to explicitly mention that's the case as the commit > > > message currently implies it is visible by only mentioning hiding it. > > > > > > ... so can you please double check as you suggested above? We should be able to > > > explain why it is or is not visible today. > > > > It is currently hidden i.e following code returns 1 in the host > > but returns 0 inside the guest. > > > > aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1); > > brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT); > > > > Hence - will update the commit message here as suggested. > > This is by virtue of the masking we do in the kvm/sysreg.c below.
Yep, once this patch is applied.
I think we might have some crossed wires here; I'm only really asking for the commit message (and title) to be updated and clarified.
Ignoring the patchlet above, and just considering the original patch:
IIUC before the patch is applied, the ID_AA64DFR0_EL1.BRBE field is zero for the guest because we don't have an arm64_ftr_bits entry for the ID_AA64DFR0_EL1.BRBE field, and so init_cpu_ftr_reg() will leave that as zero in arm64_ftr_reg::sys_val, and hence when read_sanitised_id_aa64dfr0_el1() calls read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), the BRBE field will be zero.
This series as-is doesn't add an arm64_ftr_bits entry for ID_AA64DFR0_EL1.BRBE, so it'd still be hidden from a guest regardless of whether we add explicit masking in read_sanitised_id_aa64dfr0_el1(). The reason to add that masking is to be explicit, so that if/when we add an arm64_ftr_bits entry for ID_AA64DFR0_EL1.BRBE, it isn't exposed to a guest unexpectedly.
Similarly, IIUC the BRBE register accesses are *already* trapped, and emulate_sys_reg() will log a warning an inject an UNDEFINED exception into the guest if the guest tries to access the BRBE registers. Any well-behaved guest *shouldn't* do that, but a poorly-behaved guest could do that and (slowly) spam dmesg with messages about the unhandled sysreg traps. The reasons to handle thos regs is largely to suppress that warning, and to make it clear that we intend for those to be handled as undef. So the commit title should be something like:
KVM: arm64: explicitly handle BRBE register accesses as UNDEFINED
.. and the message should mention the key points from the above.
Suzuki, does that sound right to you?
Anshuman, can you go re-write the commit message with that in mind?
Mark.
| ![\](/images/icornerr.gif) |