Re: [Xen-devel] [RFC PATCH V3 01/12] xen/mem_event: Cleanup of mem_event structures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.15 at 22:46, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> +static int hvm_memory_event_traps(long parameters, mem_event_request_t *req)

It looks like parameters isn't really a signed quantity...

>  void hvm_memory_event_cr0(unsigned long value, unsigned long old) 
>  {
> -    hvm_memory_event_traps(current->domain->arch.hvm_domain
> -                             .params[HVM_PARAM_MEMORY_EVENT_CR0],
> -                           MEM_EVENT_REASON_CR0,
> -                           value, old, 0, 0);
> +    mem_event_request_t req = {
> +        .reason = MEM_EVENT_REASON_MOV_TO_CR0,
> +        .vcpu_id = current->vcpu_id,
> +        .data.mov_to_cr.new_value = value,
> +        .data.mov_to_cr.old_value = old
> +    };
> +
> +    long parameters = current->domain->arch.hvm_domain
> +                        .params[HVM_PARAM_MEMORY_EVENT_CR0];
> +
> +    if ( (parameters & HVMPME_onchangeonly) && (value == old) )
> +        return;
> +
> +    hvm_memory_event_traps(parameters, &req);
>  }
>  
>  void hvm_memory_event_cr3(unsigned long value, unsigned long old) 
>  {
> -    hvm_memory_event_traps(current->domain->arch.hvm_domain
> -                             .params[HVM_PARAM_MEMORY_EVENT_CR3],
> -                           MEM_EVENT_REASON_CR3,
> -                           value, old, 0, 0);
> +    mem_event_request_t req = {
> +        .reason = MEM_EVENT_REASON_MOV_TO_CR3,
> +        .vcpu_id = current->vcpu_id,
> +        .data.mov_to_cr.new_value = value,
> +        .data.mov_to_cr.old_value = old
> +    };
> +
> +    long parameters = current->domain->arch.hvm_domain
> +                        .params[HVM_PARAM_MEMORY_EVENT_CR3];
> +
> +    if ( (parameters & HVMPME_onchangeonly) && (value == old) )
> +        return;
> +
> +    hvm_memory_event_traps(parameters, &req);
>  }
>  
>  void hvm_memory_event_cr4(unsigned long value, unsigned long old) 
>  {
> -    hvm_memory_event_traps(current->domain->arch.hvm_domain
> -                             .params[HVM_PARAM_MEMORY_EVENT_CR4],
> -                           MEM_EVENT_REASON_CR4,
> -                           value, old, 0, 0);
> +    mem_event_request_t req = {
> +        .reason = MEM_EVENT_REASON_MOV_TO_CR4,
> +        .vcpu_id = current->vcpu_id,
> +        .data.mov_to_cr.new_value = value,
> +        .data.mov_to_cr.old_value = old
> +    };
> +
> +    long parameters = current->domain->arch.hvm_domain
> +                        .params[HVM_PARAM_MEMORY_EVENT_CR4];
> +
> +    if ( (parameters & HVMPME_onchangeonly) && (value == old) )
> +        return;
> +
> +    hvm_memory_event_traps(parameters, &req);
>  }

These three functions are extremely similar, and hence there's a lot of
duplication now. Time for another helper?

> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -559,7 +559,10 @@ int mem_sharing_notify_enomem(struct domain *d, unsigned 
> long gfn,
>  {
>      struct vcpu *v = current;
>      int rc;
> -    mem_event_request_t req = { .gfn = gfn };
> +    mem_event_request_t req = {
> +        .reason = MEM_EVENT_REASON_MEM_SHARING,
> +        .data.mem_sharing.gfn = gfn
> +    };
>  
>      if ( (rc = __mem_event_claim_slot(d, 
>                          &d->mem_event->share, allow_sleep)) < 0 )
> @@ -571,7 +574,7 @@ int mem_sharing_notify_enomem(struct domain *d, unsigned 
> long gfn,
>          mem_event_vcpu_pause(v);
>      }
>  
> -    req.p2mt = p2m_ram_shared;
> +    req.data.mem_sharing.p2mt = p2m_ram_shared;
>      req.vcpu_id = v->vcpu_id;

These should both be moved up into the initialized imo.

> @@ -1309,15 +1318,15 @@ void p2m_mem_paging_resume(struct domain *d)
>          if ( !(rsp.flags & MEM_EVENT_FLAG_DROP_PAGE) )
>          {
>              gfn_lock(p2m, rsp.gfn, 0);
> -            mfn = p2m->get_entry(p2m, rsp.gfn, &p2mt, &a, 0, NULL);
> +            mfn = p2m->get_entry(p2m, rsp.data.mem_access.gfn, &p2mt, &a, 0, 
> NULL);

Long line. Also you replace rsp.gfn here, but not in the gfn_lock()
invocation one line up, which seems wrong. (Tim, Andres - shouldn't
these locking macros evaluate all their arguments?) Considering the
further uses below, for readability perhaps worth a local variable?

> @@ -1390,39 +1399,40 @@ void p2m_mem_event_emulate_check(struct vcpu *v, 
> const mem_event_response_t *rsp
>          xenmem_access_t access;
>          bool_t violation = 1;
>  
> -        if ( p2m_get_mem_access(v->domain, rsp->gfn, &access) == 0 )
> +        if ( p2m_get_mem_access(v->domain, rsp->data.mem_access.gfn, 
> &access) == 0 )

Long line again (more further down).

> --- a/xen/include/public/mem_event.h
> +++ b/xen/include/public/mem_event.h
> @@ -27,9 +27,13 @@
>  #ifndef _XEN_PUBLIC_MEM_EVENT_H
>  #define _XEN_PUBLIC_MEM_EVENT_H
>  
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
>  #include "xen.h"
>  #include "io/ring.h"
>  
> +#define MEM_EVENT_INTERFACE_VERSION 0x00000001

I think at least this and the xen.h include belong outside the
conditional above. And whether or which parts of the interface to
limit to hypervisor and tools really depends on whether you're
very certain that kernels can't use any of this (including perhaps
on themselves).

> +/* Reasons for the vm event request */
> +/* Default case */
> +#define MEM_EVENT_REASON_UNKNOWN                 0
> +/* Memory access violation */
> +#define MEM_EVENT_REASON_MEM_ACCESS              1
> +/* Memory sharing event */
> +#define MEM_EVENT_REASON_MEM_SHARING             2
> +/* Memory paging event */
> +#define MEM_EVENT_REASON_MEM_PAGING              3
> +/* CR0 was updated */
> +#define MEM_EVENT_REASON_MOV_TO_CR0              4
> +/* CR3 was updated */
> +#define MEM_EVENT_REASON_MOV_TO_CR3              5
> +/* CR4 was updated */
> +#define MEM_EVENT_REASON_MOV_TO_CR4              6
> +/* An MSR was updated. Does NOT honour HVMPME_onchangeonly */
> +#define MEM_EVENT_REASON_MOV_TO_MSR              9
> +/* Debug operation executed (int3) */
> +#define MEM_EVENT_REASON_SOFTWARE_BREAKPOINT     7
> +/* Single-step (MTF) */
> +#define MEM_EVENT_REASON_SINGLESTEP              8

Any reason why these aren't put here in order?

> @@ -97,31 +112,74 @@ struct mem_event_regs_x86 {
>      uint32_t _pad;
>  };
>  
> -typedef struct mem_event_st {
> -    uint32_t flags;
> -    uint32_t vcpu_id;
> -
> +struct mem_event_mem_access_data {
>      uint64_t gfn;
>      uint64_t offset;
>      uint64_t gla; /* if gla_valid */
> +    uint8_t access_r;
> +    uint8_t access_w;
> +    uint8_t access_x;
> +    uint8_t gla_valid;
> +    uint8_t fault_with_gla;
> +    uint8_t fault_in_gpt;
> +    uint16_t _pad;
> +};

So you decided to convert bits to bytes. Isn't that wasteful namely
as these are part of the ring entries, especially if further flags need
to be added later?

Jan

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