Re: [Xen-devel] [PATCH 06/18] PVH xen: Introduce PVH guest type and some basic changes.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -644,6 +644,13 @@ int arch_set_info_guest(
>      unsigned int i;
>      int rc = 0, compat;
>  
> +    /* This removed when all patches are checked in and PVH is done. */
> +    if ( is_pvh_vcpu(v) )
> +    {
> +        printk("PVH: You don't have the correct xen version for PVH\n");
> +        return -EINVAL;
> +    }
> +

As the patch doesn't add code setting guest_type to is_pvh, this is
pointless to be added here. The only logical thing, if at all, would be
an ASSERT().

> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -38,7 +38,13 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +#ifndef NDEBUG
> +/* PVH 32bitfixme : see emulate_gate_op call from do_general_protection */
> +#define GUEST_KERNEL_RPL(d) (is_pvh_domain(d) ? ({ BUG(); 0; }) :    \
> +                                                is_pv_32bit_domain(d) ? 1 : 
> 3)
> +#else
>  #define GUEST_KERNEL_RPL(d) (is_pv_32bit_domain(d) ? 1 : 3)
> +#endif

As it is easily doable, please without explicit check of NDEBUG. E.g.

#define GUEST_KERNEL_RPL(d) ({ ASSERT(!is_pvh_domain(d)); \
                                                is_pv_32bit_domain(d) ? 1 : 3; 
})

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -238,6 +238,14 @@ struct mem_event_per_domain
>      struct mem_event_domain access;
>  };
>  
> +/*
> + * PVH is a PV guest running in an HVM container. While is_hvm_* checks are
> + * false for it, it uses many of the HVM data structs.
> + */
> +enum guest_type {
> +    is_pv, is_pvh, is_hvm

Pretty odd names for enumerators - it's more conventional for them
to have a prefix identifying their enumeration type in some way.

> @@ -732,8 +743,12 @@ void watchdog_domain_destroy(struct domain *d);
>  
>  #define VM_ASSIST(_d,_t) (test_bit((_t), &(_d)->vm_assist))
>  
> -#define is_hvm_domain(d) ((d)->is_hvm)
> +#define is_pv_domain(d) ((d)->guest_type == is_pv)
> +#define is_pv_vcpu(v)   (is_pv_domain(v->domain))

Even if the pre-existing is_hvm_vcpu() gives a bad example -
please properly parenthesize macro parameters.

> +#define is_hvm_domain(d) ((d)->guest_type == is_hvm)
>  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
> +#define is_pvh_domain(d) ((d)->guest_type == is_pvh)
> +#define is_pvh_vcpu(v)   (is_pvh_domain(v->domain))

Same here.

Jan

>  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>                             cpumask_weight((v)->cpu_affinity) == 1)
>  #ifdef HAS_PASSTHROUGH



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