Re: [PATCH 08/12] KVM: arm64: nv: Add emulation of AT S12E{0,1}{R,W}

[Alexandru Elisei <alexandru.elisei@arm.com>]

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