Re: [Xen-devel] [RFC PATCH V3 09/12] x86/hvm: factor out and rename vm_event related functions into separate file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.15 at 22:46, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> +static int hvm_event_traps(long parameters, vm_event_request_t *req)
> +{
> +    int rc;
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +
> +    if ( !(parameters & HVMPME_MODE_MASK) )
> +        return 0;
> +
> +    rc = vm_event_claim_slot(currd, &currd->vm_event->monitor);
> +    if ( rc == -ENOSYS )
> +    {
> +        /* If there was no ring to handle the event, then
> +         * simple continue executing normally. */

Coding style.

> +        return 1;
> +    }
> +    else if ( rc < 0 )
> +        return rc;

Pointless "else".

> +void hvm_event_cr0(unsigned long value, unsigned long old)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +    vm_event_request_t req = {
> +        .reason = VM_EVENT_REASON_MOV_TO_CR0,
> +        .vcpu_id = curr->vcpu_id,
> +        .data.mov_to_cr.new_value = value,
> +        .data.mov_to_cr.old_value = old
> +    };
> +
> +    long params = currd->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_CR0];

No blank lines between declarations please (they're used to separate
declarations from statements). Also while I can see the need for
"curr", I don't think you need "currd" - it's being used just once.

> --- /dev/null
> +++ b/xen/include/asm-x86/hvm/event.h
> @@ -0,0 +1,40 @@
> +/*
> + * event.h: Hardware virtual machine assist events.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 
> Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#ifndef __ASM_X86_HVM_EVENT_H__
> +#define __ASM_X86_HVM_EVENT_H__
> +
> +/* Called for current VCPU on crX changes by guest */
> +void hvm_event_cr0(unsigned long value, unsigned long old);
> +void hvm_event_cr3(unsigned long value, unsigned long old);
> +void hvm_event_cr4(unsigned long value, unsigned long old);
> +void hvm_event_msr(unsigned long msr, unsigned long value);
> +/* Called for current VCPU on int3: returns -1 if no listener */
> +int hvm_event_int3(unsigned long gla);
> +int hvm_event_single_step(unsigned long gla);

Either have correct comments for all individual functions or groups
thereof, or none at all. IOW, please be consistent.

> -/* Called for current VCPU on single step: returns -1 if no listener */
> -int hvm_memory_event_single_step(unsigned long gla);

Oddly enough you can't even say you just copied what was there, as
you lost this comment in any case.

Jan


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