Re: [Xen-devel] [PATCH v3 00/12] Alternate p2m: support multiple copies of host p2m

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 07/09/2015 12:49 PM, Wei Liu wrote:
> On Wed, Jul 08, 2015 at 06:35:33PM +0000, Sahita, Ravi wrote:
>> Hi Wei,
>>
>> Given where we stand (close) on the altp2m patch series - we would like to 
>> request an extension of about a week to complete the last couple of patches 
>> in the series for inclusion in 4.6. 
>> Some of the suggestions may have implications on other patches and on our 
>> tests hence asking for the extension (we know what we need to change). 
>>
>> Hope that is acceptable. This is a quick current status snapshot of v3: 
>> (this doesn't have a couple tools patches that Tamas has contributed but 
>> those will be included in rev4)
>>
>> altp2m series patch v3 status
>> 0    [PATCH v3 00/12] Alternate p2m: support multiple copies of host p2m     
>>                         Sign-off        Reviewed                Acked        
>>            Status
>> -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>> 1    [Xen-devel] [PATCH v3 01/13] common/domain: Helpers to pause a domain 
>> while in context  Andrew Cooper                                           
>> good?
>> 2    [Xen-devel] [PATCH v3 02/13] VMX: VMFUNC and #VE definitions and 
>> detection.             Ed White        Andrew Cooper   Jun Nakajima          
>>   good
>> 3    [Xen-devel] [PATCH v3 03/13] VMX: implement suppress #VE.               
>>                         Ed White        Andrew Cooper   Jun Nakajima         
>>    good
>> 4    [Xen-devel] [PATCH v3 04/13] x86/HVM: Hardware alternate p2m support 
>> detection          Ed White        Andrew Cooper                           
>> good?
>> 5    [Xen-devel] [PATCH v3 05/13] x86/altp2m: basic data structures and 
>> support routines             Ed White        Andrew Cooper                   
>>         Locking being reviewed by George Dunlap
>> 6    [Xen-devel] [PATCH v3 06/13] VMX/altp2m: add code to support EPTP 
>> switching and #VE     Ed White        Andrew Cooper   Jun Nakajima           
>>  good
>> 7    [Xen-devel] [PATCH v3 07/13] VMX: add VMFUNC leaf 0 (EPTP switching) to 
>> emulator                Ravi Sahita     ***                                  
>>            ***Proposed Changes ready staged for v4
>> 8    [Xen-devel] [PATCH v3 08/13] x86/altp2m: add control of suppress_ve     
>>                         Ed White        ***Andrew Cooper                     
>>            *** George Dunlap has an alt patch based on v2 7 of 12
>> 9    [Xen-devel] [PATCH v3 09/13] x86/altp2m: alternate p2m memory events    
>>                 Ed White        Andrew Cooper   George Dunlap           good
>> 10   [Xen-devel] [PATCH v3 10/13] x86/altp2m: add remaining support routines 
>>                 Ed White        Andew Cooper                                 
>>    good?
>> 11   [Xen-devel] [PATCH v3 11/13] x86/altp2m: define and implement alternate 
>> p2m HVMOP type  Ed White        ***                                          
>>    *** Need to create HVMOP sub-types for altp2m
>> 12   [Xen-devel] [PATCH v3 12/13] x86/altp2m: Add altp2mhvm HVM domain 
>> parameter             Ed White        Andew Cooper                           
>>          Wei's comments addressed and staged for v4
>> 13   [Xen-devel] [PATCH v3 13/13] x86/altp2m: XSM hooks for altp2m HVM ops   
>>                 Ravi Sahita     Daniel De Graaf         Daniel De Graaf      
>>    *** Will be impacted by HVMOP changes
>>
> 
> Hi Ravi
> 
> Thanks for the email. Let me elaborate on this.
> 
> First thing, you asked for an freeze extension, but as I understand it
> you meant a freeze exception. I will reply on this basis.
> 
> Overall the series is of very high quality and very useful feature,
> personally I would very much like this feature to be accepted in 4.6,
> but I did set out criteria for granting a freeze exception [0]. The
> series in question needs to be in a state that's only pending a few
> tweaks and publicly endorsed by maintainers, plus other case by case
> consideration.
> 
> With my limited knowledge of hypervisor side code I can't tell if the
> series is very close. But some very long sub-threads prompt me into
> thinking there are still issues (big or small).
> 
> As far as I can tell, there are several issues here with regard to this
> patch series on hypervisor side:
> 
> 1. Patch #1 is neither acked nor reviewed.
> 
> 2. Patch #5, George is looking at locking scheme. Although that patch
>    is already reviewed by Andrew and locking scheme backed by Tim, I
>    would like a formal ack from George as current P2M maintainer.

I expect that the locking is probably fine, but it definitely needs to
be a comment in the code explaining the codepaths where the order
becomes important, and why we need this 3-level "sandwiched" locking thing.

> Question for hypervisor maintainers, how much risk do you think this
> series impose?  If this feature is off, is the code path more or less
> the same of before?  It seems to touch core functionality (P2M). Note
> that any bug in that area would block the release.

So there's no reason that it should be high risk; in theory the feature
is off by default, and if it's off, the only changes to existing paths
should consist of
1. Checking to see if it's enabled, and when it finds it's not, skipping
all the new code
2. Going about setting bit 63 in all the ept entries, and dealing with
the fact that it's probably going to be set now.

Neither of those are very high risk; and the second path will be tested
millions of times in a single vm-boot-shutdown smoke test.

However, the series as it stands has more impact on existing paths than
it needs to.  It seems that a bunch of structures are pro-actively
initiated and allocated for each domain created, even if the entire
feature is disabled at boot.  Additionally, a lot of the set-up and
tear-down operations could be completely skipped if
d->arch.altp2m_active == 0.

In the interest of making this easier to accept during a code freeze, I
suggest going through the code again with an eye to lowering the impact.
 I don't think you need to spend a great deal of effort or do any major
refactorings; but there should be a lot of "low hanging fruit" where you
can just say "if (!d->arch.altp2m_active) return;".

Additionally, at very least you should gate allocating and initializing
altp2m tables and eptp on hvm_funcs.altp2m_supported; better yet, gate
it on it having been enabled for the domain on create.

The second would mean, I guess, calling the initialization stuff when
the parameter is set; that looks to be in like with what nestedhvm does.

Use your best judgement to find the balance between limiting the impact
and spending more time changing the code.

Note also that I'll be working tomorrow, doing code review, but that
I'll be out of the office Monday and Friday next week.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel