Re: [Xen-devel] [PATCH for-4.12 6/8] xen/arm: Implement workaround for Cortex-A76 erratum 1165522

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 29/01/2019 00:52, Stefano Stabellini wrote:
> On Mon, 28 Jan 2019, Julien Grall wrote:
> > On 1/27/19 9:55 AM, Julien Grall wrote:
> > > Hi,
> > >
> > > On 1/25/19 9:36 PM, Stefano Stabellini wrote:
> > > > On Thu, 24 Jan 2019, Julien Grall wrote:
> > > > > @James, please correct me if I am wrong below :).
> > > > >
> > > > > On 24/01/2019 00:52, Stefano Stabellini wrote:
> > > > > > On Wed, 28 Nov 2018, Julien Grall wrote:
> > > > > ... in the context of the errata, you have to imagine what can happen if
> > > > > an AT
> > > > > instruction is inserted (via speculation) between each instruction and
> > > > > what
> > > > > happen if the system registers are re-ordered.
> > > > >
> > > > > The key of the erratum is VTTBR_EL2. This is what will stop a speculated
> > > > > AT
> > > > > instruction to allocate a TLBs entry because you are not allowed to
> > > > > cache a
> > > > > translation that will fault. Without the isb() here, the VTTBR_EL2 may
> > > > > be
> > > > > synchronized before the rest of the context, so a speculated AT
> > > > > instruction
> > > > > may use an inconsistent state and allocate a TLB entry with an
> > > > > unexpected
> > > > > translation against the guest.
> > > > >
> > > > > So here, we want to ensure the rest of the context is synchronized
> > > > > before
> > > > > writing to VTTBR_EL2, hence the isb().
> > > > OK. I understand the explanation, thank you.
> > > >
> > > > I just thought that the CPU would be smart enough to only reorder system
> > > > registers writes when appropriate, especially when the CPU is also doing
> > > > speculation at the same time. Why would it speculate if it knows that it
> > > > is reordering sysreg writes that can badly affect the speculation
> > > > itself? Let me say that it doesn't sound like a "sane" behavior to me.
> > > > But if it behaves this way, it behaves this way...
> > > I hope you are aware we are speaking about an erratum here... Not what the
> > > Arm Arm allows.
> I know -- we are talking about a specific CPU implementation. That is
> why it seems strange to me that a CPU would reorder things that it
> should know they cause trouble to speculation. Anyway, no point in
> discussing hardware design choices at this stage.
I agree that speculation may not happen as I described in my previous e-mail. 
However, we have to assume that any behavior allowed by the Arm Arm can happen 
unless stated otherwise by the specific processor documentation.
This series is based on the Arm Arm and the erratum description provides in the 
Software Developer Errata Notice for the Cortex-A76 [1], both are available 
publicly.
Regarding re-ordering, the wording in the document does not provide any strong 
evidence the writes to system register cannot be re-ordered. The section 
"conditions" actually suggests the invert (i.e re-ordering is possible).
[...]

> > > > > > >             /* Ensure VTTBR_EL2 is synchronized before flushing the
> > > > > > > TLBs */
> > > > > > >             isb();
> > > > > > >         }
> > > > > > > @@ -1504,6 +1545,23 @@ static uint32_t __read_mostly vtcr;
> > > > > > >     static void setup_virt_paging_one(void *data)
> > > > > > >     {
> > > > > > >         WRITE_SYSREG32(vtcr, VTCR_EL2);
> > > > > > > +
> > > > > > > +    /*
> > > > > > > +     * ARM64_WORKAROUND_AT_SPECULATE: We want to keep the TLBs free
> > > > > > > from
> > > > > > > +     * entries related to EL1/EL0 translation regime until a guest
> > > > > > > vCPU
> > > > > > > +     * is running. For that, we need to set-up VTTBR to point to an
> > > > > > > empty
> > > > > > > +     * page-table and turn on stage-2 translation.
> > > > > > I don't understand why this is needed: isn't the lack of HCR_VM (due
> > > > > > to
> > > > > > your previous patch) supposed to be sufficient? How can there be
> > > > > > speculation without HCR_VM?
> > > > > HCR_EL2.VM unsets means the stage-2 will not be used for the EL1/EL0
> > > > > translation regime. In the context of the erratum, the AT can still
> > > > > speculate
> > > > > except it will not take into account the stage-2. The dependencies on
> > > > > VMID
> > > > > stills applies when HCR_EL2.VM is unset, so from my understanding, the
> > > > > entry
> > > > > could get cached to whatever is VTTBR_EL2.VMID.
> > > > Damn! Even if at this point of the boot sequence there is no EL1 / EL0
> > > > at all? How can that speculation happen? Shouldn't the first EL1 / EL0
> > > > speculation occur after the first leave_hypervisor_tail?
> > > How do you know EL1 was not run before hand? Imagine we did a soft reboot or
> > > kexec Xen...
> > >
> > > But the speculation in that context is may be because the processor noticed
> > > an AT instruction targeting EL1 in the stream.
> This seems to be extremely improbable, borderline impossible to me, but
> I can imagine that we might want to be extra-paranoid to make sure all
> potential issues are covered.
The "Workaround" section of the erratum contains the following wording:

"Note that a workaround is only required if the system software
contains an AT instruction as part of an executable page."

Xen contains AT instruction in an executable page, so speculation can happen and 
we don't know when.
[...]

> Overall, I think we could probably get away without a couple of changes
> from this patch, but since it is basically impossible for me to prove
> it, I'll give my Reviewed-by. I saw that you resent the series already.
> I'll take care of committing it.
>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Thank you for the review!

Cheers,

[1] 
https://silver.arm.com/download/Documentation/BX500-DA-10008-r0p0-02rel0/Arm_Cortex_A76_MP052_Software_Developer_Errata_Notice_v11.0.pdf
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel