Hey, On Mon, Jun 03, 2024 at 01:36:54PM +0100, Marc Zyngier wrote: [...] > > + /* > > + * Layer the guest hypervisor's trap configuration on top of our own if > > + * we're in a nested context. > > + */ > > + if (!vcpu_has_nv(vcpu) || is_hyp_ctxt(vcpu)) > > + goto write; > > + > > + if (guest_hyp_fpsimd_traps_enabled(vcpu)) > > + val &= ~CPACR_ELx_FPEN; > > + if (guest_hyp_sve_traps_enabled(vcpu)) > > + val &= ~CPACR_ELx_ZEN; > > I'm afraid this isn't quite right. You are clearing both FPEN (resp > ZEN) bits based on any of the two bits being clear, while what we want > is to actually propagate the 0 bits (and only those). An earlier version of the series I had was effectively doing this, applying the L0 trap configuration on top of L1's CPTR_EL2. Unless I'm missing something terribly obvious, I think this is still correct, as: - If we're in a hyp context, vEL2's CPTR_EL2 is loaded into CPACR_EL1. The independent EL0/EL1 enable bits are handled by hardware. All this junk gets skipped and we go directly to writing CPTR_EL2. - If we are not in a hyp context, vEL2's CPTR_EL2 gets folded into the hardware value for CPTR_EL2. TGE must be 0 in this case, so there is no conditional trap based on what EL the vCPU is in. There's only two functional trap states at this point, hence the all-or-nothing approach. > What I have in my tree is something along the lines of: > > cptr = vcpu_sanitised_cptr_el2(vcpu); > tmp = cptr & (CPACR_ELx_ZEN_MASK | CPACR_ELx_FPEN_MASK); > val &= ~(tmp ^ (CPACR_ELx_ZEN_MASK | CPACR_ELx_FPEN_MASK)); My hesitation with this is it gives the impression that both trap bits are significant, but in reality only the LSB is useful. Unless my understanding is disastrously wrong, of course :) Anyway, my _slight_ preference is towards keeping what I have if possible, with a giant comment explaining the reasoning behind it. But I can take your approach instead too. -- Thanks, Oliver
