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
