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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home