Sheng Yang
2010-Mar-01 09:43 UTC
[Xen-devel] [PATCH][v4] PV extension of HVM(hybrid) support in Xen
Hi Keir Here is the latest (hybrid) patchset. Change from v3: 1. Minor polish the patchset. Replace several specific is_hvm_pv_evtchn_domain() judgement. 2. Name changed... Change from v2: 1. Change the name "hybrid" to "PV featured HVM", as well as flag and macro names. 2. Merge the hypercall page with HVM. 3. Clean up VIRQ delivery mechanism, fixing the lock issue. 4. Remove the reserved the region in E820(but another issue remains, I can''t get location of grant table elegantly, as I described in Linux part patches) Please review, thanks! -- regards Yang, Sheng _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Mar-01 10:00 UTC
[Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
If the Linux guys are happy with the interface then this looks fine to me and it can go in as soon as 4.0 is out the door. -- Keir On 01/03/2010 09:43, "Sheng Yang" <sheng@linux.intel.com> wrote:> Hi Keir > > Here is the latest (hybrid) patchset. > > Change from v3: > > 1. Minor polish the patchset. Replace several specific > is_hvm_pv_evtchn_domain() judgement. > 2. Name changed... > > Change from v2: > > 1. Change the name "hybrid" to "PV featured HVM", as well as flag and macro > names. > 2. Merge the hypercall page with HVM. > 3. Clean up VIRQ delivery mechanism, fixing the lock issue. > 4. Remove the reserved the region in E820(but another issue remains, I can''t > get location of grant table elegantly, as I described in Linux part patches) > > Please review, thanks!_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Mar-01 10:21 UTC
[Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
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?> + 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.> + 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?> + d->hvm_pv_enabled |= XEN_HVM_PV_EVTCHN_ENABLED; > + printk("HVM: PV featured evtchn enabled\n");Please remove your debugging printks.> + } > +param_fail5: > + rcu_unlock_domain(d); > + break; > + } > + > default: > { > gdprintk(XENLOG_WARNING, "Bad HVM op %ld.\n", op);Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Mar-01 10:52 UTC
Re: [Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
On Mon, 1 Mar 2010, Keir Fraser wrote:> If the Linux guys are happy with the interface then this looks fine to me > and it can go in as soon as 4.0 is out the door. >Could you please wait for a week or two before applying this patch series? I am working on a different approach on interrupt remapping, basically a replacement for patch 5, it also requires some modifications on the xen side. The basic idea is that from the kernel perspective asking event channel delivery for real physical interrupts or event channel delivery for emulated interrupts should be same thing. The code to do the former is basically xen_setup_pirqs() in the pvops tree. I got it to work for the first time a couple of days ago but it still needs to be polished before I can send it to the list. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Sheng Yang
2010-Mar-01 11:40 UTC
[Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
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. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Mar-02 01:49 UTC
Re: [Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
On 03/01/2010 03:40 AM, Sheng Yang wrote:> 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... > >[...]>> 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... >Could you expand on this? Do you mean in the pvops Xen time code? What do you mean? J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Sheng Yang
2010-Mar-02 03:36 UTC
Re: [Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
On Tuesday 02 March 2010 09:49:55 Jeremy Fitzhardinge wrote:> On 03/01/2010 03:40 AM, Sheng Yang wrote: > > 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... > > [...] > > >> 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... > > Could you expand on this? Do you mean in the pvops Xen time code? What > do you mean? >The function pvclock_get_nsec_offset() in Linux kernel have this:>static u64 pvclock_get_nsec_offset(struct pvclock_shadow_time *shadow) >{ > u64 delta = native_read_tsc() - shadow->tsc_timestamp; > return scale_delta(delta, shadow->tsc_to_nsec_mul, shadow- >tsc_shift); >}tsc_timestamp take the vcpu beginning at 0, so that''s the assumption. So I adjusted guest TSC(the return value of native_read_tsc()) to satisfy this assumption. -- regards Yang, Sheng _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Mar-02 04:39 UTC
Re: [Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
On 03/01/2010 07:36 PM, Sheng Yang wrote:>> static u64 pvclock_get_nsec_offset(struct pvclock_shadow_time *shadow) >> { >> u64 delta = native_read_tsc() - shadow->tsc_timestamp; >> return scale_delta(delta, shadow->tsc_to_nsec_mul, shadow- >> tsc_shift); >> } >> > tsc_timestamp take the vcpu beginning at 0, so that''s the assumption.Why would it be 0? Xen sets tsc_timestamp to the current tsc when it updates the time parameters, which is whenever the vcpu is scheduled on a pcpu (and other times). There''s no expectation that the tsc starts from 0, since that won''t ever be the case. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Sheng Yang
2010-Mar-02 05:04 UTC
Re: [Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
On Tuesday 02 March 2010 12:39:34 Jeremy Fitzhardinge wrote:> On 03/01/2010 07:36 PM, Sheng Yang wrote: > >> static u64 pvclock_get_nsec_offset(struct pvclock_shadow_time *shadow) > >> { > >> u64 delta = native_read_tsc() - shadow->tsc_timestamp; > >> return scale_delta(delta, shadow->tsc_to_nsec_mul, shadow- > >> tsc_shift); > >> } > > > > tsc_timestamp take the vcpu beginning at 0, so that''s the assumption. > > Why would it be 0? Xen sets tsc_timestamp to the current tsc when it > updates the time parameters, which is whenever the vcpu is scheduled on > a pcpu (and other times). There''s no expectation that the tsc starts > from 0, since that won''t ever be the case. >Sorry, I misunderstood. HVM assume it start from 0... PV is following the native. We set the offset to 0, so that PV tsc is the same as native. -- regards Yang, Sheng _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Mar-02 05:21 UTC
Re: [Xen-devel] Re: [PATCH][v4] PV extension of HVM(hybrid) support in Xen
On 03/01/2010 09:04 PM, Sheng Yang wrote:> Sorry, I misunderstood. HVM assume it start from 0... PV is following the > native. We set the offset to 0, so that PV tsc is the same as native. >OK, I see. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel