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

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

On Monday 01 March 2010 18:21:27 Tim Deegan wrote:
> At 09:43 +0000 on 01 Mar (1267436621), Sheng Yang wrote:
> > @@ -3109,6 +3117,36 @@
> >          break;
> >      }
> >
> > +    case HVMOP_enable_pv: {
> 
> Why does this have to be explicitly enabled?  Can't you just notice that
> a domain is using the evtchnop hypercalls?

The issue is pv timer. It assumed the tsc start from 0, which is different 
from HVM. So I'd like to give it a explicit call here. Otherwise it can be 
hooked in evtchn binding, but I don't think that's clear...
 
> > +        struct xen_hvm_pv_type a;
> > +        struct domain *d;
> > +
> > +        if ( copy_from_guest(&a, arg, 1) )
> > +            return -EFAULT;
> > +
> > +        rc = rcu_lock_target_domain_by_id(a.domid, &d);
> 
> Domains do this to each other?  It looks like it has surprising
>  side-effects.

Should not allowed... I think a.domid should always be the current domain. 
Replace it with DOMID_SELF?
 
> > +        if ( rc != 0 )
> > +            return rc;
> > +
> > +        rc = -EINVAL;
> > +        if ( !is_hvm_domain(d) )
> > +            goto param_fail5;
> > +
> > +        rc = xsm_hvm_param(d, op);
> > +        if ( rc )
> > +            goto param_fail5;
> > +
> > +        if (a.flags & HVM_PV_EVTCHN) {
> > +            update_domain_wallclock_time(d);
> > +            hvm_funcs.set_tsc_offset(d->vcpu[0], 0);
> 
> Only vcpu 0?  Doesn't this do horrible things to timekeeping in the guest?

The other vcpus are initialized when it is brought up. TSC started from 0 is a 
fundamental assumption for pv clock in Linux... 
> 
> > +            d->hvm_pv_enabled |= XEN_HVM_PV_EVTCHN_ENABLED;
> > +            printk("HVM: PV featured evtchn enabled\n");
> 
> Please remove your debugging printks.

OK...

-- 
regards
Yang, Sheng

> 
> > +        }
> > +param_fail5:
> > +        rcu_unlock_domain(d);
> > +        break;
> > +    }
> > +
> >      default:
> >      {
> >          gdprintk(XENLOG_WARNING, "Bad HVM op %ld.\n", op);
> 
> Tim.
>