Preface working plan for altp2m during freeze exception

Preface working plan for altp2m during freeze exception

[Sahita, Ravi]

Hi Maintainers,

First - Thanks for all your reviews so far!

As we head into this week of freeze exception for altp2m, we wanted to share our plan for addressing the remaining opens on this patch series.
This may be the general operating procedure but we wanted to  make sure we were operating with the same plan - hopefully this makes things smoother for this week:

We are working on addressing review comments in this order (and you will see this pattern in our review responses):

* Category 1 - Address review comments that affect ABI - these are of course required and will be addressed first.

* Category 2 - Address review comments that do not affect ABI - we will try to address ones that we think we can realistically meet within the time bounds - we ask you for some flexibility on these. If these cannot be addressed within the allotted exception time-frame, hopefully these wont be blockers for 4.6 since they can be addressed by follow-on patches.

* Category 3 - Address review comments that are really design questions - These we will try to address by short descriptions in review replies that attempt to give a gist of the design we followed, but of course design changes obviously cannot be done at this late stage - hopefully that is expected.

* Category 4 - Address trivial changes as we naturally update patches, however if we run out of time, some may remain un-addressed (to be taken care of post 4.6).

Can we please get a "yes - makes sense" sort of acknowledgement of this plan from the Maintainers? 

Of course any other feedback also on this plan will be great from you.

Thanks,
Ravi

I have specified our status here  based on the categories above just for completeness.
There are some patches that are in a good "r-b'ed no comments" state - could those be acked?

Current status from v5 series reviews:
-----------------------------------------------
Reviewed	Acked	altp2m series patch name and status from v5 reviews if not acked

Y		N	[PATCH v3 01/15] common/domain: Helpers to pause a domain while in context
Status if not acked: 	Jan has commented that he is ok with this patch - need r-b and ack please 

Y		Y	[PATCH v3 02/15] VMX: VMFUNC and #VE definitions and detection.

Y		Y	[PATCH v3 03/15] VMX: implement suppress #VE.
Status if not acked:	Needs a re-ack by Jun, George has acked by (we will remove older r-b's)

Y		N	[PATCH v3 04/15] x86/HVM: Hardware alternate p2m support detection
Status if not acked:	No issues reported - needs an ack please

Y		N	[PATCH v3 05/15] x86/altp2m: basic data structures and support routines
Status if not acked:	Category 3: we will write a short description of some design questions in review replies
			Category 2: moving altp2m struct to be dynamically allocated - this has minor benefit and big downside so will be lower priority, also some error handling fixes

Y		Y	[PATCH v3 06/15] VMX/altp2m: add code to support EPTP switching and #VE
			Category 2: related to handling of VMCS fields for EPTP index - we are trying to match hardware behavior

Y		N	[PATCH v3 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator
Status if not acked:	Category 2/3 - changes staged after Jan's feedback on v5 - ack with those addressed in v6?

Y		N	[PATCH v3 08/15] x86/altp2m: add control of suppress_ve
Status if not acked:	Now has r-b both Jan and George -  need maintainers ack on this one please

Y		Y	[PATCH v3 09/15] x86/altp2m: alternate p2m memory events
			Minor but Category 1: VM event struct padding	

Y		N	[PATCH v3 10/15] x86/altp2m: add remaining support routines
Status if not acked:	Category 3: we will write a short description in reply for why race condition cannot occur in log dirty/altp2m interaction
			Category 2: ept wrappers to be added to p2m (does not affect ABI)
			Category 4: one review comment from George's review

Y		N	[PATCH v3 11/15] x86/altp2m: define and implement alternate p2m HVMOP types
Status if not acked:	Category 1: couple things - make enable_notify more generic
			Category 2: couple things - hvm_op code flow changes and 1 other minor one from Jan's review

Y		Y	[PATCH v3 12/15] x86/altp2m: Add altp2mhvm HVM domain parameter

Y		Y	[PATCH v3 13/15] x86/altp2m: XSM hooks for altp2m HVM ops

Y		Y	[PATCH v4 14/15] tools/libxc: add support to altp2m hvmops
			ABI changes mean we will have to make minor changes here (to update)

Y		N	[PATCH v4 15/15] tools/xen-access: altp2m testcases
Status if not acked:	No pending open issues- ack needed please (ABI changes mean we will have to make minor changes here)

END

Re: Preface working plan for altp2m during freeze exception

[Jan Beulich]

>>> On 17.07.15 at 21:43, <ravi.sahita@intel.com> wrote:
> We are working on addressing review comments in this order (and you will see 
> this pattern in our review responses):
> 
> * Category 1 - Address review comments that affect ABI - these are of course 
> required and will be addressed first.
> 
> * Category 2 - Address review comments that do not affect ABI - we will try to 
> address ones that we think we can realistically meet within the time bounds - 
> we ask you for some flexibility on these. If these cannot be addressed within 
> the allotted exception time-frame, hopefully these wont be blockers for 4.6 
> since they can be addressed by follow-on patches.

Not sure - we've had bad experience with allowing code to go in with
the promise for later adjustments (which then never happened)...

> * Category 3 - Address review comments that are really design questions - 
> These we will try to address by short descriptions in review replies that 
> attempt to give a gist of the design we followed, but of course design 
> changes obviously cannot be done at this late stage - hopefully that is 
> expected.

If you really just mean questions on the design (rather than questions
possibly resulting int the requirement to change the design), then
that'd be fine of course. I think you understand that we shouldn't be
deferring issues that require design adjustments. Otoh I don't even
recall what design questions there were.

> * Category 4 - Address trivial changes as we naturally update patches, 
> however if we run out of time, some may remain un-addressed (to be taken care 
> of post 4.6).

See above (point 2).

> Can we please get a "yes - makes sense" sort of acknowledgement of this plan 
> from the Maintainers? 

Considering the limitations above, this is only a "maybe" from me.

> Y		N	[PATCH v3 05/15] x86/altp2m: basic data structures and support routines
> Status if not acked:	Category 3: we will write a short description of some 
> design questions in review replies
> 			Category 2: moving altp2m struct to be dynamically allocated - this has minor 
> benefit and big downside so will be lower priority, also some error handling 
> fixes

Big downside? You're not referring to the mechanical adjustments this
implies, are you?

> Y		N	[PATCH v3 07/15] VMX: add VMFUNC leaf 0 (EPTP switching) to emulator
> Status if not acked:	Category 2/3 - changes staged after Jan's feedback on v5 - 
> ack with those addressed in v6?

Let's see what v6 looks like.

> Y		N	[PATCH v3 08/15] x86/altp2m: add control of suppress_ve
> Status if not acked:	Now has r-b both Jan and George -  need maintainers ack on 
> this one please

Who do you refer to by "maintainer" here? I think the trivial
adjustment to xen/arch/x86/mm/mem_sharing.c can in the worst
case go in without Andres' ack. And everything else is covered
by George's authorship and review.

Jan

Re: Preface working plan for altp2m during freeze exception

[Sahita, Ravi]

>From: Jan Beulich [mailto:JBeulich@suse.com]
>Sent: Monday, July 20, 2015 12:12 AM
>
>>>> On 17.07.15 at 21:43, <ravi.sahita@intel.com> wrote:
>> We are working on addressing review comments in this order (and you
>> will see this pattern in our review responses):
>>
>> * Category 1 - Address review comments that affect ABI - these are of
>> course required and will be addressed first.
>>
>> * Category 2 - Address review comments that do not affect ABI - we
>> will try to address ones that we think we can realistically meet
>> within the time bounds - we ask you for some flexibility on these. If
>> these cannot be addressed within the allotted exception time-frame,
>> hopefully these wont be blockers for 4.6 since they can be addressed by
>follow-on patches.
>
>Not sure - we've had bad experience with allowing code to go in with the
>promise for later adjustments (which then never happened)...
>

For some of the remaining items (not addressed by our v6) we have some tentative patches that we could share with you post 4.6, we just think we don't have time to get to Category 2 things (and probably not all of Category 4) to do a good job on them - please tell us based on your read of the v6 series if that is close to an acceptable "final - 1" series - with any minor i's to be dotted and t's to be crossed in the final - since realistically, we need to get a final patch series to you guys by Wednesday evening PDT (Thursday AM for you) - correct?

>> * Category 3 - Address review comments that are really design
>> questions - These we will try to address by short descriptions in
>> review replies that attempt to give a gist of the design we followed,
>> but of course design changes obviously cannot be done at this late
>> stage - hopefully that is expected.
>
>If you really just mean questions on the design (rather than questions possibly
>resulting int the requirement to change the design), then that'd be fine of
>course. I think you understand that we shouldn't be deferring issues that
>require design adjustments. Otoh I don't even recall what design questions
>there were.
>
>> * Category 4 - Address trivial changes as we naturally update patches,
>> however if we run out of time, some may remain un-addressed (to be
>> taken care of post 4.6).
>
>See above (point 2).
>
>> Can we please get a "yes - makes sense" sort of acknowledgement of
>> this plan from the Maintainers?
>
>Considering the limitations above, this is only a "maybe" from me.
>

Could you please review the v6 patches and see if your "maybe" can change to a "good for 4.6, pending changes post 4.6" - thanks.
Also please see my responses to your other open comments - I've tried to address all of them.

>> Y		N	[PATCH v3 05/15] x86/altp2m: basic data structures
>and support routines
>> Status if not acked:	Category 3: we will write a short description of some
>> design questions in review replies
>> 			Category 2: moving altp2m struct to be dynamically
>allocated - this
>> has minor benefit and big downside so will be lower priority, also
>> some error handling fixes
>
>Big downside? You're not referring to the mechanical adjustments this
>implies, are you?
>

Just time to turn around and test the changes with our tests - like I said, we have a patch for this now - but time is short, so would request for this to not be the blocker.


>> Y		N	[PATCH v3 07/15] VMX: add VMFUNC leaf 0 (EPTP
>switching) to emulator
>> Status if not acked:	Category 2/3 - changes staged after Jan's feedback on
>v5 -
>> ack with those addressed in v6?
>
>Let's see what v6 looks like.

Thanks.

>
>> Y		N	[PATCH v3 08/15] x86/altp2m: add control of
>suppress_ve
>> Status if not acked:	Now has r-b both Jan and George -  need maintainers
>ack on
>> this one please
>
>Who do you refer to by "maintainer" here? I think the trivial adjustment to
>xen/arch/x86/mm/mem_sharing.c can in the worst case go in without Andres'
>ack. And everything else is covered by George's authorship and review.

Ok thanks,

Ravi

>
>Jan