Hi,
On Tue, Jun 25, 2024 at 02:35:07PM +0100, Marc Zyngier wrote:
> On the face of it, AT S12E{0,1}{R,W} is pretty simple. It is the
> combination of AT S1E{0,1}{R,W}, followed by an extra S2 walk.
>
> However, there is a great deal of complexity coming from combining
> the S1 and S2 attributes to report something consistent in PAR_EL1.
>
> This is an absolute mine field, and I have a splitting headache.
>
> [..]
> +static u8 compute_sh(u8 attr, u64 desc)
> +{
> + /* Any form of device, as well as NC has SH[1:0]=0b10 */
> + if (MEMATTR_IS_DEVICE(attr) || attr == MEMATTR(NC, NC))
> + return 0b10;
> +
> + return FIELD_GET(PTE_SHARED, desc) == 0b11 ? 0b11 : 0b10;
If shareability is 0b00 (non-shareable), the PAR_EL1.SH field will be 0b10
(outer-shareable), which seems to be contradicting PAREncodeShareability().
> +}
> +
> +static u64 compute_par_s12(struct kvm_vcpu *vcpu, u64 s1_par,
> + struct kvm_s2_trans *tr)
> +{
> + u8 s1_parattr, s2_memattr, final_attr;
> + u64 par;
> +
> + /* If S2 has failed to translate, report the damage */
> + if (tr->esr) {
> + par = SYS_PAR_EL1_RES1;
> + par |= SYS_PAR_EL1_F;
> + par |= SYS_PAR_EL1_S;
> + par |= FIELD_PREP(SYS_PAR_EL1_FST, tr->esr);
> + return par;
> + }
> +
> + s1_parattr = FIELD_GET(SYS_PAR_EL1_ATTR, s1_par);
> + s2_memattr = FIELD_GET(GENMASK(5, 2), tr->desc);
> +
> + if (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_FWB) {
> + if (!kvm_has_feat(vcpu->kvm, ID_AA64PFR2_EL1, MTEPERM, IMP))
> + s2_memattr &= ~BIT(3);
> +
> + /* Combination of R_VRJSW and R_RHWZM */
> + switch (s2_memattr) {
> + case 0b0101:
> + if (MEMATTR_IS_DEVICE(s1_parattr))
> + final_attr = s1_parattr;
> + else
> + final_attr = MEMATTR(NC, NC);
> + break;
> + case 0b0110:
> + case 0b1110:
> + final_attr = MEMATTR(WbRaWa, WbRaWa);
> + break;
> + case 0b0111:
> + case 0b1111:
> + /* Preserve S1 attribute */
> + final_attr = s1_parattr;
> + break;
> + case 0b0100:
> + case 0b1100:
> + case 0b1101:
> + /* Reserved, do something non-silly */
> + final_attr = s1_parattr;
> + break;
> + default:
> + /* MemAttr[2]=0, Device from S2 */
> + final_attr = s2_memattr & GENMASK(1,0) << 2;
> + }
> + } else {
> + /* Combination of R_HMNDG, R_TNHFM and R_GQFSF */
> + u8 s2_parattr = s2_memattr_to_attr(s2_memattr);
> +
> + if (MEMATTR_IS_DEVICE(s1_parattr) ||
> + MEMATTR_IS_DEVICE(s2_parattr)) {
> + final_attr = min(s1_parattr, s2_parattr);
> + } else {
> + /* At this stage, this is memory vs memory */
> + final_attr = combine_s1_s2_attr(s1_parattr & 0xf,
> + s2_parattr & 0xf);
> + final_attr |= combine_s1_s2_attr(s1_parattr >> 4,
> + s2_parattr >> 4) << 4;
> + }
> + }
> +
> + if ((__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_CD) &&
> + !MEMATTR_IS_DEVICE(final_attr))
> + final_attr = MEMATTR(NC, NC);
> +
> + par = FIELD_PREP(SYS_PAR_EL1_ATTR, final_attr);
> + par |= tr->output & GENMASK(47, 12);
> + par |= FIELD_PREP(SYS_PAR_EL1_SH,
> + compute_sh(final_attr, tr->desc));
> +
> + return par;
>
It seems that the code doesn't combine shareability attributes, as per rule
RGDTNP and S2CombineS1MemAttrs() or S2ApplyFWBMemAttrs(), which both end up
calling S2CombineS1Shareability().
Thanks,
Alex
