Re: [Xen-devel] [PATCH] pvh: add a call to pit_init to init spinlock

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Mon, 10 Mar 2014 09:10:23 +0000
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 08.03.14 at 03:28, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1299,6 +1299,9 @@ int hvm_vcpu_initialise(struct vcpu *v)
> >          v->arch.hvm_vcpu.hcall_64bit = 1;    /* PVH 32bitfixme. */
> >          /* This is for hvm_long_mode_enabled(v). */
> >          v->arch.hvm_vcpu.guest_efer = EFER_SCE | EFER_LMA |
> > EFER_LME; +
> > +        if ( v->vcpu_id == 0 )
> > +            pit_init(v, cpu_khz);
> 
> I know I'm asking a lot, but did you consider using the opportunity
> to do what the HVM call site says - move this to
> hvm_domain_initialise()? Or, considering that PV wants this too,
> even move it to arch_domain_create()?

I started that route.. but thought it might be too intrusive for 4.4.
I had it moved to hvm_domain_initialise, but arch_domain_create is 
even better. It's hard to anticipate what any maintainer would prefer, 
the impression so far is to not touch anything un-needed or un-necessary... 


> > --- a/xen/arch/x86/hvm/i8254.c
> > +++ b/xen/arch/x86/hvm/i8254.c
> > @@ -454,6 +454,9 @@ void pit_init(struct vcpu *v, unsigned long
> > cpu_khz) 
> >      spin_lock_init(&pit->lock);
> >  
> > +    if ( is_pvh_vcpu(v) )
> > +        return;
> > +
> >      if ( is_hvm_domain(d) )
> >      {
> >          register_portio_handler(d, PIT_BASE, 4, handle_pit_io);
> 
> So I'm confused: Both HVM and PV want the function executed to
> its end. Why would PVH bail early? The more that I don't think
> pit_reset() would do any harm to PVH. And considering the I/O
> model, I think the ultimate goal would need to be to switch above
> is_hvm_domain() to has_hvm_container_domain(). If that doesn't
> work right now, it should probably get another "pvh fixme"
> attached.

Mostly to defer it to come back and spend time to look at pv and hvm
ways, and figure which would be the best way to do it. But looking
at the code again, pit_reset should also work right now for pvh. 

I also see pit_deinit is not called for pv. anyways, please see patch v1.

thanks
mukesh


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