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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Nov 10, 2016 at 6:33 AM, Razvan Cojocaru <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?

 

>          union {
>              struct vm_event_emul_read_data read;
>              struct vm_event_emul_insn_data insn;
>     diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>     index 177319d..85cbb7c 100644
>     --- a/xen/include/public/domctl.h
>     +++ b/xen/include/public/domctl.h
>     @@ -1086,6 +1086,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
>      #define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION       5
>      #define XEN_DOMCTL_MONITOR_EVENT_CPUID                 6
>      #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       7
>     +#define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
>
>      struct xen_domctl_monitor_op {
>          uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
>     diff --git a/xen/include/public/vm_event.h
>     b/xen/include/public/vm_event.h
>     index c28be5a..ba9accf 100644
>     --- a/xen/include/public/vm_event.h
>     +++ b/xen/include/public/vm_event.h
>     @@ -105,6 +105,11 @@
>       * if any of those flags are set, only those will be honored).
>       */
>      #define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9)
>
>
> Just reading this comment it is not entirely clear whether the event
> will be sent before or after the interrupt. Also, there is a typo in
> spelling occurring.
>
>
>     +/*
>     + * Have a one-shot VM_EVENT_REASON_INTERRUPT event sent for the first
>     + * interrupt occuring after resuming the VCPU.
>     + */
>
>     +#define VM_EVENT_FLAG_GET_NEXT_INTERRUPT (1 << 10)

The event is sent before the actual interrupt effects, as is our
convention for vm_events (the test is for pending interrupts so they had
not had effects yet).

I'm not sure about "occuring", the sources I've found online suggest my
spelling is correct, but perhaps this is a case of a word that can be
spelled in more ways, such as "color" and "colour":
https://en.wiktionary.org/wiki/occuring

I can send a V3 if you'd like me to clarify when the event is being sent
with regards to interrupt delivery (in fact I think replacing the word
"occuring" with "pending" would nicely solve both your objections here).

That would work, thanks.

Tamas

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