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

[Julien Grall <julien.grall@arm.com>]

(CC Mark for the TLB question)

Hi Stefano,

On 21/03/17 00:31, Stefano Stabellini wrote:
> On Thu, 16 Mar 2017, Julien Grall wrote:
>> 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.
>
> I understand, but restoring HCR_EL2 twice is very counter-intuitive. At
> the very least we need a good comment above each of the two
> write(HCR_EL2) calls.
>
> Another option would be to only "fix" HCR_RW in p2m_restore_state, and
> do the full restore in leave_hypervisor_tail. The purpose of doing that
> is separation of responsibilities: it would be clearer what is the
> responsibility of each of the two.

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.

But to be fair, the function p2m_restore_state is working by chance as 
technically more registers (SCTLR, TTBR,...) should be restored for some 
translation. I think it would benefit some rework but this is post Xen 
4.9 task.

Cheers,

-- 
Julien Grall

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