Sheng Yang
2010-Mar-08 07:21 UTC
[Xen-devel] [PATCH][v7] PV extension of HVM(hybrid) support in Xen
Hi Keir Here is the v7 of hybrid patch in Xen. Change from v6: 1. Minor comments fix. 2. Make PV clocksource to be a feature can be controlled by CPUID. Change from v5: Address the comments from Tim. Change from v4: 1. Add support for PV clocksource on HVM. (Replace evtchn enabling with pv clock enabling). 2. Update the patch following Tim''s comments. Change from v3: 1. Minor polish the patchset. Replace several specific is_hvm_pv_evtchn_domain() judgement. 2. Name changed... Change from v2: 1. Change the name "hybrid" to "PV featured HVM", as well as flag and macro names. -- regards Yang, Sheng _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Mar-10 14:44 UTC
[Xen-devel] Re: [PATCH][v7] PV extension of HVM(hybrid) support in Xen
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@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Sheng Yang
2010-Mar-11 02:11 UTC
[Xen-devel] Re: [PATCH][v7] PV extension of HVM(hybrid) support in Xen
On Wednesday 10 March 2010 22:44:46 Tim Deegan wrote:> Once again: please sort this out between yourself and Stefano so we only > have one patchset doing this feature.I would work with Stefano on PV evtchn for HVM. And the PV clocksource would be a standalone 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)?Um... I think no other HVM should call this, and evtchn shouldn''t work without PV clocksource... And it have two meaning here: one is setting up the start up IP for AP, another is initial PV clock. I would update this to get it more clear.> > > 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?Um? It''s not native cpuid, but the one configuration file specific.> > > 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.OK. -- regards Yang, Sheng> > > + /* The features want to enable */ > > + uint32_t flags; > > +#define HVM_PV_CLOCK (1ull<<0) > > +}; > > > > #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Mar-11 12:02 UTC
[Xen-devel] Re: [PATCH][v7] PV extension of HVM(hybrid) support in Xen
At 02:11 +0000 on 11 Mar (1268273506), Sheng Yang wrote:> > Shouldn''t this be gated on (d->hvm_pv_enabled & XEN_HVM_PV_CLOCK_ENABLED) > > rather than (eip != 0)? > > Um... I think no other HVM should call this, and evtchn shouldn''t work > without PV clocksource... And it have two meaning here: one is setting > up the start up IP for AP, another is initial PV clock. I would > update this to get it more clear.Yes, I was just talking about the last line, where you set the offset to zero. Since you have introduced a flag that says whether the clocksource is enabled, you should use it.> > Maybe use cpuid_edx() here? > > Um? It''s not native cpuid, but the one configuration file specific.Yes, you''re quite right. I misread the code. Thanks, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel