On Thu, Nov 10, 2016 at 6:33 AM, Razvan Cojocaru <rcojoc...@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
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel