Re: [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events

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

Hello,

On 23/06/16 06:49, Corneliu ZUZU wrote:
> On 6/23/2016 8:31 AM, Corneliu ZUZU wrote:
>> On 6/22/2016 10:41 PM, Julien Grall wrote:
>>>
>>>
>>> On 22/06/2016 20:37, Corneliu ZUZU wrote:
>>>> I've also realized that it's a bit complicated to avoid writing HCR
>>>> from
>>>> 2 places.
>>>> That's because:
>>>> - p2m_restore_state is part of the process of switching to another vCPU
>>>> and the HCR write _must be committed_ here because other components
>>>> depend on that, like address-translation functions
>>>> - I want vm_event_vcpu_enter to be called _after_ the switch to the
>>>> vCPU
>>>> is completed
>>>> - I want HCR_TVM to be set in vm_event_vcpu_enter because setting
>>>> necessary traps _for cr vm-events_ to work should be done there
>>>> (setting
>>>> HCR_TVM bit makes sense to be there and the purpose is to centralize
>>>> operations such as this for code comprehensibility; also, on the X86
>>>> counterpart a similar operation is done for trapping CR3, so it
>>>> would be
>>>> nice to keep the symmetry)
>>>>
>>>> Would it be such a stretch to have HCR written in 2 places? (the second
>>>> time happens rarely anyway: it's unlikely(..) to have to do the
>>>> write in
>>>> vm_event_vcpu_enter)
>>>
>>> Not really. It was mostly to avoid setting/clearing HCR bits in
>>> different place in the code. It makes more difficult to know what is
>>> the final result of the register.
>>>
>>> Anyway, let's skip it for now, if it is too difficult.
>>>
>>> Regards,
>>>
>>
>> Then perhaps something like the following would be suitable:
>>
>> 1. store hcr in arch_domain (register_t hcr)
>>
>> 2. add a function in asm-arm/processor.h (or where else?) which only
>> does:
>>     static inline void update_hcr(struct domain *d)
>>     {
>>         WRITE_SYSREG(d->arch.hcr, HCR_EL2);
>>         isb();
>>     }
>>
>> 3.  modify p2m_restore_state to do:
>>     n->domain->arch.hcr &= ~HCR_VM;
>>     update_hcr(n->domain);
>>     p2m_load_VTTBR(n->domain);
>>
>>     n->domain->arch.hcr |= HCR_VM;
>>
>>     if ( is_32bit_domain(n->domain) )
>>         n->domain->arch.hcr &= ~HCR_RW;
>>     else
>>         n->domain->arch.hcr |= HCR_RW;

This is not safe at all, p2m_restore_state is vCPU specific at you 
modify domain information.

However, if we store the hcr per domain, overriding every context switch 
is pointless as the domain will always be 32-bit/64-bit.

>>
>>     update_hcr(n->domain);
>>
>>     WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
>>     isb();
>>
>> 4. and vcpu_enter_adjust_traps to
>>
>>     if ( unlikely(0 != v->domain->arch.monitor.write_ctrlreg_enabled) )
>>     {
>>          if ( likely(v->domain->arch.hcr & HCR_TVM) )
>>              return;
>>          v->domain->arch.hcr |= HCR_TVM;
>>     }
>>     else
>>     {
>>          if ( likely(!(v->domain->arch.hcr & HCR_TVM)) )
>>              return;
>>          v->domain->arch.hcr &= ~HCR_TVM;
>>     }

This does not need to be done in vcpu_enter_adjust_traps everytime. You 
can set the bit in arch.hcr in DOMCTL_MONITOR_EVENT_WRITE_CTRLREG.

>>
>>     update_hcr(v->domain);
>>
>> That way at least it's easier to follow where update_hcr is called.

I don't see much reason to store the value in the domain and have 
multiple update_hcr. If we store the value, then we should only call 
update_hcr once when returning to the guest.

> And also set the initial value of HCR at the moment of creation, i.e. in
> arch_domain_create as
>
> d->arch.hcr = READ_SYSREG(HCR_EL2)

We control the value of HCR_EL2, so it would be better to assign the 
list of flags here.

Regards,

-- 
Julien Grall

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