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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.07.13 at 04:32, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> Chagnes in V8:

Same spelling typo in all of the change title lines...

>   - Got to VMCS for DPL check instead of checking the rpl in
>     guest_kernel_mode.  Note, we drop the const qualifier from
>     vcpu_show_registers() to accomodate the hvm function call in
>     guest_kernel_mode().
>   - Also, hvm_kernel_mode is put in hvm.c because it's called from
>     guest_kernel_mode in regs.h which is a pretty early header include.
>     Hence, we can't place it in hvm.h like other similar functions.

Are you saying that because you tried it, or just because it looks
like so? The use of the function is in a macro, and hence if the
macro isn't used too early this could still work out. I say this
because the function would clearly benefit from getting inlined.

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

Sorry for having overlooked this earlier - this really ought to
assert for is_pv_domain(), if any assertion is to be added here
at all.

> --- a/xen/include/asm-x86/x86_64/regs.h
> +++ b/xen/include/asm-x86/x86_64/regs.h
> @@ -10,10 +10,13 @@
>  #define ring_2(r)    (((r)->cs & 3) == 2)
>  #define ring_3(r)    (((r)->cs & 3) == 3)
>  
> -#define guest_kernel_mode(v, r)                                 \
> -    (!is_pv_32bit_vcpu(v) ?                                     \
> -     (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) :        \
> -     (ring_1(r)))
> +bool_t hvm_kernel_mode(struct vcpu *);
> +
> +#define guest_kernel_mode(v, r)                                   \
> +    (is_pvh_vcpu(v) ? (hvm_kernel_mode(v)) :                      \
> +     (!is_pv_32bit_vcpu(v) ?                                      \
> +      (ring_3(r) && ((v)->arch.flags & TF_kernel_mode)) :         \
> +      (ring_1(r))))

While I see that you attempt to follow the style used here before,
it has been inconsistent and shouldn't be made worse: Please drop
the extra parentheses around function calls / function-like macro
invocations.

Jan


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