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

["Ky Srinivasan" <ksrinivasan@xxxxxxxxxx>]

>>> On Thu, Mar 6, 2008 at  5:15 AM, in message
<20080306101542.GA22422@xxxxxxxxxxxxxxxxxxxxx>, Tim Deegan
<Tim.Deegan@xxxxxxxxxx> wrote: 
> 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.

The Veridian API allows the guest to pass in a variable list of arguments to 
the TLB flush call ( HvFlushVirtualAddressList). Furthermore, both forms of the 
flush APIs (HvFlushVirtualAddressSpace and HvFlushVirtualAdressList) can 
specify a list of vcpus that should be involved in the flush process. So, as 
you have noted we will need a mechanism to co-ordinate the flush operation 
amongst the set of vcpus involved which means we need to be able give up the 
physical CPU in the hypervisor waiting for the flush to complete. I have used 
wait_on_xen_event_channel() to implement this synchronization. Since we don't 
preserve the stack state when we block in the hypervisor, I have used a 
seperate per-vcpu page for dealing with hypercall input parameters for calls 
that can potentially block in the hypervisor. From what I have seen, win2k8 
server mostly specifies  all the processors in ProcessorMask. So, I chose to 
implement TLB flush operations using a single serialization object that keeps 
track of both the set of vcpus involved in the flush operation as well as the 
list of pages to be flushed.

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

I can fix this.

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

I agree that there is no need to isolate the shim's dependence on the base Xen 
code (xen_call_vector_t). I implemented this shim a year ago and at that point 
it was not clear what Microsoft might do with the Veridian specification.  So, 
clearly some of the design choices that I made a year ago may not be the right 
choice today. However, I still think that having an intercept framework where  
one can implement Veridian specific functionality without cluttering up the 
base Xen code is still the right approach. 
    
> It seems to duplicate a lot
> of things (does it really need its own spinlock implementation?)

Clearly not! As I noted in an earlier email to Kier, I will be the first to 
admit that these patches require significant cleanup and I am willing to clean 
them up. A lot of what you see has historical baggage and I wanted to get some 
feedback before I invested the time to clean things up.

> 
> 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"?)

Agreed. 

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

Regards,

K. Y
> 
> Tim



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