Re: [Xen-devel] [RFC PATCH 6/16]: PVH xen: Define pvh guest and header changes..

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.01.13 at 02:51, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> +/* We need vcpu because during context switch, going from pure PV to PVH,
> + * in save_segments(), current has been updated to next, and no longer 
> pointing
> + * to the pure PV. BTW, for PVH, we update regs->selectors on each vmexit */
>  #define read_segment_register(vcpu, regs, name)                 \

I can only hope that at the end of this patch set the comment
matches reality - at this point in the series it doesn't afaict.

> --- a/xen/include/asm-x86/x86_64/regs.h       Fri Jan 11 16:25:27 2013 -0800
> +++ b/xen/include/asm-x86/x86_64/regs.h       Fri Jan 11 16:27:46 2013 -0800
> @@ -11,9 +11,10 @@
>  #define ring_3(r)    (((r)->cs & 3) == 3)
>  
>  #define guest_kernel_mode(v, r)                                 \
> +   ( is_pvh_vcpu(v) ? ({BUG_ON(v!=current); ring_0(r);}) :  \

If this BUG_ON() really has to stay here, you ought to add
white space inside the braces and around the !=.

>      (!is_pv_32bit_vcpu(v) ?                                     \
>       (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) :        \
> -     (ring_1(r)))
> +     (ring_1(r))) )

As you add a level of parentheses, you also ought to adjust
indentation.

> --- a/xen/include/xen/lib.h   Fri Jan 11 16:25:27 2013 -0800
> +++ b/xen/include/xen/lib.h   Fri Jan 11 16:27:46 2013 -0800
> @@ -45,6 +45,14 @@ do {                                    
>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
>  #endif
>  
> +/* While PVH feature is experimental, make it an unconditional assert */
> +#define PVH_ASSERT(p)              \
> +    do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
> +#define NO_PVH_ASSERT_VCPU(v)         \
> +    do { if (is_pvh_vcpu(v)) PVH_ASSERT(0); } while (0)
> +#define NO_PVH_ASSERT_DOMAIN(d)       \
> +    do { if (is_pvh_domain(d)) PVH_ASSERT(0); } while (0)

What's this?

At the very least, you want e.g.

+    do { PVH_ASSERT(!is_pvh_vcpu(v)); } while (0)

for the printed string to be meaningful (and then you can also
drop the do/while).

But the defines, if needed at all, are grossly misplaced in any case;
there ought to be a pvh header for such stuff.

> @@ -278,6 +281,7 @@ struct domain
>  
>      /* Is this an HVM guest? */
>      bool_t           is_hvm;
> +    bool_t           is_pvh;      /* see above for description */

These are mutually exclusive (also with PV), so perhaps better
to have a single enum-type variable?

Jan

>  #ifdef HAS_PASSTHROUGH
>      /* Does this guest need iommu mappings? */
>      bool_t           need_iommu;



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