Re: [Xen-devel] [PATCH V4 01/13] xen/mem_event: Cleanup of mem_event structures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.02.15 at 19:53, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> +static void hvm_memory_event_cr(uint32_t reason, unsigned long value,
> +                                unsigned long old)
> +{
> +    mem_event_request_t req = {
> +        .reason = reason,
> +        .vcpu_id = current->vcpu_id,
> +        .u.mov_to_cr.new_value = value,
> +        .u.mov_to_cr.old_value = old
> +    };
> +
> +    uint64_t parameters = 0 ;
> +    switch(reason)

Blank line between declarations and statements please. And no blank
before a semicolon.

> +    {
> +    case MEM_EVENT_REASON_MOV_TO_CR0:
> +        parameters = current->domain->arch.hvm_domain
> +                      .params[HVM_PARAM_MEMORY_EVENT_CR0];
> +        break;
> +    case MEM_EVENT_REASON_MOV_TO_CR3:
> +        parameters = current->domain->arch.hvm_domain
> +                      .params[HVM_PARAM_MEMORY_EVENT_CR3];
> +        break;
> +    case MEM_EVENT_REASON_MOV_TO_CR4:
> +        parameters = current->domain->arch.hvm_domain
> +                      .params[HVM_PARAM_MEMORY_EVENT_CR4];
> +        break;
> +    };

I think you should bail in the default case; perhaps add an
ASSERT_UNREACHABLE(). And with that I think readability (and
maybe even generated code) would benefit if you just latched
the index into .params[].

> @@ -598,6 +600,12 @@ int mem_sharing_sharing_resume(struct domain *d)
>      {
>          struct vcpu *v;
>  
> +        if ( rsp.version != MEM_EVENT_INTERFACE_VERSION )
> +        {
> +            gdprintk(XENLOG_WARNING, "mem_event interface version 
> mismatch!\n");

Why gdprintk()?

> @@ -1310,18 +1322,19 @@ void p2m_mem_paging_resume(struct domain *d)
>          /* Fix p2m entry if the page was not dropped */
>          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);
> +            uint64_t gfn = rsp.u.mem_access.gfn;
> +            gfn_lock(p2m, gfn, 0);

Blank line between declarations and statements. Also - why uint64_t
instead of just unsigned long?

> +/* 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              7
> +/* Debug operation executed (int3) */

If you absolutely want to mention architecture specific things in a
generic header, please make this an example (i.e. insert "e.g."
above).

> +#define MEM_EVENT_REASON_SOFTWARE_BREAKPOINT     8
> +/* Single-step (MTF) */

Same here then (this one is even VT-x specific).

> @@ -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 {

Do you really need all these _data tags?

Jan


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