[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 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.

Aside the erratum, a processor is allowed to do whatever it wants if it is within the Arm Arm. These registers are described as out-of-context and should not be used by speculation in EL2. If you want to use them in EL2, you need an isb() before any instruction in EL2 using them otherwise you may use an inconsistent context. This is giving enough freedom to the processor while the impact in the software is minimal.

[...]


           /* 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.



Even if speculation happens without HCR_EL2, why do we need to set it
now? Isn't setting empty_root_mfn enough?

The main goal here is to have the TLBs in a known state after the CPU has been initialized. After the sequence below, we are sure that the TLBs don't contain entries associated to the EL1/EL0 regime and and a speculated AT instruction
will not be able to allocate more.

Without HCR_EL2.VM set, the stage-2 page-table will not get used. So a
speculated AT instruction could still allocate an entry in TLB. It is not a major issue as it would be against INVALID_VMID, yet it is not a very sane
situation for the hypervisor.

I have a question on the tlb flush.  Do we need it because the tlb is
not guaranteed to be clean after boot?

You don't know the state of the TLBs after boot.


Also, do we need a flush_tlb_all_local()? Would flush_tlb_local be
enough, maybe executed immediately before switching VTTBR_EL2? I guess
it depends on whether the speculation happens on the local VMID only.

Speculation can only happen using system registers. So only on the local VMID only.

If it only speculate on the local VMID, then flush_tlb_all_local()
should suffice?

We have two VMIDs in play here: whatever was the value in VTTBR_EL2.VMID before the function and INVALID_VMID. We would need to flush the former and this would require empty root trick because speculation can happen as soon as flush ended.

But then, you rely on Xen to only use a single VMID at boot. While this is the case today, I can't tell if it will be in the future.

So the flush_tlb_local is the safest.

Hmmm, I meant flush_tlb_all_local here.

Cheers,

--
Julien Grall

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

 


Rackspace

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