Re: [PATCH 5/7] xen: Make event channel work with PV featured HVM

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

On Tuesday 09 February 2010 19:52:56 Ian Campbell wrote:
> On Mon, 2010-02-08 at 08:05 +0000, Sheng Yang wrote:
> > +       if (xen_hvm_pv_evtchn_enabled()) {
> > +               if (enable_hvm_pv(HVM_PV_EVTCHN))
> > +                       return -EINVAL;
> > +[...]
> > +               callback_via =
> > HVM_CALLBACK_VECTOR(X86_PLATFORM_IPI_VECTOR); +              
> > set_callback_via(callback_via);
> > +
> > +               x86_platform_ipi_callback = do_hvm_pv_evtchn_intr;
> 
> Why this indirection via X86_PLATFORM_IPI_VECTOR?
> 
> Apart from that why not use CALLBACKOP_register subop CALLBACKTYPE_event
> pointing to xen_hypervisor_callback the same as a full PV guest?
> 
> This would remove all the evtchn related code from HVMOP_enable_pv which
> I think should be eventually unnecessary as an independent hypercall
> since all HVM guests should simply be PV capable by default -- the
> hypervisor only needs to track if the guest has made use of specific PV
> functionality, not the umbrella "is PV" state.

The reason is the bounce frame buffer implemented by PV guest to inject a 
event is too complex here... Basically you need to setup a stack like hardware 
would do, and return to the certain guest CS:IP to handle this. And you need 
to take care of every case, e.g. guest in the ring0 or ring3, guest in the 
interrupt context or not, and the recursion of the handler, and so on. 
Hardware can easily handle all these elegantly, you just need to inject a 
vector through hardware provided method. That's much easily and elegant. Take 
the advantage of hardware is still a part of our target. :)

And even with CALLBACKOP_register, I think the change in hypervisor is needed. 
And I think the updated approach is near to your idea, and  I am totally agree 
that a functionality based enabling is better than a big umbrella. Now you can 
see, a generic enabling is discard, and current the enabling is in feature 
branch enabling, one at a time(though there is only one now...). The message 
for the evtchn enabling of HVM hypercall transfered is, the guest won't use 
IOAPIC/LAPIC now, it would purely use evtchn; so hypervisor indeed need change 
to continue to service the guest. The "umbrella 'is PV'" isn't in usage now(in 
fact I still put a flag there because I was wandering if I can optimize PV 
halt if hypervisor is aware of it, but maybe it's too early to consider this 
now..). The only three positions in Xen patch mentioned hvm_pv, are for 
evtchn. So the meaning here is: HVM can be PV featured, but sometime(e.g. for 
evtchn) hypervisor need aware of the state of guest, so we have judgment for 
evtchn, as well as clear the hardware TSC offset in preparing for the PV timer 
which would come along with evtchn. That is something we have to do. And we 
don't have the similar thing for halt now, because hypervisor don't need to 
know about it. We only let hypervisor know when it's necessary.

I would remove XEN_HVM_PV_ENABLED flags in the xen patchset, if it remind you 
of the umbrella of "HVM PV" features.

-- 
regards
Yang, Sheng