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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Feb 10, 2015 at 1:52 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> 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.

Ack

>
>> +    {
>> +    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[].

Simply initializing parameters to be 0 is enough IMHO. The only
callers of this function already explicitly choose one of these cases.
Furthermore, hvm params are retired later in the series anyway so
there wouldn't really be much benefit in tweaking this here.

>
>> @@ -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()?

Is that only for debug cases?

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

The type of mem_access.gfn is uint64_t so its that for consistency.

>
>> +/* 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).

Ack.

>
>> +#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?

Not really, no.

>
> Jan
>

Thanks,
Tamas

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