[Xen-devel] Re: [PATCH][v7] PV extension of HVM(hybrid) support in Xen

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

Once again: please sort this out between yourself and Stefano so we only
have one patchset doing this feature.

At 07:21 +0000 on 08 Mar (1268032899), Sheng Yang wrote:
Content-Description: hybrid-xen.patch
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -686,7 +686,16 @@
>  
>      if ( is_hvm_vcpu(v) )
>      {
> +        unsigned long eip, cs;
> +
>          hvm_set_info_guest(v);
> +
> +        eip = c(user_regs.eip);
> +        if (eip != 0) {
> +            cs = eip >> 12 << 8;
> +            hvm_vcpu_reset_state(v, cs, 0);
> +            hvm_funcs.set_tsc_offset(v, 0);

Shouldn't this be gated on (d->hvm_pv_enabled & XEN_HVM_PV_CLOCK_ENABLED)
rather than (eip != 0)?

> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -686,6 +686,7 @@
>      struct domain *d = current->domain;
>      /* Optionally shift out of the way of Viridian architectural leaves. */
>      uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
> +    unsigned int tmp_eax, tmp_ebx, tmp_ecx, tmp_edx;
>  
>      idx -= base;
>      if ( idx > 3 ) 
> @@ -716,6 +717,14 @@
>          *edx = 0;          /* Features 2 */
>          if ( !is_hvm_vcpu(current) )
>              *ecx |= XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD;
> +
> +        /* Check if additional feature specified, e.g. Hybrid */
> +        if ( !is_viridian_domain(d) ) {
> +            domain_cpuid(d, 0x40000002, 0,
> +                         &tmp_eax, &tmp_ebx, &tmp_ecx, &tmp_edx);
> +            if (tmp_edx != 0)
> +                *edx = tmp_edx & XEN_CPUID_FEAT2_MASK;
> +        }

Maybe use cpuid_edx() here?

> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -127,6 +127,15 @@
>  typedef struct xen_hvm_set_mem_type xen_hvm_set_mem_type_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_type_t);
>  
> +/* Enable PV extended HVM mode. Should called by BSP */

This comment doesn't really explain what the hypercall does.

> +#define HVMOP_enable_pv 9
> +struct xen_hvm_pv_type {
> +    /* Should be DOMID_SELF so far */
> +    domid_t domid;

Please just kill this field rather than requiring the caller to set it
to DOMID_SELF and then checking that he did it.

> +    /* The features want to enable */
> +    uint32_t flags;
> +#define HVM_PV_CLOCK  (1ull<<0)
> +};
>  
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  


-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel