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