Re: [Xen-devel][PATCH][RFC] Supporting Enlightened Windows 2008 Server

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 07:28 +0000 on 06 Mar (1204788507), Keir Fraser wrote:
> Personally I think the approach is ugly, and also you have not yet presented
> evidence that supporting the Viridian paravirtualisations improves
> performance. If it doesn't then it's a waste of time; if it does then it
> raises the question of which hypercalls provide the benefit, and do we get a
> smaller neater patch by supporting just those? One final comment is that the
> TLB management code that this slaps on top of the core hypervisor looks a
> bit scary to me. Tim Deegan may care to comment more on that.

Some blame lies with the mismatch between the Viridian interface and
Xen's; there needs to be a way for the TLB flush hypercall to block
indefinitely.  But I can't see how that turns into more than an atomic_t
for TlbFlushInhibit and a block-and-schedule operation.  In the current
patches, there's quite a lot of locking and ownership going on as well.
I'm confused by the use of wait_on_xen_event_channel(0, xyz); event
channels don't seem to come into it.

I'll mention now, since I have the patch in front of me, that I dislike
the addition of an "ext_id" field to the HVM save format header and
associated special treatment in the save/restore code; you should be
able to figure out that this is a w2k8 domain from the presence of your
other records in the save file.



More generally, I agree that the approach is very heavyweight.  I don't
see the need for a framework here, since there's no other proposed user
of it that would want the same interface.  It seems to duplicate a lot
of things (does it really need its own spinlock implementation?)

It's certainly not in Xen coding style, even in the framework
implementation.  (The MS habit of encoding scope and type information in
variable names annoys the heck out of me.  Why does a lock field in an
nsPartition_t need to be called "nsLock"?)

The naming in general could do with a kicking -- calling everything
"Novell Shim" is understandable for historical reasons but not really
descriptive of its function.   But maybe that can wait.

Tim

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel