Re: [Xen-devel] [PATCH 02/18] xen/arm: Restore HCR_EL2 register

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 22/03/17 17:54, Stefano Stabellini wrote:
> On Wed, 22 Mar 2017, Marc Zyngier wrote:
> > On 22/03/17 12:45, Mark Rutland wrote:
> > > On Wed, Mar 22, 2017 at 12:16:20PM +0000, Julien Grall wrote:
> > > > (CC Mark for the TLB question)
> > > [Adding Marc since he should understand this better than I do]
> > >
> > > I've trimmed a lot of context here, since it wasn't clear if it was
> > > relevant to the question. If there's something I've missed, please point
> > > that out explicitly.
> > >
> > > > I am not entirely sure if we can only restore HCR_RW. My concern is
> > > > some of the bits are cached in the TLBs. Looking at the ARM ARM
> > > > (both v7 and v8 see D4.7 in DDI0487A.k_iss10775): "In these
> > > > descriptions, TLB entries for a translation regime for a particular
> > > > Exception level are out of context when executing at a higher
> > > > Exception level.". Which I interpret as TLB result cannot be cached
> > > > when translating a guest VA to guest PA at EL2. So I guess restoring
> > > > only HCR_RW might be fine.
> > > My understanding is that when a translation regime is out-of-context,
> > > the only requirement is that the core does not begin speculative walks
> > > for that translation regime. See ARM DDI 0487A.k_iss10775, page D4-1735,
> > > "Use of out-of-context translation regimes".
> > +1. An AT instruction is certainly allowed to hit in the TLB, and the
> > only thing the core is not allowed to do is to speculate (because you
> > cannot restore a guest context atomically).
> >
> > > I believe that an explicit address translation involving a translation
> > > regime can validly make use of (any part of) that regime's state and/or
> > > allocate new TLB entries for that regime.
> > >
> > > It looks like you're asking if you can avoid installing all of the
> > > registers related to the EL1&0 regime when performing an EL1&0
> > > translation at EL2, right?
> > >
> > > I do not believe that is valid; my understanding is that all of the
> > > relevant registers need to be installed first.
> > Indeed. I can't see how the you can perform an AT without first
> > restoring all of the context that define how the page tables are parsed
> > and including all the element that may be used to hit in the TLBs. Even
> > worse, you could create TLB entries (as you said, AT doesn't preclude
> > TLBs from being populated) that would conflict with an existing one on
> > the next lookup, generating a Conflict Abort.
> >
> > In short, not restoring the full VM context is doomed.
> >
> > Now, what is the actual question? ;-)
> Hi Marc, thanks for jumping in.
>
> When we receive an SError in Xen, we determine if it should be injected
> into the guest or "handled" in Xen (by "handle" I mean crash the
> system). In case it should be injected into the guest, we set the
> relevant bit in vcpu->arch.hcr_el2 (the saved version of HCR_EL2). This
> patch moves the WRITE(vcpu->arch.hcr_el2, HCR_EL2) from context switch
> related functions (p2m_restore_state) to leave_hypervisor_tail, which is
> the last point we can move it to, before exiting Xen. That way, we are
> sure to inject an abort into the guest, no matter exactly when we
> receive the SError. So far so good, right?
>
> Now that leaves the question: what are we going to do at context switch
> time? From the SErrors and HCR_VA point of view, there is no need to
> restore the vcpu->arch.hcr_el2 of the next vcpu immediately, we can
> still do it later in leave_hypervisor_tail. But we do need the right
> HCR_RW immediately, because otherwise AT could behave erratically after
> the context switch and before leave_hypervisor_tail.
Well, this is slightly untrue. The function p2m_restore_state is used 
for 2 purposes:
        - Context switch vCPU
	- Temporarily switching to a P2M to perform AT instruction on behalf of 
a vCPU.
For the former, AT instruction should never happen because as soon as we 
context switch we will return to the guest. So, where it matter is when 
p2m_restore_state is used to perform some AT translation (such as when 
DOM0 is booting or as it should be done in memaccess).
> Given that restoring vcpu->arch.hcr_el2 twice (once in
> leave_hypervisor_tail and once at context switch) looks weird, I
> suggested to only restore the HCR_RW bit at context switch time and do a
> full restore of vcpu->arch.hcr_el2 in leave_hypervisor_tail: the benefit
> would be a clear separation of responsibility of the two operations.
> Hence the question above. But it looks like it's not safe?
From the discussion I had with Mar{c,k}, we should restore all the 
registers that could be cached in TLB when performing an AT instruction. 
Otherwise the TLB may generate conflict abort.
But as I said previously, the function p2m_restore_state is a mess. It 
should be reworked to differentiate the case where we context switch the 
vCPU and the one we perform an AT instruction. This would need some 
rework that I think is too late for Xen 4.9.
So for the time being, I would keep both and add a comment explaining why.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel