On Thu, 18 Jul 2024 16:10:20 +0100,
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> 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().
Yup, well caught.
> > + 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().
That as well. See below what I'm stashing on top.
Thanks,
M.
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index e66c97fc1fd3..28c4344d1c34 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -459,13 +459,34 @@ static u8 combine_s1_s2_attr(u8 s1, u8 s2)
return final;
}
+#define ATTR_NSH 0b00
+#define ATTR_RSV 0b01
+#define ATTR_OSH 0b10
+#define ATTR_ISH 0b11
+
static u8 compute_sh(u8 attr, u64 desc)
{
+ u8 sh;
+
/* 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 ATTR_OSH;
+
+ sh = FIELD_GET(PTE_SHARED, desc);
+ if (sh == ATTR_RSV) /* Reserved, mapped to NSH */
+ sh = ATTR_NSH;
+
+ return sh;
+}
+
+static u8 combine_sh(u8 s1_sh, u8 s2_sh)
+{
+ if (s1_sh == ATTR_OSH || s2_sh == ATTR_OSH)
+ return ATTR_OSH;
+ if (s1_sh == ATTR_ISH || s2_sh == ATTR_ISH)
+ return ATTR_ISH;
- return FIELD_GET(PTE_SHARED, desc) == 0b11 ? 0b11 : 0b10;
+ return ATTR_NSH;
}
static u64 compute_par_s12(struct kvm_vcpu *vcpu, u64 s1_par,
@@ -540,7 +561,8 @@ static u64 compute_par_s12(struct kvm_vcpu *vcpu, u64 s1_par,
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));
+ combine_sh(FIELD_GET(SYS_PAR_EL1_SH, s1_par),
+ compute_sh(final_attr, tr->desc)));
return par;
}
--
Without deviation from the norm, progress is not possible.
