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

2016-06-24 Thread Corneliu ZUZU 

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@lists.xen.org
http://lists.xen.org/xen-devel

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

2016-06-23 Thread Julien Grall 

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

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

2016-06-22 Thread Corneliu ZUZU 

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;

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;
}

update_hcr(v->domain);

That way at least it's easier to follow where update_hcr is called.

Corneliu.


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)

Corneliu.

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

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

2016-06-22 Thread Corneliu ZUZU 

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;

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;
}

update_hcr(v->domain);

That way at least it's easier to follow where update_hcr is called.

Corneliu.

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

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

2016-06-22 Thread Julien Grall 



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,

--
Julien Grall

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

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

2016-06-22 Thread Corneliu ZUZU 

On 6/22/2016 9:39 PM, Corneliu ZUZU wrote:

On 6/22/2016 8:17 PM, Julien Grall wrote:

On 22/06/16 17:35, Corneliu ZUZU wrote:


Julien,


Hello Corneliu,


I was trying to implement having HCR stored in arch_domain or arch_vcpu
as suggested above and I'm a bit confused about the code in
p2m_restore_state.
I'm hoping you can provide some feedback on this matter. Here's the
current implementation of the function:

 void p2m_restore_state(struct vcpu *n)
 {
 register_t hcr;

 hcr = READ_SYSREG(HCR_EL2);
 WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
 isb();

 p2m_load_VTTBR(n->domain);
 isb();

 if ( is_32bit_domain(n->domain) )
 hcr &= ~HCR_RW;
 else
 hcr |= HCR_RW;

 WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
 isb();

 WRITE_SYSREG(hcr, HCR_EL2);
 isb();
 }

First of all, I see the HCR_VM bit being unset (=0) but I don't quite
understand why and even more peculiar is the fact that I couldn't find
any place where the bit is set (=1) again.


After the first write to HCR_EL2, "hcr" still have the VM bit set as 
we only mask it. So the second write will re-set the VM bit.




Ooh..right. Don't know how I missed that, I guess I was too focused in 
finding a -different- place where HCR was modified.


I am not sure why the VM bit is unset/set in this function. I am not 
able to find a paragraph justifying it in the ARM ARM. I have CCed 
some ARM folks to check if I missed something.




An answer to that would be useful. I'm also curious if there's a 
reason why HCR_RW is set/unset afterwards and not before and why 
there's an isb() after calling p2m_load_VTTBR if that function already 
has an isb() @ its end.



Secondly, why this order of operations? More specifically, why is
p2m_load_VTTBR done after the HCR_VM bit is unset and before the HCR_RW
bit is set/unset? Can't we write HCR only once here?
And finally, I see the function is called by construct_dom0. The code
there looks like:

 /*
  * The following loads use the domain's p2m and require current to
  * be a vcpu of the domain, temporarily switch
  */
 saved_current = current;
 p2m_restore_state(v);
 [...]
 /* Now that we are done restore the original p2m and current. */
 set_current(saved_current);
 p2m_restore_state(saved_current);

I suppose the significant changes p2m_restore_state does for the 
code in
between ("[...]") is setting VTTBR & SCTLR which are used by 
translation

functions such as gvirt_to_maddr (which seems to use PAR_EL1).
What I don't grasp is what effect setting the VTTBR has if 
HCR.HCR_VM is

unset and left unset...


HCR.VM is not left unset (see why above).

Regards,



Thanks,
Corneliu.


Julien,

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)


Regards,
Corneliu.

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

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

2016-06-22 Thread Corneliu ZUZU 

On 6/22/2016 8:17 PM, Julien Grall wrote:

On 22/06/16 17:35, Corneliu ZUZU wrote:


Julien,


Hello Corneliu,


I was trying to implement having HCR stored in arch_domain or arch_vcpu
as suggested above and I'm a bit confused about the code in
p2m_restore_state.
I'm hoping you can provide some feedback on this matter. Here's the
current implementation of the function:

 void p2m_restore_state(struct vcpu *n)
 {
 register_t hcr;

 hcr = READ_SYSREG(HCR_EL2);
 WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
 isb();

 p2m_load_VTTBR(n->domain);
 isb();

 if ( is_32bit_domain(n->domain) )
 hcr &= ~HCR_RW;
 else
 hcr |= HCR_RW;

 WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
 isb();

 WRITE_SYSREG(hcr, HCR_EL2);
 isb();
 }

First of all, I see the HCR_VM bit being unset (=0) but I don't quite
understand why and even more peculiar is the fact that I couldn't find
any place where the bit is set (=1) again.


After the first write to HCR_EL2, "hcr" still have the VM bit set as 
we only mask it. So the second write will re-set the VM bit.




Ooh..right. Don't know how I missed that, I guess I was too focused in 
finding a -different- place where HCR was modified.


I am not sure why the VM bit is unset/set in this function. I am not 
able to find a paragraph justifying it in the ARM ARM. I have CCed 
some ARM folks to check if I missed something.




An answer to that would be useful. I'm also curious if there's a reason 
why HCR_RW is set/unset afterwards and not before and why there's an 
isb() after calling p2m_load_VTTBR if that function already has an isb() 
@ its end.



Secondly, why this order of operations? More specifically, why is
p2m_load_VTTBR done after the HCR_VM bit is unset and before the HCR_RW
bit is set/unset? Can't we write HCR only once here?
And finally, I see the function is called by construct_dom0. The code
there looks like:

 /*
  * The following loads use the domain's p2m and require current to
  * be a vcpu of the domain, temporarily switch
  */
 saved_current = current;
 p2m_restore_state(v);
 [...]
 /* Now that we are done restore the original p2m and current. */
 set_current(saved_current);
 p2m_restore_state(saved_current);

I suppose the significant changes p2m_restore_state does for the code in
between ("[...]") is setting VTTBR & SCTLR which are used by translation
functions such as gvirt_to_maddr (which seems to use PAR_EL1).
What I don't grasp is what effect setting the VTTBR has if HCR.HCR_VM is
unset and left unset...


HCR.VM is not left unset (see why above).

Regards,



Thanks,
Corneliu.

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

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

2016-06-22 Thread Julien Grall 

On 22/06/16 17:35, Corneliu ZUZU wrote:


Julien,


Hello Corneliu,


I was trying to implement having HCR stored in arch_domain or arch_vcpu
as suggested above and I'm a bit confused about the code in
p2m_restore_state.
I'm hoping you can provide some feedback on this matter. Here's the
current implementation of the function:

 void p2m_restore_state(struct vcpu *n)
 {
 register_t hcr;

 hcr = READ_SYSREG(HCR_EL2);
 WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
 isb();

 p2m_load_VTTBR(n->domain);
 isb();

 if ( is_32bit_domain(n->domain) )
 hcr &= ~HCR_RW;
 else
 hcr |= HCR_RW;

 WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
 isb();

 WRITE_SYSREG(hcr, HCR_EL2);
 isb();
 }

First of all, I see the HCR_VM bit being unset (=0) but I don't quite
understand why and even more peculiar is the fact that I couldn't find
any place where the bit is set (=1) again.


After the first write to HCR_EL2, "hcr" still have the VM bit set as we 
only mask it. So the second write will re-set the VM bit.


I am not sure why the VM bit is unset/set in this function. I am not 
able to find a paragraph justifying it in the ARM ARM. I have CCed some 
ARM folks to check if I missed something.



Secondly, why this order of operations? More specifically, why is
p2m_load_VTTBR done after the HCR_VM bit is unset and before the HCR_RW
bit is set/unset? Can't we write HCR only once here?
And finally, I see the function is called by construct_dom0. The code
there looks like:

 /*
  * The following loads use the domain's p2m and require current to
  * be a vcpu of the domain, temporarily switch
  */
 saved_current = current;
 p2m_restore_state(v);
 [...]
 /* Now that we are done restore the original p2m and current. */
 set_current(saved_current);
 p2m_restore_state(saved_current);

I suppose the significant changes p2m_restore_state does for the code in
between ("[...]") is setting VTTBR & SCTLR which are used by translation
functions such as gvirt_to_maddr (which seems to use PAR_EL1).
What I don't grasp is what effect setting the VTTBR has if HCR.HCR_VM is
unset and left unset...


HCR.VM is not left unset (see why above).

Regards,

--
Julien Grall

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

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

2016-06-22 Thread Corneliu ZUZU 

On 6/17/2016 1:36 PM, Corneliu ZUZU wrote:

On 6/16/2016 7:49 PM, Julien Grall wrote:

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:


+case MWS_TTBCR:
+MWS_EMUL(TTBCR);
+break;
+default:
+break;
+}
+
+w->status = MWS_NOWRITE;
+}
+
+static inline void vcpu_enter_adjust_traps(struct vcpu *v)
+{
+register_t old_hcr, hcr;
+
+hcr = (old_hcr = READ_SYSREG(HCR_EL2));
+
+if ( unlikely(0 != 
v->domain->arch.monitor.write_ctrlreg_enabled) )

+hcr |= HCR_TVM;
+else
+hcr &= ~HCR_TVM;
+
+if ( unlikely(hcr != old_hcr) )
+{
+WRITE_SYSREG(hcr, HCR_EL2);
+isb();
+}


The HCR register is also updated in p2m_restore_state to set 
HCR_EL2.RW. I would prefer to have a single place where HCR is updated.


Maybe we should store the HCR value per-domain?


Yes, that would be nice. Will look into that for v2.


Julien,

I was trying to implement having HCR stored in arch_domain or arch_vcpu 
as suggested above and I'm a bit confused about the code in 
p2m_restore_state.
I'm hoping you can provide some feedback on this matter. Here's the 
current implementation of the function:


void p2m_restore_state(struct vcpu *n)
{
register_t hcr;

hcr = READ_SYSREG(HCR_EL2);
WRITE_SYSREG(hcr & ~HCR_VM, HCR_EL2);
isb();

p2m_load_VTTBR(n->domain);
isb();

if ( is_32bit_domain(n->domain) )
hcr &= ~HCR_RW;
else
hcr |= HCR_RW;

WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
isb();

WRITE_SYSREG(hcr, HCR_EL2);
isb();
}

First of all, I see the HCR_VM bit being unset (=0) but I don't quite 
understand why and even more peculiar is the fact that I couldn't find 
any place where the bit is set (=1) again.
Secondly, why this order of operations? More specifically, why is 
p2m_load_VTTBR done after the HCR_VM bit is unset and before the HCR_RW 
bit is set/unset? Can't we write HCR only once here?
And finally, I see the function is called by construct_dom0. The code 
there looks like:


/*
 * The following loads use the domain's p2m and require current to
 * be a vcpu of the domain, temporarily switch
 */
saved_current = current;
p2m_restore_state(v);
[...]
/* Now that we are done restore the original p2m and current. */
set_current(saved_current);
p2m_restore_state(saved_current);

I suppose the significant changes p2m_restore_state does for the code in 
between ("[...]") is setting VTTBR & SCTLR which are used by translation 
functions such as gvirt_to_maddr (which seems to use PAR_EL1).
What I don't grasp is what effect setting the VTTBR has if HCR.HCR_VM is 
unset and left unset...


Thanks,
Corneliu.

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

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

2016-06-17 Thread Julien Grall 

Hello Corneliu,

On 17/06/16 11:36, Corneliu ZUZU wrote:

On 6/16/2016 7:49 PM, Julien Grall wrote:

On 16/06/16 15:13, Corneliu ZUZU wrote:


[...]


Please mention that PRRR and NMRR are aliased to respectively MAIR0
and MAIR1. This will avoid to spend time trying to understanding why
the spec says they are trapped but you don't "handle" them.


I mentioned that in traps.h (see "AKA" comments). Will put the same
comment here then.


I noticed it later. But it was not obvious to find.

[...]


diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 000..3f23fec
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,112 @@


[...]


+#include 
+#include 
+
+#if CONFIG_ARM_64
+
+#define MWSINF_SCTLR32,SCTLR_EL1
+#define MWSINF_TTBR064,TTBR0_EL1
+#define MWSINF_TTBR164,TTBR1_EL1
+#define MWSINF_TTBCR64,TCR_EL1
+
+#elif CONFIG_ARM_32
+
+#define MWSINF_SCTLR32,SCTLR
+#define MWSINF_TTBR064,TTBR0
+#define MWSINF_TTBR164,TTBR1


The values are the same as for arm64 (*_EL1 is aliased to * for
arm32). Please avoid duplication.


(see comment below about later reply)


Your later reply explain why you did not expose TTBR*_32 to ARM64, but 
does not explain why the 3 define above is the same as the ARM64.







+#define MWSINF_TTBR0_32 32,TTBR0_32
+#define MWSINF_TTBR1_32 32,TTBR1_32
+#define MWSINF_TTBCR32,TTBCR
+
+#endif
+
+#define MWS_EMUL_(val, sz, r...) WRITE_SYSREG##sz((uint##sz##_t)
(val), r)


The cast is not necessary.


+#define MWS_EMUL(r) CALL_MACRO(MWS_EMUL_, w->value, MWSINF_##r)
+
+static inline void vcpu_enter_write_data(struct vcpu *v)
+{
+struct monitor_write_data *w = >arch.vm_event.write_data;
+
+if ( likely(MWS_NOWRITE == w->status) )
+return;
+
+switch ( w->status )
+{
+case MWS_SCTLR:
+MWS_EMUL(SCTLR);
+break;
+case MWS_TTBR0:
+MWS_EMUL(TTBR0);
+break;
+case MWS_TTBR1:
+MWS_EMUL(TTBR1);
+break;
+#if CONFIG_ARM_32
+case MWS_TTBR0_32:
+MWS_EMUL(TTBR0_32);
+break;
+case MWS_TTBR1_32:
+MWS_EMUL(TTBR1_32);
+break;
+#endif


Aarch32 kernel can return on an AArch64 Xen. This means that
TTBR{0,1}_32 could be trapped and the write therefore be properly
emulated.


MCR TTBRx writes from AArch32 guests appear as writes of the low 32-bits
of AArch64 TTBRx_EL1 (architecturally mapped).
MCRR TTBRx writes from AArch32 guests appear as writes to AArch64
TTBRx_EL1.
Therefore, in the end the register we need to write is still TTBRx_EL1.


Why would you want to notify write to TTBR*_32 when Xen is running in 
AArch32 mode and none in Aaarch64 mode?


The vm-events for an AArch32 guests should be exactly the same
regardless of Xen mode (i.e AArch32 vs AArch64).

[...]




@@ -0,0 +1,253 @@


[...]


+/*
+ * Emulation of system-register trapped writes that do not cause
+ * VM_EVENT_REASON_WRITE_CTRLREG monitor vm-events.
+ * Such writes are collaterally trapped due to setting the
HCR_EL2.TVM bit.
+ *
+ * Regarding aarch32 domains, note that from Xen's perspective
system-registers
+ * of such domains are architecturally-mapped to aarch64 registers
in one of
+ * three ways:
+ *  - low 32-bits mapping   (e.g. aarch32 DFAR -> aarch64
FAR_EL1[31:0])
+ *  - high 32-bits mapping  (e.g. aarch32 IFAR -> aarch64
FAR_EL1[63:32])
+ *  - full mapping  (e.g. aarch32 SCTLR -> aarch64 SCTLR_EL1)
+ *
+ * Hence we define 2 macro variants:
+ *  - TVM_EMUL_SZ variant, for full mappings
+ *  - TVM_EMUL_LH variant, for low/high 32-bits mappings
+ */
+#define TVM_EMUL_SZ(regs, hsr, val, sz,
r...)   \
+{ \
+if ( psr_mode_is_user(regs)
)   \


Those registers are not accessible at EL0.


Hmm, I have this question noted.
I remember finding from the manuals that a write from user-mode of those
regs would still trap to EL2, but I didn't test that yet.
Will put that to test and come back with results for v2.


Testing will not tell you if a trap will occur or not. The ARM ARM may 
define it as IMPLEMENTATION DEFINED.


From my understanding of the ARMv7 spec (B1.14.1 and B1.14.13 in ARM 
DDI 0406C.c), the instruction at PL0 (user mode) will not trap to the 
hypervisor:


"Setting HCR.TVM to 1 means that any attempt, to write to a Non-secure 
memory control register from a Non-secure
PL1 or PL0 mode, that this reference manual does not describe as 
UNDEFINED , generates a Hyp Trap exception."


For ARMv8 (See description of HCR_EL2.TVM D7-1971 in ARM DDI 0487A.j), 
only NS EL1 write to the registers will be trapped.





+return inject_undef_exception(regs,
hsr);   \
+WRITE_SYSREG##sz((uint##sz##_t) (val), r);


[...]


diff --git a/xen/include/asm-arm/vm_event.h
b/xen/include/asm-arm/vm_event.h
index 4e5a272..edf9654 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -30,6 +30,12 @@ static inline int

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

2016-06-17 Thread Corneliu ZUZU 

On 6/17/2016 12:28 AM, Julien Grall wrote:



On 16/06/2016 20:24, Corneliu ZUZU wrote:

On 6/16/2016 5:26 PM, Julien Grall wrote:
Hi Julien. Yes, I agree that it's complex, I would have preferred to
split it up too and I actually tried, but the changes are tightly
coupled and they don't seem to be 'split-able'.


After I reviewed this patch I think it could be simplified a lot.

Most of the registers not trapped by vm-event could be emulated with 
one-line WRITE_SYSREGxx and does not require specific case depending 
on the architecture.


You can give a look how we handle the domain context switch in C 
(arch/arm/domain.c).


Only few of the registers will require more than one-line (such as 
MAIR*, *FAR) which could be over a macro.


Regards,



Ok, I'll take a look again but I'm not that optimistic, I remember 
trying pretty hard to simplify this (and kind of failing), I got the 
best I could.
I think it had to do with the fact the SCTLR macros & such got expanded 
too early (where I wanted to use them as prefixes to construct other 
macros).
Having said this, I'll look into your suggestions and reinspect the code 
to recall why those macros got to be the way the are and get back with 
conclusions.


Regards,
Corneliu.

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

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

2016-06-17 Thread Corneliu ZUZU 

On 6/16/2016 7:49 PM, Julien Grall wrote:

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8c50685..af61ac3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 

  #include "decode.h"
  #include "vtimer.h"
@@ -124,7 +125,12 @@ void init_traps(void)
  WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) | 
HCPTR_TTA,

   CPTR_EL2);

-/* Setup hypervisor traps */
+/* Setup hypervisor traps
+ *
+ * Note: HCR_TVM bit is also set for system-register write 
monitoring
+ * purposes (see vm_event_monitor_cr), but (for performance 
reasons) that's

+ * done selectively (see vcpu_enter_adjust_traps).
+ */
WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
   HCR_EL2);
@@ -1720,6 +1726,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
  const struct hsr_cp32 cp32 = hsr.cp32;
  int regidx = cp32.reg;
  struct vcpu *v = current;
+register_t r = get_user_reg(regs, regidx);

  if ( !check_conditional_instr(regs, hsr) )
  {
@@ -1730,6 +1737,61 @@ static void do_cp15_32(struct cpu_user_regs 
*regs,

  switch ( hsr.bits & HSR_CP32_REGS_MASK )
  {
  /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13


Please try to mention the most recent spec. This was published in 
2012, the latest release (C.c) was published in 2014.


Ack.




+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34


Ditto. This was published in 2014, whilst the most recent was 
published in 2016.



+ */
+case HSR_CPREG32(SCTLR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, SCTLR);
+break;
+case HSR_CPREG32(TTBR0_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR0_32);
+break;
+case HSR_CPREG32(TTBR1_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR1_32);
+break;
+case HSR_CPREG32(TTBCR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBCR);
+break;
+case HSR_CPREG32(DACR):
+TVM_EMUL(regs, hsr, r, DACR);
+break;
+case HSR_CPREG32(DFSR):
+TVM_EMUL(regs, hsr, r, DFSR);
+break;
+case HSR_CPREG32(IFSR):
+TVM_EMUL(regs, hsr, r, IFSR);
+break;
+case HSR_CPREG32(DFAR):
+TVM_EMUL(regs, hsr, r, DFAR);
+break;
+case HSR_CPREG32(IFAR):
+TVM_EMUL(regs, hsr, r, IFAR);
+break;

[...]

+case HSR_CPREG32(ADFSR):
+TVM_EMUL(regs, hsr, r, ADFSR);
+break;
+case HSR_CPREG32(AIFSR):
+TVM_EMUL(regs, hsr, r, AIFSR);
+break;
+case HSR_CPREG32(MAIR0):
+TVM_EMUL(regs, hsr, r, MAIR0);
+break;
+case HSR_CPREG32(MAIR1):
+TVM_EMUL(regs, hsr, r, MAIR1);
+break;


Please mention that PRRR and NMRR are aliased to respectively MAIR0 
and MAIR1. This will avoid to spend time trying to understanding why 
the spec says they are trapped but you don't "handle" them.


I mentioned that in traps.h (see "AKA" comments). Will put the same 
comment here then.





+case HSR_CPREG32(AMAIR0):
+TVM_EMUL(regs, hsr, r, AMAIR0);
+break;
+case HSR_CPREG32(AMAIR1):
+TVM_EMUL(regs, hsr, r, AMAIR1);
+break;
+case HSR_CPREG32(CONTEXTIDR):
+TVM_EMUL(regs, hsr, r, CONTEXTIDR);
+break;
+
+/*
   * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
   *
   * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1853,6 +1915,13 @@ static void do_cp15_32(struct cpu_user_regs 
*regs,

  static void do_cp15_64(struct cpu_user_regs *regs,
 const union hsr hsr)
  {
+struct vcpu *v = current;
+const struct hsr_cp64 cp64 = hsr.cp64;
+sysreg64_t r = {
+.low32 = (uint32_t) get_user_reg(regs, cp64.reg1),


The cast is not necessary.


Ack.




+.high32 = (uint32_t) get_user_reg(regs, cp64.reg2)


Ditto.


+};
+
  if ( !check_conditional_instr(regs, hsr) )
  {
  advance_pc(regs, hsr);
@@ -1862,6 +1931,19 @@ static void do_cp15_64(struct cpu_user_regs 
*regs,

  switch ( hsr.bits & HSR_CP64_REGS_MASK )
  {
  /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13
+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34


Same remarks as above for the spec version.


+ */
+case HSR_CPREG64(TTBR0):
+TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR0_64);
+break;
+case HSR_CPREG64(TTBR1):
+TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR1_64);
+break;
+
+/*
   * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
   *
   * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1891,8 +1973,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
   */
  default:
  {
-const struct hsr_cp64 cp64 = hsr.cp64;
-
  gdprintk(XENLOG_ERR,
   "%s p15, %d,

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

2016-06-16 Thread Julien Grall 



On 16/06/2016 20:24, Corneliu ZUZU wrote:

On 6/16/2016 5:26 PM, Julien Grall wrote:
Hi Julien. Yes, I agree that it's complex, I would have preferred to
split it up too and I actually tried, but the changes are tightly
coupled and they don't seem to be 'split-able'.


After I reviewed this patch I think it could be simplified a lot.

Most of the registers not trapped by vm-event could be emulated with 
one-line WRITE_SYSREGxx and does not require specific case depending on 
the architecture.


You can give a look how we handle the domain context switch in C 
(arch/arm/domain.c).


Only few of the registers will require more than one-line (such as 
MAIR*, *FAR) which could be over a macro.


Regards,

--
Julien Grall

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

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

2016-06-16 Thread Corneliu ZUZU 

On 6/16/2016 5:26 PM, Julien Grall wrote:

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:
Add ARM support for control-register write monitoring through the 
vm-events

subsystem.

Chosen ARM system control-registers that can be monitored are:
 - VM_EVENT_ARM_SCTLR:  AArch32 SCTLR, AArch64 SCTLR_EL1
 - VM_EVENT_ARM_TTBR{0,1}:  AArch32 TTBR{0,1}, AArch64 TTBR{0,1}_EL1
 - VM_EVENT_ARM_TTBCR:  AArch32 TTBCR, AArch64 TCR_EL1

Trapping of write operations of these registers was attained by 
setting the

HCR_EL2.TVM / HCR.TVM bit.

Signed-off-by: Corneliu ZUZU 
---
  MAINTAINERS|   1 +
  xen/arch/arm/Makefile  |   1 +
  xen/arch/arm/traps.c   | 126 +++-
  xen/arch/arm/vm_event.c| 112 ++
  xen/common/monitor.c   |   2 -
  xen/common/vm_event.c  |   2 -
  xen/include/asm-arm/domain.h   |  30 +
  xen/include/asm-arm/traps.h| 253 
+

  xen/include/asm-arm/vm_event.h |  22 +++-
  xen/include/public/vm_event.h  |   8 +-
  xen/include/xen/monitor.h  |   2 -
  xen/include/xen/vm_event.h |   2 -
  12 files changed, 543 insertions(+), 18 deletions(-)


I think this patch would benefit to be split in multiple patches to 
ease the review and also describe the infrastructure you have 
introduced (TVM_* & co).


I will review in detail later.

Regards,



Hi Julien. Yes, I agree that it's complex, I would have preferred to 
split it up too and I actually tried, but the changes are tightly 
coupled and they don't seem to be 'split-able'.


Corneliu.

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

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

2016-06-16 Thread Julien Grall 

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8c50685..af61ac3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 

  #include "decode.h"
  #include "vtimer.h"
@@ -124,7 +125,12 @@ void init_traps(void)
  WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) | HCPTR_TTA,
   CPTR_EL2);

-/* Setup hypervisor traps */
+/* Setup hypervisor traps
+ *
+ * Note: HCR_TVM bit is also set for system-register write monitoring
+ * purposes (see vm_event_monitor_cr), but (for performance reasons) that's
+ * done selectively (see vcpu_enter_adjust_traps).
+ */
  WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
   HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
   HCR_EL2);
@@ -1720,6 +1726,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
  const struct hsr_cp32 cp32 = hsr.cp32;
  int regidx = cp32.reg;
  struct vcpu *v = current;
+register_t r = get_user_reg(regs, regidx);

  if ( !check_conditional_instr(regs, hsr) )
  {
@@ -1730,6 +1737,61 @@ static void do_cp15_32(struct cpu_user_regs *regs,
  switch ( hsr.bits & HSR_CP32_REGS_MASK )
  {
  /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13


Please try to mention the most recent spec. This was published in 2012, 
the latest release (C.c) was published in 2014.



+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34


Ditto. This was published in 2014, whilst the most recent was published 
in 2016.



+ */
+case HSR_CPREG32(SCTLR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, SCTLR);
+break;
+case HSR_CPREG32(TTBR0_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR0_32);
+break;
+case HSR_CPREG32(TTBR1_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR1_32);
+break;
+case HSR_CPREG32(TTBCR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBCR);
+break;
+case HSR_CPREG32(DACR):
+TVM_EMUL(regs, hsr, r, DACR);
+break;
+case HSR_CPREG32(DFSR):
+TVM_EMUL(regs, hsr, r, DFSR);
+break;
+case HSR_CPREG32(IFSR):
+TVM_EMUL(regs, hsr, r, IFSR);
+break;
+case HSR_CPREG32(DFAR):
+TVM_EMUL(regs, hsr, r, DFAR);
+break;
+case HSR_CPREG32(IFAR):
+TVM_EMUL(regs, hsr, r, IFAR);
+break;

[...]

+case HSR_CPREG32(ADFSR):
+TVM_EMUL(regs, hsr, r, ADFSR);
+break;
+case HSR_CPREG32(AIFSR):
+TVM_EMUL(regs, hsr, r, AIFSR);
+break;
+case HSR_CPREG32(MAIR0):
+TVM_EMUL(regs, hsr, r, MAIR0);
+break;
+case HSR_CPREG32(MAIR1):
+TVM_EMUL(regs, hsr, r, MAIR1);
+break;


Please mention that PRRR and NMRR are aliased to respectively MAIR0 and 
MAIR1. This will avoid to spend time trying to understanding why the 
spec says they are trapped but you don't "handle" them.



+case HSR_CPREG32(AMAIR0):
+TVM_EMUL(regs, hsr, r, AMAIR0);
+break;
+case HSR_CPREG32(AMAIR1):
+TVM_EMUL(regs, hsr, r, AMAIR1);
+break;
+case HSR_CPREG32(CONTEXTIDR):
+TVM_EMUL(regs, hsr, r, CONTEXTIDR);
+break;
+
+/*
   * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
   *
   * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1853,6 +1915,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,
  static void do_cp15_64(struct cpu_user_regs *regs,
 const union hsr hsr)
  {
+struct vcpu *v = current;
+const struct hsr_cp64 cp64 = hsr.cp64;
+sysreg64_t r = {
+.low32 = (uint32_t) get_user_reg(regs, cp64.reg1),


The cast is not necessary.


+.high32 = (uint32_t) get_user_reg(regs, cp64.reg2)


Ditto.


+};
+
  if ( !check_conditional_instr(regs, hsr) )
  {
  advance_pc(regs, hsr);
@@ -1862,6 +1931,19 @@ static void do_cp15_64(struct cpu_user_regs *regs,
  switch ( hsr.bits & HSR_CP64_REGS_MASK )
  {
  /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13
+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34


Same remarks as above for the spec version.


+ */
+case HSR_CPREG64(TTBR0):
+TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR0_64);
+break;
+case HSR_CPREG64(TTBR1):
+TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR1_64);
+break;
+
+/*
   * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
   *
   * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1891,8 +1973,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
   */
  default:
  {
-const struct hsr_cp64 cp64 = hsr.cp64;
-
  gdprintk(XENLOG_ERR,
   "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
   cp64.read ? "mrrc" : "mcrr",
@@ -2128,10 +2208,50 @@ static void

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

2016-06-16 Thread Julien Grall 

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:

Add ARM support for control-register write monitoring through the vm-events
subsystem.

Chosen ARM system control-registers that can be monitored are:
 - VM_EVENT_ARM_SCTLR:  AArch32 SCTLR, AArch64 SCTLR_EL1
 - VM_EVENT_ARM_TTBR{0,1}:  AArch32 TTBR{0,1}, AArch64 TTBR{0,1}_EL1
 - VM_EVENT_ARM_TTBCR:  AArch32 TTBCR, AArch64 TCR_EL1

Trapping of write operations of these registers was attained by setting the
HCR_EL2.TVM / HCR.TVM bit.

Signed-off-by: Corneliu ZUZU 
---
  MAINTAINERS|   1 +
  xen/arch/arm/Makefile  |   1 +
  xen/arch/arm/traps.c   | 126 +++-
  xen/arch/arm/vm_event.c| 112 ++
  xen/common/monitor.c   |   2 -
  xen/common/vm_event.c  |   2 -
  xen/include/asm-arm/domain.h   |  30 +
  xen/include/asm-arm/traps.h| 253 +
  xen/include/asm-arm/vm_event.h |  22 +++-
  xen/include/public/vm_event.h  |   8 +-
  xen/include/xen/monitor.h  |   2 -
  xen/include/xen/vm_event.h |   2 -
  12 files changed, 543 insertions(+), 18 deletions(-)


I think this patch would benefit to be split in multiple patches to ease 
the review and also describe the infrastructure you have introduced 
(TVM_* & co).


I will review in detail later.

Regards,

--
Julien Grall

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

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

2016-06-16 Thread Corneliu ZUZU 
Add ARM support for control-register write monitoring through the vm-events
subsystem.

Chosen ARM system control-registers that can be monitored are:
- VM_EVENT_ARM_SCTLR:  AArch32 SCTLR, AArch64 SCTLR_EL1
- VM_EVENT_ARM_TTBR{0,1}:  AArch32 TTBR{0,1}, AArch64 TTBR{0,1}_EL1
- VM_EVENT_ARM_TTBCR:  AArch32 TTBCR, AArch64 TCR_EL1

Trapping of write operations of these registers was attained by setting the
HCR_EL2.TVM / HCR.TVM bit.

Signed-off-by: Corneliu ZUZU 
---
 MAINTAINERS|   1 +
 xen/arch/arm/Makefile  |   1 +
 xen/arch/arm/traps.c   | 126 +++-
 xen/arch/arm/vm_event.c| 112 ++
 xen/common/monitor.c   |   2 -
 xen/common/vm_event.c  |   2 -
 xen/include/asm-arm/domain.h   |  30 +
 xen/include/asm-arm/traps.h| 253 +
 xen/include/asm-arm/vm_event.h |  22 +++-
 xen/include/public/vm_event.h  |   8 +-
 xen/include/xen/monitor.h  |   2 -
 xen/include/xen/vm_event.h |   2 -
 12 files changed, 543 insertions(+), 18 deletions(-)
 create mode 100644 xen/arch/arm/vm_event.c
 create mode 100644 xen/include/asm-arm/traps.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9a224d4..634f359 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -402,6 +402,7 @@ M:  Tamas K Lengyel 
 S: Supported
 F: xen/common/mem_access.c
 F: xen/*/vm_event.c
+F: xen/arch/*/vm_event.c
 F: xen/*/monitor.c
 F: xen/include/*/mem_access.h
 F: xen/include/*/monitor.h
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 9e38da3..390df0a 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -40,6 +40,7 @@ obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
 obj-y += smc.o
+obj-y += vm_event.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
 #obj-bin-y += o
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8c50685..af61ac3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "decode.h"
 #include "vtimer.h"
@@ -124,7 +125,12 @@ void init_traps(void)
 WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) | HCPTR_TTA,
  CPTR_EL2);
 
-/* Setup hypervisor traps */
+/* Setup hypervisor traps
+ *
+ * Note: HCR_TVM bit is also set for system-register write monitoring
+ * purposes (see vm_event_monitor_cr), but (for performance reasons) that's
+ * done selectively (see vcpu_enter_adjust_traps).
+ */
 WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
  HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
  HCR_EL2);
@@ -1720,6 +1726,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 const struct hsr_cp32 cp32 = hsr.cp32;
 int regidx = cp32.reg;
 struct vcpu *v = current;
+register_t r = get_user_reg(regs, regidx);
 
 if ( !check_conditional_instr(regs, hsr) )
 {
@@ -1730,6 +1737,61 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 switch ( hsr.bits & HSR_CP32_REGS_MASK )
 {
 /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13
+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34
+ */
+case HSR_CPREG32(SCTLR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, SCTLR);
+break;
+case HSR_CPREG32(TTBR0_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR0_32);
+break;
+case HSR_CPREG32(TTBR1_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR1_32);
+break;
+case HSR_CPREG32(TTBCR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBCR);
+break;
+case HSR_CPREG32(DACR):
+TVM_EMUL(regs, hsr, r, DACR);
+break;
+case HSR_CPREG32(DFSR):
+TVM_EMUL(regs, hsr, r, DFSR);
+break;
+case HSR_CPREG32(IFSR):
+TVM_EMUL(regs, hsr, r, IFSR);
+break;
+case HSR_CPREG32(DFAR):
+TVM_EMUL(regs, hsr, r, DFAR);
+break;
+case HSR_CPREG32(IFAR):
+TVM_EMUL(regs, hsr, r, IFAR);
+break;
+case HSR_CPREG32(ADFSR):
+TVM_EMUL(regs, hsr, r, ADFSR);
+break;
+case HSR_CPREG32(AIFSR):
+TVM_EMUL(regs, hsr, r, AIFSR);
+break;
+case HSR_CPREG32(MAIR0):
+TVM_EMUL(regs, hsr, r, MAIR0);
+break;
+case HSR_CPREG32(MAIR1):
+TVM_EMUL(regs, hsr, r, MAIR1);
+break;
+case HSR_CPREG32(AMAIR0):
+TVM_EMUL(regs, hsr, r, AMAIR0);
+break;
+case HSR_CPREG32(AMAIR1):
+TVM_EMUL(regs, hsr, r, AMAIR1);
+break;
+case HSR_CPREG32(CONTEXTIDR):
+TVM_EMUL(regs, hsr, r, CONTEXTIDR);
+break;
+
+/*
  * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
  *
  * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1853,6 +1915,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 static void do_cp15_64(struct cpu_user_regs *regs,