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

Cheers,

--
Julien Grall

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