Re: [Xen-devel] [PATCH 08/17] [V3]PVH xen: domain creation code changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.04.13 at 03:02, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> @@ -859,19 +868,26 @@ int arch_set_info_guest(
>  
>          if ( !cr3_page )
>          {
> -            destroy_gdt(v);
> +            if ( !is_pvh_vcpu(v) )
> +                destroy_gdt(v);

Can't the check rather be done in destroy_gdt(), not the least
because the pattern here repeats further down?

> @@ -938,6 +955,13 @@ int arch_set_info_guest(
>  
>      update_cr3(v);
>  
> +    if ( is_pvh_vcpu(v) )

Can you get here for a PVH vCPU? The update_cr3() right before
that is suspicious - you shouldn't need that for PVH.

> @@ -968,16 +992,21 @@ void arch_vcpu_reset(struct vcpu *v)
>  static void
>  unmap_vcpu_info(struct vcpu *v)
>  {
> -    unsigned long mfn;
> +    unsigned long mfn, *mfnp;
> +
> +    if ( is_pvh_vcpu(v) )
> +        mfnp = &v->arch.hvm_vcpu.hvm_pvh.vcpu_info_mfn;
> +    else
> +        mfnp = &v->arch.pv_vcpu.vcpu_info_mfn;

This suggests you want to pull out the vcpu_info_mfn field, at
once also making it available for future use in HVM guests.

> @@ -639,7 +639,8 @@ static void hap_update_cr3(struct vcpu *v, int do_locking)
>  const struct paging_mode *
>  hap_paging_get_mode(struct vcpu *v)
>  {
> -    return !hvm_paging_enabled(v)   ? &hap_paging_real_mode :
> +    return is_pvh_vcpu(v) ? &hap_paging_long_mode :
> +        !hvm_paging_enabled(v)   ? &hap_paging_real_mode :
>          hvm_long_mode_enabled(v) ? &hap_paging_long_mode :
>          hvm_pae_enabled(v)       ? &hap_paging_pae_mode  :
>                                     &hap_paging_protected_mode;

In the series description you say that only 32-bit kernel support is
missing, yet this doesn't look right for a 32-bit PVH guest.

> @@ -323,6 +328,19 @@ static inline unsigned long 
> hvm_get_shadow_gs_base(struct vcpu *v)
>      return hvm_funcs.get_shadow_gs_base(v);
>  }
>  
> +static inline int hvm_pvh_set_vcpu_info(struct vcpu *v, 
> +                                        struct vcpu_guest_context *ctxtp)
> +{
> +    return hvm_funcs.pvh_set_vcpu_info(v, ctxtp);
> +}
> +
> +static inline int hvm_pvh_read_descriptor(unsigned int sel, 
> +               const struct vcpu *v, const struct cpu_user_regs *regs, 
> +               unsigned long *base, unsigned long *limit, unsigned int *ar)
> +{
> +    return hvm_funcs.pvh_read_descriptor(sel, v, regs, base, limit, ar);
> +}

So you nicely dropped the hvm_ prefix from the structure field
names, but kept the redundant prefixes on the functions' ones?
Pretty odd, and confusing now that HVM != PVH (!= PV).

> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -104,6 +104,13 @@ struct nestedvcpu {
>  
>  #define vcpu_nestedhvm(v) ((v)->arch.hvm_vcpu.nvcpu)
>  
> +/* add any PVH specific fields here */
> +struct pvh_hvm_vcpu_ext
> +{
> +    /* Guest-specified relocation of vcpu_info. */
> +    unsigned long vcpu_info_mfn;
> +};
> +
>  struct hvm_vcpu {
>      /* Guest control-register and EFER values, just as the guest sees them. 
> */
>      unsigned long       guest_cr[5];
> @@ -170,6 +177,8 @@ struct hvm_vcpu {
>      struct hvm_trap     inject_trap;
>  
>      struct viridian_vcpu viridian;
> +
> +    struct pvh_hvm_vcpu_ext hvm_pvh;

Same here - hvm_pvh_ (or equally pvh_hvm_) just make no sense.

Also, as you add this to hvm_vcpu and iirc you only dropped
the union with the PV side for arch_domain - are you not using
_any_ field in pv_vcpu?

Jan


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