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

[Sheng Yang <sheng@linux.intel.com>]

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__) */
>