[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 6/23/2016 2:11 PM, Julien Grall wrote:
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.

Oh right, the RW bit needs not be set/unset anymore with this change.



    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.

I wanted to keep X86-ARM symmetry and it seemed more intuitive to have these kind of adjustments with the vcpu_enter code motion. But now that I think about it, given the fact that we have the guarantee that after monitor_domctl and before reentering the vCPU p2m_restore_state gets called (due to domain_pause/domain_unpause) - thus actually committing the hcr update at the proper time - technically monitor_domctl _is_ the optimal place to set arch.hcr in. In conclusion, I'm thinking of discarding the entire idea of introducing vm_event_vcpu_enter, it seems to me now that this would also render a simpler code.



    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.

Yep, that will happen with the above-mentioned discarding of vm_event_vcpu_enter idea.


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.

Right, that will happen too.


Regards,


Thanks for the useful insights,
Corneliu.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.