Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-23 Thread Razvan Cojocaru 
On 02/23/2016 01:00 PM, Corneliu ZUZU wrote:
> On 2/23/2016 12:54 PM, Razvan Cojocaru wrote:
>> On 02/23/2016 11:09 AM, Corneliu ZUZU wrote:
>>> On 2/18/2016 9:35 PM, Corneliu ZUZU wrote:
 This patch adds ARM support for guest-request monitor vm-events.

 Summary of changes:
 == Moved to common-side:
* XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
arch_monitor_domctl_event to common monitor_domctl)
* hvm_event_guest_request, hvm_event_traps (also added target vcpu
 as param)
* guest-request bits from X86 'struct arch_domain' (to common
 'struct domain')
 == ARM implementations:
* do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
hvm_event_guest_request (as on X86)
* arch_monitor_get_capabilities: updated to reflect support for
XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
* vm_event_init_domain (does nothing), vm_event_cleanup_domain
 == Misc:
* hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer
X86-specific. ARM-side implementation of this function
 currently does
nothing, that will be added in a separate patch.

 Signed-off-by: Corneliu ZUZU 
>>> Before sending in the next revision of this patch, I have a few
>>> questions I'd like to ask.
>>> That being the case, I thought it would be ok to also include *all* the
>>> changes that will be done in the next revision here for (potentially)
>>> additional feedback.
>>>
>>> Already discussed changes TBD:
>>> * Add #define altp2m_active(d) (0) and implement
>>> p2m_get_vcpu_altp2m_idx(v) for ARM to remove #ifdef CONFIG_X86 in
>>> hvm_event_traps (Stefano, Tamas)
>>> * Remove wrong copyright comment (Jan)
>>> * Change bitfields members type back to unsigned int (Jan)
>>> * Move hvm_event_traps and hvm_event_guest_request to common/vm_event.c
>>> and rename them to vm_event_traps and vm_event_guest_request (Jan)
>>>
>>> Questions:
>>>
>>> 1) I've noticed the practice in Xen is to prepend the arch_ prefix to
>>> functions that usually have a counterpart on the common-side, i.e. there
>>> are a lot of arch-specific functions missing this prefix. Would it then
>>> be advised to:
>>> - rename arch_monitor_get_capabilities to
>>> vm_event_monitor_get_capabilities and move it to vm_event.h
>>> - rename arch_hvm_event_fill_regs to vm_event_fill_regs and move it
>>> to vm_event.h
>> Please see the recent commit adc75eba8b15c7103a010f736fe62e3fb2383964 in
>> staging.
>>
>>
>> Cheers,
>> Razvan
>>
> 
> Great, that settles arch_hvm_event_fill_regs -> vm_event_fill_regs.
> Should I then do the same for arch_monitor_get_capabilities (->
> vm_event_monitor_get_capabilities)?

Sounds good to me, so yes, unless Tamas or Jan think otherwise.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-22 Thread Razvan Cojocaru 
On 02/22/2016 01:38 PM, Jan Beulich wrote:
 On 22.02.16 at 12:26,  wrote:
>> On 2/22/2016 12:14 PM, Jan Beulich wrote:
>> On 19.02.16 at 19:01,  wrote:
 On 2/19/2016 7:15 PM, Jan Beulich wrote:
 On 19.02.16 at 17:25,  wrote:
>> On 2/19/2016 4:26 PM, Jan Beulich wrote:
>> On 18.02.16 at 20:35,  wrote:
>> On the "HVM-ish" note, is there some incompatibility between ARM and the
>> concept of HVM?
> ARM guests are neither PV nor HVM right now, but somewhere in
> the middle (PVHv2 may come closest).
 I did not know that, but the fact that there is already "hvm-like" code
 written for ARM didn't hint me towards that fact either :)
 I'm aware that I'm far from familiar with the codebase right now, I'm
 browsing more of the code these days and taking notes to try and
 understand in depth at least the parts I'm sending contributions for.
 I've already got some questions I want to post to the mailing list soon,
 *including* exactly how the distinction between the guest-types comes
 into play w/ the vm-events code.
 Specifically, I'm talking for example about the following piece of code
 from the X86 arch_monitor_get_capabilities:

   /*
* At the moment only Intel HVM domains are supported. However, event
* delivery could be extended to AMD and PV domains.
*/
   if ( !is_hvm_domain(d) || !cpu_has_vmx )
   return capabilities;

 == "However, event delivery could be extended to AMD and PV domains".
 This comment begs for questions like:
 * what would be necessary to extend support to PV domains?
 * can we really do this operation without hardware assisted
 virtualization whatsoever? If not, how much can we do without that?
 * what about pvh?

 Since I have other questions like the above and I'll probably have more
 while I'm trying to get a better picture of the code, would it be ok if
 we defer addressing these issues to then?
>>> Yes, you should definitely not hijack this thread for other, more
>>> general inquiries.
>>
>> Ok then, should I also understand then that for now it's ok to keep the 
>> "HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were 
>> referring to these 2 functions above) on the common-side event.c until 
>> we address these issues?
>> Or I could try to move them to common/vm_event.c as you suggest renamed 
>> to vm_event_traps & vm_event_guest_request and also rename 
>> arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?).
> 
> I'd say dropping the hvm_ suffixes / infixes would be fine (and
> even desirable) alongside their movement to common/vm_event.c,
> but the question really needs to go to the maintainers of that
> code.

I agree.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-22 Thread Jan Beulich 
>>> On 22.02.16 at 12:26,  wrote:
> On 2/22/2016 12:14 PM, Jan Beulich wrote:
> On 19.02.16 at 19:01,  wrote:
>>> On 2/19/2016 7:15 PM, Jan Beulich wrote:
>>> On 19.02.16 at 17:25,  wrote:
> On 2/19/2016 4:26 PM, Jan Beulich wrote:
> On 18.02.16 at 20:35,  wrote:
> On the "HVM-ish" note, is there some incompatibility between ARM and the
> concept of HVM?
 ARM guests are neither PV nor HVM right now, but somewhere in
 the middle (PVHv2 may come closest).
>>> I did not know that, but the fact that there is already "hvm-like" code
>>> written for ARM didn't hint me towards that fact either :)
>>> I'm aware that I'm far from familiar with the codebase right now, I'm
>>> browsing more of the code these days and taking notes to try and
>>> understand in depth at least the parts I'm sending contributions for.
>>> I've already got some questions I want to post to the mailing list soon,
>>> *including* exactly how the distinction between the guest-types comes
>>> into play w/ the vm-events code.
>>> Specifically, I'm talking for example about the following piece of code
>>> from the X86 arch_monitor_get_capabilities:
>>>
>>>   /*
>>>* At the moment only Intel HVM domains are supported. However, event
>>>* delivery could be extended to AMD and PV domains.
>>>*/
>>>   if ( !is_hvm_domain(d) || !cpu_has_vmx )
>>>   return capabilities;
>>>
>>> == "However, event delivery could be extended to AMD and PV domains".
>>> This comment begs for questions like:
>>> * what would be necessary to extend support to PV domains?
>>> * can we really do this operation without hardware assisted
>>> virtualization whatsoever? If not, how much can we do without that?
>>> * what about pvh?
>>>
>>> Since I have other questions like the above and I'll probably have more
>>> while I'm trying to get a better picture of the code, would it be ok if
>>> we defer addressing these issues to then?
>> Yes, you should definitely not hijack this thread for other, more
>> general inquiries.
> 
> Ok then, should I also understand then that for now it's ok to keep the 
> "HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were 
> referring to these 2 functions above) on the common-side event.c until 
> we address these issues?
> Or I could try to move them to common/vm_event.c as you suggest renamed 
> to vm_event_traps & vm_event_guest_request and also rename 
> arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?).

I'd say dropping the hvm_ suffixes / infixes would be fine (and
even desirable) alongside their movement to common/vm_event.c,
but the question really needs to go to the maintainers of that
code.

Jan


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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-22 Thread Corneliu ZUZU 

On 2/22/2016 12:14 PM, Jan Beulich wrote:

On 19.02.16 at 19:01,  wrote:

On 2/19/2016 7:15 PM, Jan Beulich wrote:

On 19.02.16 at 17:25,  wrote:

On 2/19/2016 4:26 PM, Jan Beulich wrote:

On 18.02.16 at 20:35,  wrote:

---
MAINTAINERS |   1 +
xen/arch/arm/hvm.c  |   8 +++
xen/arch/x86/hvm/event.c| 116 
++--
xen/arch/x86/hvm/hvm.c  |   1 +
xen/arch/x86/monitor.c  |  14 -
xen/arch/x86/vm_event.c |   1 +
xen/common/Makefile |   2 +-
xen/common/hvm/Makefile |   3 +-
xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?

Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce
that file, it was already there, all I did is add handling of
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).

No, I'm referring to your initial attempt to create arch/arm/hvm/...

I don't understand. Have I done that again with this patch?

Not the exact same thing, but something going along those same
lines of thinking.


On the "HVM-ish" note, is there some incompatibility between ARM and the
concept of HVM?

ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).

I did not know that, but the fact that there is already "hvm-like" code
written for ARM didn't hint me towards that fact either :)
I'm aware that I'm far from familiar with the codebase right now, I'm
browsing more of the code these days and taking notes to try and
understand in depth at least the parts I'm sending contributions for.
I've already got some questions I want to post to the mailing list soon,
*including* exactly how the distinction between the guest-types comes
into play w/ the vm-events code.
Specifically, I'm talking for example about the following piece of code
from the X86 arch_monitor_get_capabilities:

  /*
   * At the moment only Intel HVM domains are supported. However, event
   * delivery could be extended to AMD and PV domains.
   */
  if ( !is_hvm_domain(d) || !cpu_has_vmx )
  return capabilities;

== "However, event delivery could be extended to AMD and PV domains".
This comment begs for questions like:
* what would be necessary to extend support to PV domains?
* can we really do this operation without hardware assisted
virtualization whatsoever? If not, how much can we do without that?
* what about pvh?

Since I have other questions like the above and I'll probably have more
while I'm trying to get a better picture of the code, would it be ok if
we defer addressing these issues to then?

Yes, you should definitely not hijack this thread for other, more
general inquiries.


Ok then, should I also understand then that for now it's ok to keep the 
"HVM-ish" hvm_event_traps & hvm_event_guest_request (I suppose you were 
referring to these 2 functions above) on the common-side event.c until 
we address these issues?
Or I could try to move them to common/vm_event.c as you suggest renamed 
to vm_event_traps & vm_event_guest_request and also rename 
arch_hvm_event_fill_regs to arch_vm_event_fill_regs (?).



--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
unsigned long *pirq_eoi_map;
unsigned long pirq_eoi_map_mfn;

-/* Monitor options */

+/* Arch-specific monitor options */
struct {
-unsigned int write_ctrlreg_enabled   : 4;
-unsigned int write_ctrlreg_sync  : 4;
-unsigned int write_ctrlreg_onchangeonly  : 4;
-unsigned int mov_to_msr_enabled  : 1;
-unsigned int mov_to_msr_extended : 1;
-unsigned int singlestep_enabled  : 1;
-unsigned int software_breakpoint_enabled : 1;
-unsigned int guest_request_enabled   : 1;
-unsigned int guest_request_sync  : 1;
+uint16_t write_ctrlreg_enabled   : 4;
+uint16_t write_ctrlreg_sync  : 4;
+uint16_t write_ctrlreg_onchangeonly  : 4;
+uint16_t mov_to_msr_enabled  : 1;
+uint16_t mov_to_msr_extended : 1;
+uint16_t singlestep_enabled  : 1;
+uint16_t software_breakpoint_enabled : 1;
} monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?

Some time before this patch there was a discussion I had w/ ARM
maintainer Ian Campbell who made the suggestion to try and move parts of
the monitor vm-events code to the common-side.
(you can find that discussion here:
https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
In short, the reason for his suggestion was that that previous

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-22 Thread Jan Beulich 
>>> On 19.02.16 at 19:01,  wrote:
> On 2/19/2016 7:15 PM, Jan Beulich wrote:
> On 19.02.16 at 17:25,  wrote:
>>> On 2/19/2016 4:26 PM, Jan Beulich wrote:
>>> On 18.02.16 at 20:35,  wrote:
> ---
>MAINTAINERS |   1 +
>xen/arch/arm/hvm.c  |   8 +++
>xen/arch/x86/hvm/event.c| 116 
> ++--
>xen/arch/x86/hvm/hvm.c  |   1 +
>xen/arch/x86/monitor.c  |  14 -
>xen/arch/x86/vm_event.c |   1 +
>xen/common/Makefile |   2 +-
>xen/common/hvm/Makefile |   3 +-
>xen/common/hvm/event.c  |  96 +
 So here you _again_ try to introduce something HVM-ish for ARM.
 Why? Why can't this code live in common/vm_event.c?
>>> Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce
>>> that file, it was already there, all I did is add handling of
>>> HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).
>> No, I'm referring to your initial attempt to create arch/arm/hvm/...
> 
> I don't understand. Have I done that again with this patch?

Not the exact same thing, but something going along those same
lines of thinking.

>>> On the "HVM-ish" note, is there some incompatibility between ARM and the
>>> concept of HVM?
>> ARM guests are neither PV nor HVM right now, but somewhere in
>> the middle (PVHv2 may come closest).
> 
> I did not know that, but the fact that there is already "hvm-like" code 
> written for ARM didn't hint me towards that fact either :)
> I'm aware that I'm far from familiar with the codebase right now, I'm 
> browsing more of the code these days and taking notes to try and 
> understand in depth at least the parts I'm sending contributions for.
> I've already got some questions I want to post to the mailing list soon, 
> *including* exactly how the distinction between the guest-types comes 
> into play w/ the vm-events code.
> Specifically, I'm talking for example about the following piece of code 
> from the X86 arch_monitor_get_capabilities:
> 
>  /*
>   * At the moment only Intel HVM domains are supported. However, event
>   * delivery could be extended to AMD and PV domains.
>   */
>  if ( !is_hvm_domain(d) || !cpu_has_vmx )
>  return capabilities;
> 
> == "However, event delivery could be extended to AMD and PV domains".
> This comment begs for questions like:
> * what would be necessary to extend support to PV domains?
> * can we really do this operation without hardware assisted 
> virtualization whatsoever? If not, how much can we do without that?
> * what about pvh?
> 
> Since I have other questions like the above and I'll probably have more 
> while I'm trying to get a better picture of the code, would it be ok if 
> we defer addressing these issues to then?

Yes, you should definitely not hijack this thread for other, more
general inquiries.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -376,17 +376,15 @@ struct arch_domain
>unsigned long *pirq_eoi_map;
>unsigned long pirq_eoi_map_mfn;
>
> -/* Monitor options */
> +/* Arch-specific monitor options */
>struct {
> -unsigned int write_ctrlreg_enabled   : 4;
> -unsigned int write_ctrlreg_sync  : 4;
> -unsigned int write_ctrlreg_onchangeonly  : 4;
> -unsigned int mov_to_msr_enabled  : 1;
> -unsigned int mov_to_msr_extended : 1;
> -unsigned int singlestep_enabled  : 1;
> -unsigned int software_breakpoint_enabled : 1;
> -unsigned int guest_request_enabled   : 1;
> -unsigned int guest_request_sync  : 1;
> +uint16_t write_ctrlreg_enabled   : 4;
> +uint16_t write_ctrlreg_sync  : 4;
> +uint16_t write_ctrlreg_onchangeonly  : 4;
> +uint16_t mov_to_msr_enabled  : 1;
> +uint16_t mov_to_msr_extended : 1;
> +uint16_t singlestep_enabled  : 1;
> +uint16_t software_breakpoint_enabled : 1;
>} monitor;
 What is this type change supposed to achieve in general, and in
 particular in the context of this patch?
>>> Some time before this patch there was a discussion I had w/ ARM
>>> maintainer Ian Campbell who made the suggestion to try and move parts of
>>> the monitor vm-events code to the common-side.
>>> (you can find that discussion here:
>>> https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
>>> In short, the reason for his suggestion was that that previous attempt
>>> of mine to add guest-request support for ARM highlighted code
>>> duplication between X86 and ARM if I were to

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 8:42 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU > wrote:


On 2/19/2016 8:27 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU
> wrote:

On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini
> wrote:

On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
> > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > + vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > >
> > > > > #define altp2m_active(d) (0)
> > > > >
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too
at some point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx =
vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more
of the internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will
automatically avoid the if
> > > statement.
> > You will still get the compile error from ARM's
struct vcpu not having
> > altp2m information.
> >
> > ~Andrew
> >
>
> Yep.

Yes, you are right, especially given that Xen is
compiled -Wall -Werror.

How do you plan to introduce altp2m support on ARM? Is
there going to be
a struct altp2mvcpu on ARM too? It is not nice to access
stuff under
v->arch from common code. Maybe we need another
arch_blah function to
set altp2m_idx.


As altp2m could be implemented for ARM as well it might make
sense to start introducing bits and pieces that would make
it easier to do that work in the future. But I agree,
accessing v->arch directly from common is not a good way to
go about it.

Tamas


I am not at all familiar w/ altp2m at the moment, but I'll
try to look into it.
Since that doesn't relate so much with the code motion of
this changeset and it might not be that straightforward to
implement, would it be ok to leave the #ifdef CONFIG_X86
there for now and remove it in a separate patch?


We are trying to avoid having to do ifdefs where-ever possible.
So in this case too introducing arch-specific function(s) that
are empty for ARM would be more appropriate.

Tamas



I understand that, I was merely asking if it would be okay to do
it in another patch, because it didn't seem that straightforward.
More concretely, are you suggesting to:
* do the "#define altp2m_active(d) (0)" as Stefano suggested


That is easy enough to implement so go with that.

* incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function


Implement an arch-specific function for this, say 
p2m_get_vcpu_altp2m_idx(v) which would just return 0 on ARM for now.


Tamas



Got it.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Tamas K Lengyel 
On Fri, Feb 19, 2016 at 11:33 AM, Corneliu ZUZU 
wrote:

> On 2/19/2016 8:27 PM, Tamas K Lengyel wrote:
>
>
>
> On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU 
> wrote:
>
>> On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:
>>
>>
>>
>> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini <
>> stefano.stabell...@eu.citrix.com> wrote:
>>
>>> On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
>>> > On 2/19/2016 6:05 PM, Andrew Cooper wrote:
>>> > > On 19/02/16 16:00, Stefano Stabellini wrote:
>>> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
>>> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
>>> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
>>> > > > > > > +
>>> > > > > > > +if ( sync )
>>> > > > > > > +{
>>> > > > > > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>>> > > > > > > +vm_event_vcpu_pause(v);
>>> > > > > > > +}
>>> > > > > > > +
>>> > > > > > > +#if CONFIG_X86
>>> > > > > > > +if ( altp2m_active(d) )
>>> > > > > > I would rather
>>> > > > > >
>>> > > > > > #define altp2m_active(d) (0)
>>> > > > > >
>>> > > > > > on ARM, removing the two ifdefs in this file.
>>> > > > > Yeah, I actually wanted to get rid of that too at some point, the
>>> > > > > question is,
>>> > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm
>>> not
>>> > > > > familiar
>>> > > > > w/ altp2m design, maybe someone that knows more of the internals
>>> of that
>>> > > > > can
>>> > > > > give a suggestion.
>>> > > > If you #define altp2m_active to (0), gcc will automatically avoid
>>> the if
>>> > > > statement.
>>> > > You will still get the compile error from ARM's struct vcpu not
>>> having
>>> > > altp2m information.
>>> > >
>>> > > ~Andrew
>>> > >
>>> >
>>> > Yep.
>>>
>>> Yes, you are right, especially given that Xen is compiled -Wall -Werror.
>>>
>>> How do you plan to introduce altp2m support on ARM? Is there going to be
>>> a struct altp2mvcpu on ARM too? It is not nice to access stuff under
>>> v->arch from common code. Maybe we need another arch_blah function to
>>> set altp2m_idx.
>>>
>>
>> As altp2m could be implemented for ARM as well it might make sense to
>> start introducing bits and pieces that would make it easier to do that work
>> in the future. But I agree, accessing v->arch directly from common is not a
>> good way to go about it.
>>
>> Tamas
>>
>>
>> I am not at all familiar w/ altp2m at the moment, but I'll try to look
>> into it.
>> Since that doesn't relate so much with the code motion of this changeset
>> and it might not be that straightforward to implement, would it be ok to
>> leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch?
>>
>
> We are trying to avoid having to do ifdefs where-ever possible. So in this
> case too introducing arch-specific function(s) that are empty for ARM would
> be more appropriate.
>
> Tamas
>
>
> I understand that, I was merely asking if it would be okay to do it in
> another patch, because it didn't seem that straightforward.
> More concretely, are you suggesting to:
> * do the "#define altp2m_active(d) (0)" as Stefano suggested
>

That is easy enough to implement so go with that.


> * incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function
>

Implement an arch-specific function for this, say
p2m_get_vcpu_altp2m_idx(v) which would just return 0 on ARM for now.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 8:27 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU > wrote:


On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini
> wrote:

On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
> > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > + req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > + vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > >
> > > > > #define altp2m_active(d) (0)
> > > > >
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too at
some point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx =
vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more of
the internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will
automatically avoid the if
> > > statement.
> > You will still get the compile error from ARM's struct
vcpu not having
> > altp2m information.
> >
> > ~Andrew
> >
>
> Yep.

Yes, you are right, especially given that Xen is compiled
-Wall -Werror.

How do you plan to introduce altp2m support on ARM? Is there
going to be
a struct altp2mvcpu on ARM too? It is not nice to access
stuff under
v->arch from common code. Maybe we need another arch_blah
function to
set altp2m_idx.


As altp2m could be implemented for ARM as well it might make
sense to start introducing bits and pieces that would make it
easier to do that work in the future. But I agree, accessing
v->arch directly from common is not a good way to go about it.

Tamas


I am not at all familiar w/ altp2m at the moment, but I'll try to
look into it.
Since that doesn't relate so much with the code motion of this
changeset and it might not be that straightforward to implement,
would it be ok to leave the #ifdef CONFIG_X86 there for now and
remove it in a separate patch?


We are trying to avoid having to do ifdefs where-ever possible. So in 
this case too introducing arch-specific function(s) that are empty for 
ARM would be more appropriate.


Tamas



I understand that, I was merely asking if it would be okay to do it in 
another patch, because it didn't seem that straightforward.

More concretely, are you suggesting to:
* do the "#define altp2m_active(d) (0)" as Stefano suggested
* incorporate "vcpu_altp2m(v).p2midx" into an arch_foo function
?

That seems easy enough to do. If so, how should I call this arch_foo 
function and where would it be appropriate to put it?


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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Tamas K Lengyel 
On Fri, Feb 19, 2016 at 11:11 AM, Corneliu ZUZU 
wrote:

> On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:
>
>
>
> On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini <
> stefano.stabell...@eu.citrix.com> wrote:
>
>> On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
>> > On 2/19/2016 6:05 PM, Andrew Cooper wrote:
>> > > On 19/02/16 16:00, Stefano Stabellini wrote:
>> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
>> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
>> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
>> > > > > > > +
>> > > > > > > +if ( sync )
>> > > > > > > +{
>> > > > > > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>> > > > > > > +vm_event_vcpu_pause(v);
>> > > > > > > +}
>> > > > > > > +
>> > > > > > > +#if CONFIG_X86
>> > > > > > > +if ( altp2m_active(d) )
>> > > > > > I would rather
>> > > > > >
>> > > > > > #define altp2m_active(d) (0)
>> > > > > >
>> > > > > > on ARM, removing the two ifdefs in this file.
>> > > > > Yeah, I actually wanted to get rid of that too at some point, the
>> > > > > question is,
>> > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm
>> not
>> > > > > familiar
>> > > > > w/ altp2m design, maybe someone that knows more of the internals
>> of that
>> > > > > can
>> > > > > give a suggestion.
>> > > > If you #define altp2m_active to (0), gcc will automatically avoid
>> the if
>> > > > statement.
>> > > You will still get the compile error from ARM's struct vcpu not having
>> > > altp2m information.
>> > >
>> > > ~Andrew
>> > >
>> >
>> > Yep.
>>
>> Yes, you are right, especially given that Xen is compiled -Wall -Werror.
>>
>> How do you plan to introduce altp2m support on ARM? Is there going to be
>> a struct altp2mvcpu on ARM too? It is not nice to access stuff under
>> v->arch from common code. Maybe we need another arch_blah function to
>> set altp2m_idx.
>>
>
> As altp2m could be implemented for ARM as well it might make sense to
> start introducing bits and pieces that would make it easier to do that work
> in the future. But I agree, accessing v->arch directly from common is not a
> good way to go about it.
>
> Tamas
>
>
> I am not at all familiar w/ altp2m at the moment, but I'll try to look
> into it.
> Since that doesn't relate so much with the code motion of this changeset
> and it might not be that straightforward to implement, would it be ok to
> leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch?
>

We are trying to avoid having to do ifdefs where-ever possible. So in this
case too introducing arch-specific function(s) that are empty for ARM would
be more appropriate.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 7:54 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini 
> wrote:


On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
> > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > + vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > >
> > > > > #define altp2m_active(d) (0)
> > > > >
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too at some
point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx =
vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more of the
internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will automatically
avoid the if
> > > statement.
> > You will still get the compile error from ARM's struct vcpu
not having
> > altp2m information.
> >
> > ~Andrew
> >
>
> Yep.

Yes, you are right, especially given that Xen is compiled -Wall
-Werror.

How do you plan to introduce altp2m support on ARM? Is there going
to be
a struct altp2mvcpu on ARM too? It is not nice to access stuff under
v->arch from common code. Maybe we need another arch_blah function to
set altp2m_idx.


As altp2m could be implemented for ARM as well it might make sense to 
start introducing bits and pieces that would make it easier to do that 
work in the future. But I agree, accessing v->arch directly from 
common is not a good way to go about it.


Tamas


I am not at all familiar w/ altp2m at the moment, but I'll try to look 
into it.
Since that doesn't relate so much with the code motion of this changeset 
and it might not be that straightforward to implement, would it be ok to 
leave the #ifdef CONFIG_X86 there for now and remove it in a separate patch?


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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 7:15 PM, Jan Beulich wrote:

On 19.02.16 at 17:25,  wrote:

On 2/19/2016 4:26 PM, Jan Beulich wrote:

On 18.02.16 at 20:35,  wrote:

---
   MAINTAINERS |   1 +
   xen/arch/arm/hvm.c  |   8 +++
   xen/arch/x86/hvm/event.c| 116 
++--
   xen/arch/x86/hvm/hvm.c  |   1 +
   xen/arch/x86/monitor.c  |  14 -
   xen/arch/x86/vm_event.c |   1 +
   xen/common/Makefile |   2 +-
   xen/common/hvm/Makefile |   3 +-
   xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?

Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce
that file, it was already there, all I did is add handling of
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).

No, I'm referring to your initial attempt to create arch/arm/hvm/...


I don't understand. Have I done that again with this patch?




On the "HVM-ish" note, is there some incompatibility between ARM and the
concept of HVM?

ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).


I did not know that, but the fact that there is already "hvm-like" code 
written for ARM didn't hint me towards that fact either :)
I'm aware that I'm far from familiar with the codebase right now, I'm 
browsing more of the code these days and taking notes to try and 
understand in depth at least the parts I'm sending contributions for.
I've already got some questions I want to post to the mailing list soon, 
*including* exactly how the distinction between the guest-types comes 
into play w/ the vm-events code.
Specifically, I'm talking for example about the following piece of code 
from the X86 arch_monitor_get_capabilities:


/*
 * At the moment only Intel HVM domains are supported. However, event
 * delivery could be extended to AMD and PV domains.
 */
if ( !is_hvm_domain(d) || !cpu_has_vmx )
return capabilities;

== "However, event delivery could be extended to AMD and PV domains".
This comment begs for questions like:
* what would be necessary to extend support to PV domains?
* can we really do this operation without hardware assisted 
virtualization whatsoever? If not, how much can we do without that?

* what about pvh?

Since I have other questions like the above and I'll probably have more 
while I'm trying to get a better picture of the code, would it be ok if 
we defer addressing these issues to then?





--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
   unsigned long *pirq_eoi_map;
   unsigned long pirq_eoi_map_mfn;
   
-/* Monitor options */

+/* Arch-specific monitor options */
   struct {
-unsigned int write_ctrlreg_enabled   : 4;
-unsigned int write_ctrlreg_sync  : 4;
-unsigned int write_ctrlreg_onchangeonly  : 4;
-unsigned int mov_to_msr_enabled  : 1;
-unsigned int mov_to_msr_extended : 1;
-unsigned int singlestep_enabled  : 1;
-unsigned int software_breakpoint_enabled : 1;
-unsigned int guest_request_enabled   : 1;
-unsigned int guest_request_sync  : 1;
+uint16_t write_ctrlreg_enabled   : 4;
+uint16_t write_ctrlreg_sync  : 4;
+uint16_t write_ctrlreg_onchangeonly  : 4;
+uint16_t mov_to_msr_enabled  : 1;
+uint16_t mov_to_msr_extended : 1;
+uint16_t singlestep_enabled  : 1;
+uint16_t software_breakpoint_enabled : 1;
   } monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?

Some time before this patch there was a discussion I had w/ ARM
maintainer Ian Campbell who made the suggestion to try and move parts of
the monitor vm-events code to the common-side.
(you can find that discussion here:
https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
In short, the reason for his suggestion was that that previous attempt
of mine to add guest-request support for ARM highlighted code
duplication between X86 and ARM if I were to leave the monitor vm-events
code as it was (that is, completely arch-specific). In an effort to
avoid that, I first submitted a 7 patch-series which tried to move as
much as possible to the common-side.
"As much as possible" meant guest-request, ctrl-reg write, single-step
and software breakpoints, all moved to the common-side. That also meant
introducing some Kconfigs to selectively activate some parts of that
now-common code, since not all of it was used on ARM at that moment.
Although conceptually that code could have remained on the common-side,
Tamas pointed out that we could get rid of

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Tamas K Lengyel 
On Fri, Feb 19, 2016 at 10:47 AM, Stefano Stabellini <
stefano.stabell...@eu.citrix.com> wrote:

> On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > > On 19/02/16 16:00, Stefano Stabellini wrote:
> > > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > > +
> > > > > > > +if ( sync )
> > > > > > > +{
> > > > > > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > > +vm_event_vcpu_pause(v);
> > > > > > > +}
> > > > > > > +
> > > > > > > +#if CONFIG_X86
> > > > > > > +if ( altp2m_active(d) )
> > > > > > I would rather
> > > > > >
> > > > > > #define altp2m_active(d) (0)
> > > > > >
> > > > > > on ARM, removing the two ifdefs in this file.
> > > > > Yeah, I actually wanted to get rid of that too at some point, the
> > > > > question is,
> > > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm
> not
> > > > > familiar
> > > > > w/ altp2m design, maybe someone that knows more of the internals
> of that
> > > > > can
> > > > > give a suggestion.
> > > > If you #define altp2m_active to (0), gcc will automatically avoid
> the if
> > > > statement.
> > > You will still get the compile error from ARM's struct vcpu not having
> > > altp2m information.
> > >
> > > ~Andrew
> > >
> >
> > Yep.
>
> Yes, you are right, especially given that Xen is compiled -Wall -Werror.
>
> How do you plan to introduce altp2m support on ARM? Is there going to be
> a struct altp2mvcpu on ARM too? It is not nice to access stuff under
> v->arch from common code. Maybe we need another arch_blah function to
> set altp2m_idx.
>

As altp2m could be implemented for ARM as well it might make sense to start
introducing bits and pieces that would make it easier to do that work in
the future. But I agree, accessing v->arch directly from common is not a
good way to go about it.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Stefano Stabellini 
On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 6:05 PM, Andrew Cooper wrote:
> > On 19/02/16 16:00, Stefano Stabellini wrote:
> > > On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> > > > On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > > > > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > > > > +
> > > > > > +if ( sync )
> > > > > > +{
> > > > > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > > > > +vm_event_vcpu_pause(v);
> > > > > > +}
> > > > > > +
> > > > > > +#if CONFIG_X86
> > > > > > +if ( altp2m_active(d) )
> > > > > I would rather
> > > > > 
> > > > > #define altp2m_active(d) (0)
> > > > > 
> > > > > on ARM, removing the two ifdefs in this file.
> > > > Yeah, I actually wanted to get rid of that too at some point, the
> > > > question is,
> > > > what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not
> > > > familiar
> > > > w/ altp2m design, maybe someone that knows more of the internals of that
> > > > can
> > > > give a suggestion.
> > > If you #define altp2m_active to (0), gcc will automatically avoid the if
> > > statement.
> > You will still get the compile error from ARM's struct vcpu not having
> > altp2m information.
> > 
> > ~Andrew
> > 
> 
> Yep.

Yes, you are right, especially given that Xen is compiled -Wall -Werror.

How do you plan to introduce altp2m support on ARM? Is there going to be
a struct altp2mvcpu on ARM too? It is not nice to access stuff under
v->arch from common code. Maybe we need another arch_blah function to
set altp2m_idx.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Jan Beulich 
>>> On 19.02.16 at 17:25,  wrote:
> On 2/19/2016 4:26 PM, Jan Beulich wrote:
> On 18.02.16 at 20:35,  wrote:
>>> ---
>>>   MAINTAINERS |   1 +
>>>   xen/arch/arm/hvm.c  |   8 +++
>>>   xen/arch/x86/hvm/event.c| 116 
>>> ++--
>>>   xen/arch/x86/hvm/hvm.c  |   1 +
>>>   xen/arch/x86/monitor.c  |  14 -
>>>   xen/arch/x86/vm_event.c |   1 +
>>>   xen/common/Makefile |   2 +-
>>>   xen/common/hvm/Makefile |   3 +-
>>>   xen/common/hvm/event.c  |  96 +
>> So here you _again_ try to introduce something HVM-ish for ARM.
>> Why? Why can't this code live in common/vm_event.c?
> 
> Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce 
> that file, it was already there, all I did is add handling of 
> HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).

No, I'm referring to your initial attempt to create arch/arm/hvm/...

> On the "HVM-ish" note, is there some incompatibility between ARM and the 
> concept of HVM?

ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).

>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -376,17 +376,15 @@ struct arch_domain
>>>   unsigned long *pirq_eoi_map;
>>>   unsigned long pirq_eoi_map_mfn;
>>>   
>>> -/* Monitor options */
>>> +/* Arch-specific monitor options */
>>>   struct {
>>> -unsigned int write_ctrlreg_enabled   : 4;
>>> -unsigned int write_ctrlreg_sync  : 4;
>>> -unsigned int write_ctrlreg_onchangeonly  : 4;
>>> -unsigned int mov_to_msr_enabled  : 1;
>>> -unsigned int mov_to_msr_extended : 1;
>>> -unsigned int singlestep_enabled  : 1;
>>> -unsigned int software_breakpoint_enabled : 1;
>>> -unsigned int guest_request_enabled   : 1;
>>> -unsigned int guest_request_sync  : 1;
>>> +uint16_t write_ctrlreg_enabled   : 4;
>>> +uint16_t write_ctrlreg_sync  : 4;
>>> +uint16_t write_ctrlreg_onchangeonly  : 4;
>>> +uint16_t mov_to_msr_enabled  : 1;
>>> +uint16_t mov_to_msr_extended : 1;
>>> +uint16_t singlestep_enabled  : 1;
>>> +uint16_t software_breakpoint_enabled : 1;
>>>   } monitor;
>> What is this type change supposed to achieve in general, and in
>> particular in the context of this patch?
> 
> Some time before this patch there was a discussion I had w/ ARM 
> maintainer Ian Campbell who made the suggestion to try and move parts of 
> the monitor vm-events code to the common-side.
> (you can find that discussion here: 
> https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
> In short, the reason for his suggestion was that that previous attempt 
> of mine to add guest-request support for ARM highlighted code 
> duplication between X86 and ARM if I were to leave the monitor vm-events 
> code as it was (that is, completely arch-specific). In an effort to 
> avoid that, I first submitted a 7 patch-series which tried to move as 
> much as possible to the common-side.
> "As much as possible" meant guest-request, ctrl-reg write, single-step 
> and software breakpoints, all moved to the common-side. That also meant 
> introducing some Kconfigs to selectively activate some parts of that 
> now-common code, since not all of it was used on ARM at that moment. 
> Although conceptually that code could have remained on the common-side, 
> Tamas pointed out that we could get rid of the Kconfigs (which in his 
> opinion bloated the code) by only moving at the moment what is 
> implemented on both X86 and ARM. As for the rest, he noted that when I 
> add implementations for the other monitor vm-events on ARM as well, I 
> could move that to common as well. The above is a result of that - 
> guest-request is implemented on both X86 and ARM with this patch, hence 
> I also moved it to common.

I agree there are two fields being removed. But the question was
why you also change the type of the other fields.

>>> --- a/xen/include/asm-x86/hvm/event.h
>>> +++ b/xen/include/asm-x86/hvm/event.h
>>> @@ -1,5 +1,9 @@
>>>   /*
>>> - * event.h: Hardware virtual machine assist events.
>>> + * include/asm-x86/hvm/event.h
>>> + *
>>> + * Arch-specific hardware virtual machine event abstractions.
>>> + *
>>> + * Copyright (c) 2016, Bitdefender S.R.L.
>> Is this true? Namely, was _all_ of the code here written by people
>> on your company, including what may have got moved here from
>> elsewhere?
> 
> My bad. Removing that line would be satisfactory, yes?

To me, yes.

Jan

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 6:02 PM, Tamas K Lengyel wrote:



On Fri, Feb 19, 2016 at 7:26 AM, Jan Beulich > wrote:


>>> On 18.02.16 at 20:35, > wrote:
> ---
>  MAINTAINERS |   1 +
>  xen/arch/arm/hvm.c  |   8 +++
>  xen/arch/x86/hvm/event.c| 116
++--
>  xen/arch/x86/hvm/hvm.c  |   1 +
>  xen/arch/x86/monitor.c  |  14 -
>  xen/arch/x86/vm_event.c |   1 +
>  xen/common/Makefile |   2 +-
>  xen/common/hvm/Makefile |   3 +-
>  xen/common/hvm/event.c  |  96
+

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?


I too am wondering if this is the right way to architect this. It 
would be better to move the guest-requested stuff into the generic 
vm_event component as it doesn't seem to be HVM specific other then it 
using an HVMOP hypercall to be triggered.


Tamas



Oh, that. "xen/common/hvm/event.c". I too don't know if it's the right 
way, but Jan, please at least don't attribute the way the code already 
is to me, I did not architect it.
And it's not human to expect doing everything perfectly in a single 
shot. If you're of the opinion that it should be in vm_event.c I will 
gladly try to put it there. Of course, that

could also be done in another patch.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 4:26 PM, Jan Beulich wrote:

On 18.02.16 at 20:35,  wrote:

---
  MAINTAINERS |   1 +
  xen/arch/arm/hvm.c  |   8 +++
  xen/arch/x86/hvm/event.c| 116 ++--
  xen/arch/x86/hvm/hvm.c  |   1 +
  xen/arch/x86/monitor.c  |  14 -
  xen/arch/x86/vm_event.c |   1 +
  xen/common/Makefile |   2 +-
  xen/common/hvm/Makefile |   3 +-
  xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?


Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce 
that file, it was already there, all I did is add handling of 
HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).
On the "HVM-ish" note, is there some incompatibility between ARM and the 
concept of HVM?





--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -376,17 +376,15 @@ struct arch_domain
  unsigned long *pirq_eoi_map;
  unsigned long pirq_eoi_map_mfn;
  
-/* Monitor options */

+/* Arch-specific monitor options */
  struct {
-unsigned int write_ctrlreg_enabled   : 4;
-unsigned int write_ctrlreg_sync  : 4;
-unsigned int write_ctrlreg_onchangeonly  : 4;
-unsigned int mov_to_msr_enabled  : 1;
-unsigned int mov_to_msr_extended : 1;
-unsigned int singlestep_enabled  : 1;
-unsigned int software_breakpoint_enabled : 1;
-unsigned int guest_request_enabled   : 1;
-unsigned int guest_request_sync  : 1;
+uint16_t write_ctrlreg_enabled   : 4;
+uint16_t write_ctrlreg_sync  : 4;
+uint16_t write_ctrlreg_onchangeonly  : 4;
+uint16_t mov_to_msr_enabled  : 1;
+uint16_t mov_to_msr_extended : 1;
+uint16_t singlestep_enabled  : 1;
+uint16_t software_breakpoint_enabled : 1;
  } monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?


Some time before this patch there was a discussion I had w/ ARM 
maintainer Ian Campbell who made the suggestion to try and move parts of 
the monitor vm-events code to the common-side.
(you can find that discussion here: 
https://www.mail-archive.com/xen-devel@lists.xen.org/msg54139.html).
In short, the reason for his suggestion was that that previous attempt 
of mine to add guest-request support for ARM highlighted code 
duplication between X86 and ARM if I were to leave the monitor vm-events 
code as it was (that is, completely arch-specific). In an effort to 
avoid that, I first submitted a 7 patch-series which tried to move as 
much as possible to the common-side.
"As much as possible" meant guest-request, ctrl-reg write, single-step 
and software breakpoints, all moved to the common-side. That also meant 
introducing some Kconfigs to selectively activate some parts of that 
now-common code, since not all of it was used on ARM at that moment. 
Although conceptually that code could have remained on the common-side, 
Tamas pointed out that we could get rid of the Kconfigs (which in his 
opinion bloated the code) by only moving at the moment what is 
implemented on both X86 and ARM. As for the rest, he noted that when I 
add implementations for the other monitor vm-events on ARM as well, I 
could move that to common as well. The above is a result of that - 
guest-request is implemented on both X86 and ARM with this patch, hence 
I also moved it to common.



--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -1,5 +1,9 @@
  /*
- * event.h: Hardware virtual machine assist events.
+ * include/asm-x86/hvm/event.h
+ *
+ * Arch-specific hardware virtual machine event abstractions.
+ *
+ * Copyright (c) 2016, Bitdefender S.R.L.

Is this true? Namely, was _all_ of the code here written by people
on your company, including what may have got moved here from
elsewhere?

Jan




My bad. Removing that line would be satisfactory, yes?

Corneliu.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 6:05 PM, Andrew Cooper wrote:

On 19/02/16 16:00, Stefano Stabellini wrote:

On Fri, 19 Feb 2016, Corneliu ZUZU wrote:

On 2/19/2016 3:49 PM, Stefano Stabellini wrote:

On Thu, 18 Feb 2016, Corneliu ZUZU wrote:

+
+if ( sync )
+{
+req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+vm_event_vcpu_pause(v);
+}
+
+#if CONFIG_X86
+if ( altp2m_active(d) )

I would rather

#define altp2m_active(d) (0)

on ARM, removing the two ifdefs in this file.

Yeah, I actually wanted to get rid of that too at some point, the question is,
what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar
w/ altp2m design, maybe someone that knows more of the internals of that can
give a suggestion.

If you #define altp2m_active to (0), gcc will automatically avoid the if
statement.

You will still get the compile error from ARM's struct vcpu not having
altp2m information.

~Andrew



Yep.

Corneliu.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Andrew Cooper 
On 19/02/16 16:00, Stefano Stabellini wrote:
> On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
>> On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
>>> On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
 +
 +if ( sync )
 +{
 +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
 +vm_event_vcpu_pause(v);
 +}
 +
 +#if CONFIG_X86
 +if ( altp2m_active(d) )
>>> I would rather
>>>
>>> #define altp2m_active(d) (0)
>>>
>>> on ARM, removing the two ifdefs in this file.
>> Yeah, I actually wanted to get rid of that too at some point, the question 
>> is,
>> what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar
>> w/ altp2m design, maybe someone that knows more of the internals of that can
>> give a suggestion.
> If you #define altp2m_active to (0), gcc will automatically avoid the if
> statement.

You will still get the compile error from ARM's struct vcpu not having
altp2m information.

~Andrew

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Tamas K Lengyel 
On Fri, Feb 19, 2016 at 7:26 AM, Jan Beulich  wrote:

> >>> On 18.02.16 at 20:35,  wrote:
> > ---
> >  MAINTAINERS |   1 +
> >  xen/arch/arm/hvm.c  |   8 +++
> >  xen/arch/x86/hvm/event.c| 116
> ++--
> >  xen/arch/x86/hvm/hvm.c  |   1 +
> >  xen/arch/x86/monitor.c  |  14 -
> >  xen/arch/x86/vm_event.c |   1 +
> >  xen/common/Makefile |   2 +-
> >  xen/common/hvm/Makefile |   3 +-
> >  xen/common/hvm/event.c  |  96 +
>
> So here you _again_ try to introduce something HVM-ish for ARM.
> Why? Why can't this code live in common/vm_event.c?
>

I too am wondering if this is the right way to architect this. It would be
better to move the guest-requested stuff into the generic vm_event
component as it doesn't seem to be HVM specific other then it using an
HVMOP hypercall to be triggered.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Stefano Stabellini 
On Fri, 19 Feb 2016, Corneliu ZUZU wrote:
> On 2/19/2016 3:49 PM, Stefano Stabellini wrote:
> > On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> > > +
> > > +if ( sync )
> > > +{
> > > +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> > > +vm_event_vcpu_pause(v);
> > > +}
> > > +
> > > +#if CONFIG_X86
> > > +if ( altp2m_active(d) )
> > I would rather
> > 
> > #define altp2m_active(d) (0)
> > 
> > on ARM, removing the two ifdefs in this file.
> 
> Yeah, I actually wanted to get rid of that too at some point, the question is,
> what do I do with "req->altp2m_idx = vcpu_altp2m(v).p2midx"? I'm not familiar
> w/ altp2m design, maybe someone that knows more of the internals of that can
> give a suggestion.

If you #define altp2m_active to (0), gcc will automatically avoid the if
statement.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Corneliu ZUZU 

On 2/19/2016 3:49 PM, Stefano Stabellini wrote:

On Thu, 18 Feb 2016, Corneliu ZUZU wrote:

+
+if ( sync )
+{
+req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+vm_event_vcpu_pause(v);
+}
+
+#if CONFIG_X86
+if ( altp2m_active(d) )

I would rather

#define altp2m_active(d) (0)

on ARM, removing the two ifdefs in this file.


Yeah, I actually wanted to get rid of that too at some point, the 
question is, what do I do with "req->altp2m_idx = 
vcpu_altp2m(v).p2midx"? I'm not familiar w/ altp2m design, maybe someone 
that knows more of the internals of that can give a suggestion.


Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Jan Beulich 
>>> On 18.02.16 at 20:35,  wrote:
> ---
>  MAINTAINERS |   1 +
>  xen/arch/arm/hvm.c  |   8 +++
>  xen/arch/x86/hvm/event.c| 116 
> ++--
>  xen/arch/x86/hvm/hvm.c  |   1 +
>  xen/arch/x86/monitor.c  |  14 -
>  xen/arch/x86/vm_event.c |   1 +
>  xen/common/Makefile |   2 +-
>  xen/common/hvm/Makefile |   3 +-
>  xen/common/hvm/event.c  |  96 +

So here you _again_ try to introduce something HVM-ish for ARM.
Why? Why can't this code live in common/vm_event.c?

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -376,17 +376,15 @@ struct arch_domain
>  unsigned long *pirq_eoi_map;
>  unsigned long pirq_eoi_map_mfn;
>  
> -/* Monitor options */
> +/* Arch-specific monitor options */
>  struct {
> -unsigned int write_ctrlreg_enabled   : 4;
> -unsigned int write_ctrlreg_sync  : 4;
> -unsigned int write_ctrlreg_onchangeonly  : 4;
> -unsigned int mov_to_msr_enabled  : 1;
> -unsigned int mov_to_msr_extended : 1;
> -unsigned int singlestep_enabled  : 1;
> -unsigned int software_breakpoint_enabled : 1;
> -unsigned int guest_request_enabled   : 1;
> -unsigned int guest_request_sync  : 1;
> +uint16_t write_ctrlreg_enabled   : 4;
> +uint16_t write_ctrlreg_sync  : 4;
> +uint16_t write_ctrlreg_onchangeonly  : 4;
> +uint16_t mov_to_msr_enabled  : 1;
> +uint16_t mov_to_msr_extended : 1;
> +uint16_t singlestep_enabled  : 1;
> +uint16_t software_breakpoint_enabled : 1;
>  } monitor;

What is this type change supposed to achieve in general, and in
particular in the context of this patch?

> --- a/xen/include/asm-x86/hvm/event.h
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -1,5 +1,9 @@
>  /*
> - * event.h: Hardware virtual machine assist events.
> + * include/asm-x86/hvm/event.h
> + *
> + * Arch-specific hardware virtual machine event abstractions.
> + *
> + * Copyright (c) 2016, Bitdefender S.R.L.

Is this true? Namely, was _all_ of the code here written by people
on your company, including what may have got moved here from
elsewhere?

Jan


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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Razvan Cojocaru 
On 02/19/2016 03:49 PM, Stefano Stabellini wrote:
> On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cd4da04..768ad32 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -356,6 +356,7 @@ M:   Tamas K Lengyel 
>>  S:  Supported
>>  F:  xen/common/vm_event.c
>>  F:  xen/common/mem_access.c
>> +F:  xen/common/hvm/event.c
>>  F:  xen/common/monitor.c
>>  F:  xen/arch/x86/hvm/event.c
>>  F:  xen/arch/x86/monitor.c
> 
> This needs an Ack from Tamas or Razvan

The MAINTAINERS change is fine with me. Unless Tamas has an issue with
it (which he most likely doesn't), it should be fine.

>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 056db1a..767edd1 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -22,6 +22,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  
>> @@ -72,6 +73,13 @@ long do_hvm_op(unsigned long op, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>  break;
>>  }
>>  
>> +case HVMOP_guest_request_vm_event:
>> +if ( guest_handle_is_null(arg) )
>> +hvm_event_guest_request();
>> +else
>> +rc = -EINVAL;
>> +break;
>> +
>>  default:
>>  {
>>  gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index 0d76efe..703faa5 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -67,7 +67,7 @@ obj-$(xenoprof)+= xenoprof.o
>>  
>>  obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o 
>> multicall.o tmem_xen.o xlat.o)
>>  
>> -subdir-$(CONFIG_X86) += hvm
>> +subdir-y += hvm
>>  
>>  subdir-$(coverage) += gcov
>>  
>> diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile
>> index a464a57..11e109d 100644
>> --- a/xen/common/hvm/Makefile
>> +++ b/xen/common/hvm/Makefile
>> @@ -1 +1,2 @@
>> -obj-y += save.o
>> +obj-$(CONFIG_X86) += save.o
>> +obj-y += event.o
>> diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
>> new file mode 100644
>> index 000..28ceadc
>> --- /dev/null
>> +++ b/xen/common/hvm/event.c
>> @@ -0,0 +1,96 @@
>> +/*
>> + * xen/common/hvm/event.c
>> + *
>> + * Common hardware virtual machine event abstractions.
>> + *
>> + * Copyright (c) 2004, Intel Corporation.
>> + * Copyright (c) 2005, International Business Machines Corporation.
>> + * Copyright (c) 2008, Citrix Systems, Inc.
>> + * Copyright (c) 2016, Bitdefender S.R.L.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along 
>> with
>> + * this program; If not, see .
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#if CONFIG_X86
>> +#include 
>> +#endif
>> +
>> +int hvm_event_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req)
>> +{
>> +int rc;
>> +struct domain *d = v->domain;
>> +
>> +rc = vm_event_claim_slot(d, >vm_event->monitor);
>> +switch ( rc )
>> +{
>> +case 0:
>> +break;
>> +case -ENOSYS:
>> +/*
>> + * If there was no ring to handle the event, then
>> + * simply continue executing normally.
>> + */
>> +return 1;
>> +default:
>> +return rc;
>> +};
>> +
>> +if ( sync )
>> +{
>> +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>> +vm_event_vcpu_pause(v);
>> +}
>> +
>> +#if CONFIG_X86
>> +if ( altp2m_active(d) )
> 
> I would rather
> 
> #define altp2m_active(d) (0)
> 
> on ARM, removing the two ifdefs in this file.

I second that, not a big fan of the #ifdefs.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-19 Thread Stefano Stabellini 
On Thu, 18 Feb 2016, Corneliu ZUZU wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cd4da04..768ad32 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -356,6 +356,7 @@ M:Tamas K Lengyel 
>  S:   Supported
>  F:   xen/common/vm_event.c
>  F:   xen/common/mem_access.c
> +F:   xen/common/hvm/event.c
>  F:   xen/common/monitor.c
>  F:   xen/arch/x86/hvm/event.c
>  F:   xen/arch/x86/monitor.c

This needs an Ack from Tamas or Razvan



> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 056db1a..767edd1 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -72,6 +73,13 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  break;
>  }
>  
> +case HVMOP_guest_request_vm_event:
> +if ( guest_handle_is_null(arg) )
> +hvm_event_guest_request();
> +else
> +rc = -EINVAL;
> +break;
> +
>  default:
>  {
>  gdprintk(XENLOG_DEBUG, "HVMOP op=%lu: not implemented\n", op);
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 0d76efe..703faa5 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -67,7 +67,7 @@ obj-$(xenoprof)+= xenoprof.o
>  
>  obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o 
> multicall.o tmem_xen.o xlat.o)
>  
> -subdir-$(CONFIG_X86) += hvm
> +subdir-y += hvm
>  
>  subdir-$(coverage) += gcov
>  
> diff --git a/xen/common/hvm/Makefile b/xen/common/hvm/Makefile
> index a464a57..11e109d 100644
> --- a/xen/common/hvm/Makefile
> +++ b/xen/common/hvm/Makefile
> @@ -1 +1,2 @@
> -obj-y += save.o
> +obj-$(CONFIG_X86) += save.o
> +obj-y += event.o
> diff --git a/xen/common/hvm/event.c b/xen/common/hvm/event.c
> new file mode 100644
> index 000..28ceadc
> --- /dev/null
> +++ b/xen/common/hvm/event.c
> @@ -0,0 +1,96 @@
> +/*
> + * xen/common/hvm/event.c
> + *
> + * Common hardware virtual machine event abstractions.
> + *
> + * Copyright (c) 2004, Intel Corporation.
> + * Copyright (c) 2005, International Business Machines Corporation.
> + * Copyright (c) 2008, Citrix Systems, Inc.
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#if CONFIG_X86
> +#include 
> +#endif
> +
> +int hvm_event_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req)
> +{
> +int rc;
> +struct domain *d = v->domain;
> +
> +rc = vm_event_claim_slot(d, >vm_event->monitor);
> +switch ( rc )
> +{
> +case 0:
> +break;
> +case -ENOSYS:
> +/*
> + * If there was no ring to handle the event, then
> + * simply continue executing normally.
> + */
> +return 1;
> +default:
> +return rc;
> +};
> +
> +if ( sync )
> +{
> +req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> +vm_event_vcpu_pause(v);
> +}
> +
> +#if CONFIG_X86
> +if ( altp2m_active(d) )

I would rather

#define altp2m_active(d) (0)

on ARM, removing the two ifdefs in this file.


> +{
> +req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
> +req->altp2m_idx = vcpu_altp2m(v).p2midx;
> +}
> +#endif
> +
> +arch_hvm_event_fill_regs(req);
> +
> +vm_event_put_request(d, >vm_event->monitor, req);
> +
> +return 1;
> +}
> +
> +void hvm_event_guest_request(void)
> +{
> +struct vcpu *curr = current;
> +struct domain *d = curr->domain;
> +
> +if ( d->monitor.guest_request_enabled )
> +{
> +vm_event_request_t req = {
> +.reason = VM_EVENT_REASON_GUEST_REQUEST,
> +.vcpu_id = curr->vcpu_id,
> +};
> +
> +hvm_event_traps(curr, d->monitor.guest_request_sync, );
> +}
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-18 Thread Corneliu ZUZU 

On 2/18/2016 11:25 PM, Tamas K Lengyel wrote:



On Thu, Feb 18, 2016 at 1:08 PM, Razvan Cojocaru 
> wrote:


On 02/18/2016 09:35 PM, Corneliu ZUZU wrote:
> This patch adds ARM support for guest-request monitor vm-events.
>
> Summary of changes:
> == Moved to common-side:
>   * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
>   arch_monitor_domctl_event to common monitor_domctl)
>   * hvm_event_guest_request, hvm_event_traps (also added target
vcpu as param)
>   * guest-request bits from X86 'struct arch_domain' (to common
'struct domain')
> == ARM implementations:
>   * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
>   hvm_event_guest_request (as on X86)
>   * arch_monitor_get_capabilities: updated to reflect support for
>   XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
>   * vm_event_init_domain (does nothing), vm_event_cleanup_domain
> == Misc:
>   * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no
longer
>   X86-specific. ARM-side implementation of this function
currently does
>   nothing, that will be added in a separate patch.

We should probably take into account what happens with Tamas'
"vm_event:
consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch
here.
That patch already affects hvm_event_fill_regs().


Well it seems one of us will have to rebase depending which patch gets 
accepted & merged first. The conflict is minimal so it's not a major 
issue. If my patch gets merged first then just have to introduce the 
empty function in the ARM header with the new name.


Tamas



Okay then, for me it's fine either way.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-18 Thread Tamas K Lengyel 
On Thu, Feb 18, 2016 at 1:08 PM, Razvan Cojocaru 
wrote:

> On 02/18/2016 09:35 PM, Corneliu ZUZU wrote:
> > This patch adds ARM support for guest-request monitor vm-events.
> >
> > Summary of changes:
> > == Moved to common-side:
> >   * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
> >   arch_monitor_domctl_event to common monitor_domctl)
> >   * hvm_event_guest_request, hvm_event_traps (also added target vcpu as
> param)
> >   * guest-request bits from X86 'struct arch_domain' (to common 'struct
> domain')
> > == ARM implementations:
> >   * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
> >   hvm_event_guest_request (as on X86)
> >   * arch_monitor_get_capabilities: updated to reflect support for
> >   XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
> >   * vm_event_init_domain (does nothing), vm_event_cleanup_domain
> > == Misc:
> >   * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer
> >   X86-specific. ARM-side implementation of this function currently
> does
> >   nothing, that will be added in a separate patch.
>
> We should probably take into account what happens with Tamas' "vm_event:
> consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch here.
> That patch already affects hvm_event_fill_regs().
>

Well it seems one of us will have to rebase depending which patch gets
accepted & merged first. The conflict is minimal so it's not a major issue.
If my patch gets merged first then just have to introduce the empty
function in the ARM header with the new name.

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

Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

2016-02-18 Thread Razvan Cojocaru 
On 02/18/2016 09:35 PM, Corneliu ZUZU wrote:
> This patch adds ARM support for guest-request monitor vm-events.
> 
> Summary of changes:
> == Moved to common-side:
>   * XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST handling (moved from X86
>   arch_monitor_domctl_event to common monitor_domctl)
>   * hvm_event_guest_request, hvm_event_traps (also added target vcpu as param)
>   * guest-request bits from X86 'struct arch_domain' (to common 'struct 
> domain')
> == ARM implementations:
>   * do_hvm_op now handling of HVMOP_guest_request_vm_event => calls
>   hvm_event_guest_request (as on X86)
>   * arch_monitor_get_capabilities: updated to reflect support for
>   XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST
>   * vm_event_init_domain (does nothing), vm_event_cleanup_domain
> == Misc:
>   * hvm_event_fill_regs renamed to arch_hvm_event_fill_regs, no longer
>   X86-specific. ARM-side implementation of this function currently does
>   nothing, that will be added in a separate patch.

We should probably take into account what happens with Tamas' "vm_event:
consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs" patch here.
That patch already affects hvm_event_fill_regs().

Tamas'll probably chime in with more.


Thanks,
Razvan

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