From mboxrd@z Thu Jan  1 00:00:00 1970
From: Sheng Yang <sheng@linux.intel.com>
Subject: Re: [PATCH][v4] PV extension of HVM(hybrid) support in
	Xen
Date: Mon, 1 Mar 2010 19:40:50 +0800
Message-ID: <201003011940.50685.sheng@linux.intel.com>
References: <201003011743.41341.sheng@linux.intel.com>
	<20100301102127.GA17243@whitby.uk.xensource.com>
Mime-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <20100301102127.GA17243@whitby.uk.xensource.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Tim Deegan <Tim.Deegan@citrix.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>, Ian Pratt <Ian.Pratt@eu.citrix.com>, xen-devel <xen-devel@lists.xensource.com>, Keir Fraser <Keir.Fraser@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

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.
>