[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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:

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
happen if the system registers are re-ordered.

The key of the erratum is VTTBR_EL2. This is what will stop a speculated
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
synchronized before the rest of the context, so a speculated AT
may use an inconsistent state and allocate a TLB entry with an
translation against the guest.

So here, we want to ensure the rest of the context is synchronized
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 */
@@ -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
+     * entries related to EL1/EL0 translation regime until a guest
+     * is running. For that, we need to set-up VTTBR to point to an
+     * page-table and turn on stage-2 translation.

I don't understand why this is needed: isn't the lack of HCR_VM (due
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
except it will not take into account the stage-2. The dependencies on
stills applies when HCR_EL2.VM is unset, so from my understanding, the
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!


[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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.