Re: [Xen-devel] [PATCH V5 3/3] xen/vm_event: Deny register writes if refused by vm_event reply

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.07.15 at 19:14, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Changes since V4:
>  - Rebased the patch to take into account Tamas' "x86/vm_event:
>    toggle singlestep from vm_event response".
>  - Moved the management of heap-allocated vm_event-related
>    domain data to the specific x86 vm_event domain init / cleanup
>    helpers.

I would have understood this to mean vm_event_cleanup_domain()
instead of ...

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -667,6 +667,8 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>  
>  void arch_domain_destroy(struct domain *d)
>  {
> +    xfree(d->arch.event_write_data);

... here. And I see it is being done there, so why also here?

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -22,11 +22,19 @@
>  
>  #include <xen/sched.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/vm_event.h>
>  
>  int vm_event_init_domain(struct domain *d)
>  {
>      struct vcpu *v;
>  
> +    if ( !d->arch.event_write_data )
> +        d->arch.event_write_data = xzalloc_array(struct monitor_write_data,
> +                                                 d->max_vcpus);

Looking at this again I wonder why the data isn't being made part of
struct arch_vcpu's vm_event sub-structure. That would also address
the complaint I have here about this not being a guaranteed maximum
page size runtime allocation.

> @@ -61,6 +72,38 @@ void vm_event_toggle_singlestep(struct domain *d, struct 
> vcpu *v)
>      hvm_toggle_singlestep(v);
>  }
>  
> +void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
> +{
> +    if ( rsp->flags & VM_EVENT_FLAG_DENY )
> +    {
> +        struct monitor_write_data *w =
> +            &v->domain->arch.event_write_data[v->vcpu_id];

That would also eliminate this strange construct.

> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -74,6 +74,11 @@
>   * VM_EVENT_FLAG_SET_EMUL_READ_DATA are set, only the latter will be 
> honored).
>   */
>  #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 5)
> + /*
> +  * Deny completion of the operation that triggered the event.
> +  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
> +  */
> +#define VM_EVENT_FLAG_DENY               (1 << 6)

Wouldn't this want adding to the get-capabilities sub-op too?

Jan


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