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

[Tamas K Lengyel <tamas@tklengyel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3420 bytes --]

On Thu, Nov 10, 2016 at 6:33 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
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

[-- Attachment #1.2: Type: text/html, Size: 4758 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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