Re: [Xen-devel] [PATCH V2] x86/vm_event: Added support for VM_EVENT_REASON_INTERRUPT

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/10/2016 06:01 PM, Tamas K Lengyel wrote:
> 
> 
> On Thu, Nov 10, 2016 at 6:33 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>> wrote:
> 
>     Hello Tamas, thanks for the review!
> 
>     On 11/10/2016 03:11 PM, Tamas K Lengyel wrote:
>     >     diff --git a/xen/include/asm-x86/vm_event.h
>     >     b/xen/include/asm-x86/vm_event.h
>     >     index ca73f99..38c624c 100644
>     >     --- a/xen/include/asm-x86/vm_event.h
>     >     +++ b/xen/include/asm-x86/vm_event.h
>     >     @@ -27,6 +27,7 @@
>     >       */
>     >      struct arch_vm_event {
>     >          uint32_t emulate_flags;
>     >     +    bool monitor_next_interrupt;
>     >
>     >
>     > This should be in domain.h with the rest of the monitor control bits (as
>     > the name of the variable suggests as well).
> 
>     Unfortunately that would alter the semantics of the patch, as this
>     variable needs to be per-VCPU. Putting it in domain.h as you suggest
>     would make it per-domain. Looking at places to put it so that it would
>     serve this purpose, struct arch_vm_event felt like the most natural
>     place.
> 
> 
> I see. I generally like to keep vm_event/monitor bits separate as to not
> end up in the spaghetti we were in a couple years ago. Maybe introducing
> a struct arch_monitor would be appropriate?

I've actually done this, to reflect the per-domain monitor bitfield:

diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index f6a40eb..faa7037 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -576,6 +576,10 @@ struct arch_vcpu
     XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;

     struct arch_vm_event *vm_event;
+
+    struct {
+        unsigned int next_interrupt_enabled : 1;
+    } monitor;
 };

 smap_check_policy_t smap_policy_change(struct vcpu *v,

This is now accesible via v->arch.monitor.next_interrupt_enabled and
does indeed feel much cleaner (plus it removes the need for an ASSERT()
or if() that Jan has previously commented on). This will be extensible
in the future for whatever per-VCPU events we may want to add.

Is this OK?


Thanks,
Razvan

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