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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano

On 03/16/2017 10:33 PM, Stefano Stabellini wrote:
> On Wed, 15 Mar 2017, Julien Grall wrote:
> > Hi Wei,
> >
> > On 15/03/17 08:34, Wei Chen wrote:
> > > On 2017/3/15 8:25, Stefano Stabellini wrote:
> > > > On Mon, 13 Mar 2017, Wei Chen wrote:
> > > > > Different domains may have different HCR_EL2 flags. For example, the
> > > > > 64-bit domain needs HCR_RW flag but the 32-bit does not need it. So
> > > > > we give each domain a default HCR_EL2 value and save it in the VCPU's
> > > > > context.
> > > > >
> > > > > HCR_EL2 register has only one bit can be updated automatically without
> > > > > explicit write (HCR_VSE). But we haven't used this bit currently, so
> > > > > we can consider that the HCR_EL2 register will not be modified while
> > > > > the guest is running. So save the HCR_EL2 while guest exiting to
> > > > > hypervisor is not neccessary. We just have to restore this register for
> > > > > each VCPU while leaving hypervisor.
> > > > >
> > > > > We prefer to restore HCR_EL2 in leave_hypervisor_tail rather than in
> > > > > ctxt_switch_to. Because the leave_hypervisor_tail is the closest place
> > > > > to the exception return. In this case, we don't need to warrant the
> > > > > HCR_EL2 will not be changed between ctxt_switch_to and exception return.
> > > > Please explain a bit better why it is good to restore HCR_EL2 in
> > > > leave_hypervisor_tail. Why is it a good thing that is closer to the
> > > > exception return? What can be the cause of a change in HCR_EL2?
> > > Ok, I will try to improve it in next version. In current Xen code, I
> > > can't see any code would change the HCR_EL2 between ctxt_switch_to and
> > > return_from_trap. But my concern is that, in the future, if someone
> > > want to insert some HCR_EL2 change code between them, we can't guarantee
> > > he will restore correct HCR_EL2 value for current vcpu.
> > Well, the purpose of this series is to inject a virtual SError to the guest
> > when a host SError is happening. The host SError will be received in the
> > hypervisor whilst the vCPU is running and no context switch (e.g call to
> > ctxt_switch) may happen if the scheduler decides to keep the vCPU running.
> >
> > Also, one could argue that HCR_EL2 could be modified on-fly in the function
> > but we may have other places in the future which will modify HCR_EL2. For
> > instance, this would be the case when the monitor app will gain support of
> > inspecting some registers.
> >
> > So we want a single place to restore HCR_EL2. And leave_hypervisor_tail is the
> > best place for that.
> You say that we want a single place to restore HCR_EL2, but this patch
> introduces two places, one is p2m_restore_state, the other is
> leave_hypervisor_tail. Are you saying Wei should remove the HCR_EL2
> restore in p2m_restore_state and just keep the one in
> leave_hypervisor_tail?
p2m_restore_state may be used to switch temporarily to a p2m state. For 
instance for TLB flushing or even doing VA -> PA translation. Though the 
later does not yet work quite well when not using the current vCPU.
Some bits in HCR_EL2 (specifically HCR_EL2.RW) will be used to know how 
to interpret the stage-1 page table as the encoding differ between 
AArch64 and AArch32.
So I think we have to keep the one in p2m_restore_state. But I would 
like to keep the number very limited.
Cheers.

--
Julien Grall

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