Requesting for freeze exception for RMRR

Requesting for freeze exception for RMRR

[Chen, Tiejun]

Hi Wei,

Here I'm trying to request the freeze exception for RMRR.

1. clarify the state of patch series / feature.

Reviewed	Acked	RMRR series v7
Y		Y	[v7][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map
Y		Y	[v7][PATCH 02/16] xen/vtd: create RMRR mapping
Y		N	[v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm 
reservation policy
Y		Y	[v7][PATCH 04/16] xen: enable XENMEM_memory_map in hvm
Y		N	[v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]
Y		N	[v7][PATCH 06/16] hvmloader/pci: skip reserved ranges
Y		N	[v7][PATCH 07/16] hvmloader/e820: construct guest e820 table
Y		Y	[v7][PATCH 08/16] tools/libxc: Expose new hypercall 
xc_reserved_device_memory_map
Y		Y	[v7][PATCH 09/16] tools: extend xc_assign_device() to support rdm 
reservation policy
Y		Y	[v7][PATCH 10/16] tools: introduce some new parameters to set rdm 
policy
Y		Y	[v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Y		Y	[v7][PATCH 12/16] tools: introduce a new parameter to set a 
predefined rdm boundary
Y		Y	[v7][PATCH 13/16] libxl: construct e820 map with RDM information 
for HVM guest
Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with 
shared rmrr
Y		Y	[v7][PATCH 16/16] tools: parse to enable new rdm policy parameters	

Note Jackson and Campbell also raised some comments to improve current 
codes.

2. explain why it needs to be in this release (benefits).

RMRR mechanism was broken for a long time and this makes VM always face 
security issues. In addition, those associated devices can't be passed 
through to VM and even result in VM crashes.

3. explain why it doesn't break things (risks).

Our policy makes sure that system will work in the original way by 
default as without the RMRR patches. And especially, this series just 
impacts those platforms which have RMRR.

4. CC relevant maintainers and release manager.
Done

Cheers,
Tiejun

Re: Requesting for freeze exception for RMRR

[Jan Beulich]

>>> On 13.07.15 at 08:31, <tiejun.chen@intel.com> wrote:
> 3. explain why it doesn't break things (risks).
> 
> Our policy makes sure that system will work in the original way by 
> default as without the RMRR patches. And especially, this series just 
> impacts those platforms which have RMRR.

I think this should read "Our policy intends to make sure ...", making
more clear that there is a risk here (supported by the history of the
series).

Jan

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Mon, Jul 13, 2015 at 02:31:58PM +0800, Chen, Tiejun wrote:
> Hi Wei,
> 
> Here I'm trying to request the freeze exception for RMRR.
> 
> 1. clarify the state of patch series / feature.
> 
> Reviewed	Acked	RMRR series v7
> Y		Y	[v7][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map
> Y		Y	[v7][PATCH 02/16] xen/vtd: create RMRR mapping
> Y		N	[v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm
> reservation policy
> Y		Y	[v7][PATCH 04/16] xen: enable XENMEM_memory_map in hvm
> Y		N	[v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]
> Y		N	[v7][PATCH 06/16] hvmloader/pci: skip reserved ranges
> Y		N	[v7][PATCH 07/16] hvmloader/e820: construct guest e820 table
> Y		Y	[v7][PATCH 08/16] tools/libxc: Expose new hypercall
> xc_reserved_device_memory_map
> Y		Y	[v7][PATCH 09/16] tools: extend xc_assign_device() to support rdm
> reservation policy
> Y		Y	[v7][PATCH 10/16] tools: introduce some new parameters to set rdm
> policy
> Y		Y	[v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
> Y		Y	[v7][PATCH 12/16] tools: introduce a new parameter to set a predefined
> rdm boundary
> Y		Y	[v7][PATCH 13/16] libxl: construct e820 map with RDM information for
> HVM guest
> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with shared
> rmrr
> Y		Y	[v7][PATCH 16/16] tools: parse to enable new rdm policy parameters	

Please work with maintainers to get those hvmloader patches acked or
reviewed.

> 
> Note Jackson and Campbell also raised some comments to improve current
> codes.
> 
> 2. explain why it needs to be in this release (benefits).
> 
> RMRR mechanism was broken for a long time and this makes VM always face
> security issues. In addition, those associated devices can't be passed
> through to VM and even result in VM crashes.
> 
> 3. explain why it doesn't break things (risks).
> 
> Our policy makes sure that system will work in the original way by default
> as without the RMRR patches. And especially, this series just impacts those
> platforms which have RMRR.
> 

Your patches touch crucial path in hvmloader to build memory map. There
is risk that it may break hvmloader even if it is turned off.

I would need at least some positive test results from you to confirm if
this feature is turned off everything works as before.

Wei.

> 4. CC relevant maintainers and release manager.
> Done
> 
> Cheers,
> Tiejun

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

> Please work with maintainers to get those hvmloader patches acked or
> reviewed.

I will do.

>
>>
>> Note Jackson and Campbell also raised some comments to improve current
>> codes.
>>
>> 2. explain why it needs to be in this release (benefits).
>>
>> RMRR mechanism was broken for a long time and this makes VM always face
>> security issues. In addition, those associated devices can't be passed
>> through to VM and even result in VM crashes.
>>
>> 3. explain why it doesn't break things (risks).
>>
>> Our policy makes sure that system will work in the original way by default
>> as without the RMRR patches. And especially, this series just impacts those
>> platforms which have RMRR.
>>
>
> Your patches touch crucial path in hvmloader to build memory map. There
> is risk that it may break hvmloader even if it is turned off.
>
> I would need at least some positive test results from you to confirm if
> this feature is turned off everything works as before.
>

Could you show what sort of test you need here? I just did boot a VM 
without any RDM parameters. I just think maybe someone had this good 
experience to check this.

Thanks
Tiejun

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Tue, Jul 14, 2015 at 09:27:17AM +0800, Chen, Tiejun wrote:
> >Please work with maintainers to get those hvmloader patches acked or
> >reviewed.
> 
> I will do.
> 
> >
> >>
> >>Note Jackson and Campbell also raised some comments to improve current
> >>codes.
> >>
> >>2. explain why it needs to be in this release (benefits).
> >>
> >>RMRR mechanism was broken for a long time and this makes VM always face
> >>security issues. In addition, those associated devices can't be passed
> >>through to VM and even result in VM crashes.
> >>
> >>3. explain why it doesn't break things (risks).
> >>
> >>Our policy makes sure that system will work in the original way by default
> >>as without the RMRR patches. And especially, this series just impacts those
> >>platforms which have RMRR.
> >>
> >
> >Your patches touch crucial path in hvmloader to build memory map. There
> >is risk that it may break hvmloader even if it is turned off.
> >
> >I would need at least some positive test results from you to confirm if
> >this feature is turned off everything works as before.
> >
> 
> Could you show what sort of test you need here? I just did boot a VM without
> any RDM parameters. I just think maybe someone had this good experience to
> check this.
> 

Yes, this is the basic test I need -- at least booting a VM with RDM
off.

Feel free to do more tests and report back if you have time.

Wei.

> Thanks
> Tiejun

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

On 2015/7/14 17:29, Wei Liu wrote:
> On Tue, Jul 14, 2015 at 09:27:17AM +0800, Chen, Tiejun wrote:
>>> Please work with maintainers to get those hvmloader patches acked or
>>> reviewed.
>>
>> I will do.

Maybe I need to update current status today.

I just send out v8:

* All tools comments raised by Jackson and Campbell are addressed and I 
don't see further feedback.

* On hv side, last one is reviewed by George. And looks Jan have no 
obvious objection to that. (Jan: If I'm wrong let me take it back. )

* On hvmloader side, there three patches now:
   patch #5 is reviewed by George and Acked by Jan;
   patch #6 is reviewed just for code implementation but that solution 
can't convince all maintainers. Honestly, this is a rare possibility of 
collision in real world and the original pci allocation is not good so 
its hard to reshape the original mechanism in one week. But actually we 
also have some other solutions but we need a little time to figure out 
how to step forward but I just think any solution wouldn't bring any 
change to other stuffs.
   patch #7 is pretty close to be Acked.

So what's your final determination?

Thanks
Tiejun

>>
>>>
>>>>
>>>> Note Jackson and Campbell also raised some comments to improve current
>>>> codes.
>>>>
>>>> 2. explain why it needs to be in this release (benefits).
>>>>
>>>> RMRR mechanism was broken for a long time and this makes VM always face
>>>> security issues. In addition, those associated devices can't be passed
>>>> through to VM and even result in VM crashes.
>>>>
>>>> 3. explain why it doesn't break things (risks).
>>>>
>>>> Our policy makes sure that system will work in the original way by default
>>>> as without the RMRR patches. And especially, this series just impacts those
>>>> platforms which have RMRR.
>>>>
>>>
>>> Your patches touch crucial path in hvmloader to build memory map. There
>>> is risk that it may break hvmloader even if it is turned off.
>>>
>>> I would need at least some positive test results from you to confirm if
>>> this feature is turned off everything works as before.
>>>
>>
>> Could you show what sort of test you need here? I just did boot a VM without
>> any RDM parameters. I just think maybe someone had this good experience to
>> check this.
>>
>
> Yes, this is the basic test I need -- at least booting a VM with RDM
> off.
>
> Feel free to do more tests and report back if you have time.
>
> Wei.
>
>> Thanks
>> Tiejun
>
>

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Fri, Jul 17, 2015 at 09:16:08AM +0800, Chen, Tiejun wrote:
> On 2015/7/14 17:29, Wei Liu wrote:
> >On Tue, Jul 14, 2015 at 09:27:17AM +0800, Chen, Tiejun wrote:
> >>>Please work with maintainers to get those hvmloader patches acked or
> >>>reviewed.
> >>
> >>I will do.
> 
> Maybe I need to update current status today.
> 
> I just send out v8:
> 
> * All tools comments raised by Jackson and Campbell are addressed and I
> don't see further feedback.
> 
> * On hv side, last one is reviewed by George. And looks Jan have no obvious
> objection to that. (Jan: If I'm wrong let me take it back. )
> 
> * On hvmloader side, there three patches now:
>   patch #5 is reviewed by George and Acked by Jan;
>   patch #6 is reviewed just for code implementation but that solution can't
> convince all maintainers. Honestly, this is a rare possibility of collision
> in real world and the original pci allocation is not good so its hard to
> reshape the original mechanism in one week. But actually we also have some
> other solutions but we need a little time to figure out how to step forward
> but I just think any solution wouldn't bring any change to other stuffs.
>   patch #7 is pretty close to be Acked.
> 
> So what's your final determination?
> 

If you and maintainers can work out a solution for hvmloader bits today
then yes, otherwise no.

Wei.

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

>> * On hvmloader side, there three patches now:
>>    patch #5 is reviewed by George and Acked by Jan;
>>    patch #6 is reviewed just for code implementation but that solution can't
>> convince all maintainers. Honestly, this is a rare possibility of collision
>> in real world and the original pci allocation is not good so its hard to
>> reshape the original mechanism in one week. But actually we also have some
>> other solutions but we need a little time to figure out how to step forward
>> but I just think any solution wouldn't bring any change to other stuffs.
>>    patch #7 is pretty close to be Acked.
>>
>> So what's your final determination?
>>
>
> If you and maintainers can work out a solution for hvmloader bits today
> then yes, otherwise no.
>

Based on my understanding, we already have some solutions but we need 
some time to put one into practice. I think its possible to work out 
this next week. As you see other stuffs are almost closed so we can put 
our all effort just on this bit.

Thanks
Tiejun

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Fri, Jul 17, 2015 at 05:24:55PM +0800, Chen, Tiejun wrote:
> >>* On hvmloader side, there three patches now:
> >>   patch #5 is reviewed by George and Acked by Jan;
> >>   patch #6 is reviewed just for code implementation but that solution can't
> >>convince all maintainers. Honestly, this is a rare possibility of collision
> >>in real world and the original pci allocation is not good so its hard to
> >>reshape the original mechanism in one week. But actually we also have some
> >>other solutions but we need a little time to figure out how to step forward
> >>but I just think any solution wouldn't bring any change to other stuffs.
> >>   patch #7 is pretty close to be Acked.
> >>
> >>So what's your final determination?
> >>
> >
> >If you and maintainers can work out a solution for hvmloader bits today
> >then yes, otherwise no.
> >
> 
> Based on my understanding, we already have some solutions but we need some
> time to put one into practice. I think its possible to work out this next
> week. As you see other stuffs are almost closed so we can put our all effort
> just on this bit.
> 

Jan, Andrew and George, can you provide some insight? If that's the case
we may be able to get that done within next week?

Wei.

> Thanks
> Tiejun

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Fri, Jul 17, 2015 at 10:30:41AM +0100, Wei Liu wrote:
> On Fri, Jul 17, 2015 at 05:24:55PM +0800, Chen, Tiejun wrote:
> > >>* On hvmloader side, there three patches now:
> > >>   patch #5 is reviewed by George and Acked by Jan;
> > >>   patch #6 is reviewed just for code implementation but that solution can't
> > >>convince all maintainers. Honestly, this is a rare possibility of collision
> > >>in real world and the original pci allocation is not good so its hard to
> > >>reshape the original mechanism in one week. But actually we also have some
> > >>other solutions but we need a little time to figure out how to step forward
> > >>but I just think any solution wouldn't bring any change to other stuffs.
> > >>   patch #7 is pretty close to be Acked.
> > >>
> > >>So what's your final determination?
> > >>
> > >
> > >If you and maintainers can work out a solution for hvmloader bits today
> > >then yes, otherwise no.
> > >
> > 
> > Based on my understanding, we already have some solutions but we need some
> > time to put one into practice. I think its possible to work out this next
> > week. As you see other stuffs are almost closed so we can put our all effort
> > just on this bit.
> > 
> 
> Jan, Andrew and George, can you provide some insight? If that's the case
> we may be able to get that done within next week?
> 

I'm replying to myself because I read some emails from v7 to v9 and get
some basic ideas. Correct me if I'm wrong.

The major concern seems to be around the PCI allocation algorithm. Jan
has different opinion from George. George provided a simple solution
that will not make things worse than before, while Jan prefers to get
everything right.

To be fair, the PCI allocation code in a bad state is not really
contributor's fault.

Jan also pointed out on IRC he thinks the proper logic he asked for is
not very hard to implement.

Given we either take George's route, which already seems to have a
patch, or Jan's route, which he thinks shouldn't be too hard to
implement, I'm inclined to say give this series another week (24th
deadline still applied). Note that we've been working on this for ages,
any delay is going to burn up more energy than necessary.

Jan and George, if you disagree with what I say above, please reply.

Wei.


> Wei.
> 
> > Thanks
> > Tiejun

Re: Requesting for freeze exception for RMRR

[Jan Beulich]

>>> On 17.07.15 at 15:21, <wei.liu2@citrix.com> wrote:
> The major concern seems to be around the PCI allocation algorithm. Jan
> has different opinion from George. George provided a simple solution
> that will not make things worse than before, while Jan prefers to get
> everything right.
> 
> To be fair, the PCI allocation code in a bad state is not really
> contributor's fault.
> 
> Jan also pointed out on IRC he thinks the proper logic he asked for is
> not very hard to implement.
> 
> Given we either take George's route, which already seems to have a
> patch, or Jan's route, which he thinks shouldn't be too hard to
> implement, I'm inclined to say give this series another week (24th
> deadline still applied). Note that we've been working on this for ages,
> any delay is going to burn up more energy than necessary.
> 
> Jan and George, if you disagree with what I say above, please reply.

My main disagreement here continues to be that we're talking
about a bug fix, and hence I don't view this as needing a freeze
exception in the first place (at least not at this point in time). Yes,
the bug fix involves adding code that looks like a new feature, but
that happens with bug fixes.

Jan

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Fri, Jul 17, 2015 at 02:43:05PM +0100, Jan Beulich wrote:
> >>> On 17.07.15 at 15:21, <wei.liu2@citrix.com> wrote:
> > The major concern seems to be around the PCI allocation algorithm. Jan
> > has different opinion from George. George provided a simple solution
> > that will not make things worse than before, while Jan prefers to get
> > everything right.
> > 
> > To be fair, the PCI allocation code in a bad state is not really
> > contributor's fault.
> > 
> > Jan also pointed out on IRC he thinks the proper logic he asked for is
> > not very hard to implement.
> > 
> > Given we either take George's route, which already seems to have a
> > patch, or Jan's route, which he thinks shouldn't be too hard to
> > implement, I'm inclined to say give this series another week (24th
> > deadline still applied). Note that we've been working on this for ages,
> > any delay is going to burn up more energy than necessary.
> > 
> > Jan and George, if you disagree with what I say above, please reply.
> 
> My main disagreement here continues to be that we're talking
> about a bug fix, and hence I don't view this as needing a freeze
> exception in the first place (at least not at this point in time). Yes,
> the bug fix involves adding code that looks like a new feature, but
> that happens with bug fixes.
> 

Fine then. I'm not going to argue feature vs bug fix at this stage.  The
final resolution is still the same. Tiejun can continue working on this
next week.

Wei.

> Jan

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

>> My main disagreement here continues to be that we're talking
>> about a bug fix, and hence I don't view this as needing a freeze
>> exception in the first place (at least not at this point in time). Yes,
>> the bug fix involves adding code that looks like a new feature, but
>> that happens with bug fixes.
>>
>
> Fine then. I'm not going to argue feature vs bug fix at this stage.  The
> final resolution is still the same. Tiejun can continue working on this
> next week.
>

Wei and Jan,

Really thanks for your clarification to this case.

Looks two key problems should be addressed as follows:

#1. mmio conflicting with RDM

As Jan suggested George's patch is fine in this phrase.

#2. construct e820

I need to understand what Jan comments properly than resend a patch.

I'm going to finalize these things next week as early as possible.

Again, thanks for all guys's help and I can't walk here without any your 
guides.

Thanks
Tiejun

Re: Requesting for freeze exception for RMRR

[Andrew Cooper]

On 17/07/15 15:01, Wei Liu wrote:
> On Fri, Jul 17, 2015 at 02:43:05PM +0100, Jan Beulich wrote:
>>>>> On 17.07.15 at 15:21, <wei.liu2@citrix.com> wrote:
>>> The major concern seems to be around the PCI allocation algorithm. Jan
>>> has different opinion from George. George provided a simple solution
>>> that will not make things worse than before, while Jan prefers to get
>>> everything right.
>>>
>>> To be fair, the PCI allocation code in a bad state is not really
>>> contributor's fault.
>>>
>>> Jan also pointed out on IRC he thinks the proper logic he asked for is
>>> not very hard to implement.
>>>
>>> Given we either take George's route, which already seems to have a
>>> patch, or Jan's route, which he thinks shouldn't be too hard to
>>> implement, I'm inclined to say give this series another week (24th
>>> deadline still applied). Note that we've been working on this for ages,
>>> any delay is going to burn up more energy than necessary.
>>>
>>> Jan and George, if you disagree with what I say above, please reply.
>> My main disagreement here continues to be that we're talking
>> about a bug fix, and hence I don't view this as needing a freeze
>> exception in the first place (at least not at this point in time). Yes,
>> the bug fix involves adding code that looks like a new feature, but
>> that happens with bug fixes.
>>
> Fine then. I'm not going to argue feature vs bug fix at this stage.  The
> final resolution is still the same. Tiejun can continue working on this
> next week.

Sorry for being slow in my maintainership role with this series.  (I
have been busy with the migration v2 side of things).

I can appreciate Wei's position that, despite this being a bugfix, it
does exhibit itself as a new feature, and we don't want to be merging a
new feature beyond the hard feature freeze point.

The PCI allocation code is in a state, but it was in a similarly bad
state before.  I agree with Jan's point of the risk that these new
changes cause a regression in booting guests, although we can mitigate
that somewhat by testing.

I feel at this point that we shouldn't block the RMRR bugfix on also
fixing the PCI allocation algorithm (which was a pre-existing issue).

Therefore, I recommend that v9 gets respun to v10 to address the current
comments, and accepted.  Afterwards, the PCI allocation algorithm gets
worked on as a bugfix activity, to pro actively cater for the risk of
regression.

~Andrew

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

> The PCI allocation code is in a state, but it was in a similarly bad
> state before.  I agree with Jan's point of the risk that these new
> changes cause a regression in booting guests, although we can mitigate
> that somewhat by testing.
>
> I feel at this point that we shouldn't block the RMRR bugfix on also
> fixing the PCI allocation algorithm (which was a pre-existing issue).
>
> Therefore, I recommend that v9 gets respun to v10 to address the current
> comments, and accepted.  Afterwards, the PCI allocation algorithm gets
> worked on as a bugfix activity, to pro actively cater for the risk of
> regression.

If you have any good solution to fix the PCI allocation algorithm 
completely, things couldn't be better :)

Thanks
Tiejun

Re: Requesting for freeze exception for RMRR

[Wei Liu]

On Fri, Jul 17, 2015 at 11:26:30PM +0800, Chen, Tiejun wrote:
> >The PCI allocation code is in a state, but it was in a similarly bad
> >state before.  I agree with Jan's point of the risk that these new
> >changes cause a regression in booting guests, although we can mitigate
> >that somewhat by testing.
> >
> >I feel at this point that we shouldn't block the RMRR bugfix on also
> >fixing the PCI allocation algorithm (which was a pre-existing issue).
> >
> >Therefore, I recommend that v9 gets respun to v10 to address the current
> >comments, and accepted.  Afterwards, the PCI allocation algorithm gets
> >worked on as a bugfix activity, to pro actively cater for the risk of
> >regression.
> 
> If you have any good solution to fix the PCI allocation algorithm
> completely, things couldn't be better :)
> 

I think Andrew means you (or someone else) improves that algorithm
later. No need to provide a perfect solution next week.

> Thanks
> Tiejun

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

> I think Andrew means you (or someone else) improves that algorithm
> later. No need to provide a perfect solution next week.
>

Yes, I understand what he mean. But I still want to further ask if he 
have such a good idea right now, maybe we can try to address that 
directly :)

Thanks
Tiejun

Re: Requesting for freeze exception for RMRR

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, July 17, 2015 11:11 PM
> 
> On 17/07/15 15:01, Wei Liu wrote:
> > On Fri, Jul 17, 2015 at 02:43:05PM +0100, Jan Beulich wrote:
> >>>>> On 17.07.15 at 15:21, <wei.liu2@citrix.com> wrote:
> >>> The major concern seems to be around the PCI allocation algorithm. Jan
> >>> has different opinion from George. George provided a simple solution
> >>> that will not make things worse than before, while Jan prefers to get
> >>> everything right.
> >>>
> >>> To be fair, the PCI allocation code in a bad state is not really
> >>> contributor's fault.
> >>>
> >>> Jan also pointed out on IRC he thinks the proper logic he asked for is
> >>> not very hard to implement.
> >>>
> >>> Given we either take George's route, which already seems to have a
> >>> patch, or Jan's route, which he thinks shouldn't be too hard to
> >>> implement, I'm inclined to say give this series another week (24th
> >>> deadline still applied). Note that we've been working on this for ages,
> >>> any delay is going to burn up more energy than necessary.
> >>>
> >>> Jan and George, if you disagree with what I say above, please reply.
> >> My main disagreement here continues to be that we're talking
> >> about a bug fix, and hence I don't view this as needing a freeze
> >> exception in the first place (at least not at this point in time). Yes,
> >> the bug fix involves adding code that looks like a new feature, but
> >> that happens with bug fixes.
> >>
> > Fine then. I'm not going to argue feature vs bug fix at this stage.  The
> > final resolution is still the same. Tiejun can continue working on this
> > next week.
> 
> Sorry for being slow in my maintainership role with this series.  (I
> have been busy with the migration v2 side of things).
> 
> I can appreciate Wei's position that, despite this being a bugfix, it
> does exhibit itself as a new feature, and we don't want to be merging a
> new feature beyond the hard feature freeze point.
> 
> The PCI allocation code is in a state, but it was in a similarly bad
> state before.  I agree with Jan's point of the risk that these new
> changes cause a regression in booting guests, although we can mitigate
> that somewhat by testing.
> 
> I feel at this point that we shouldn't block the RMRR bugfix on also
> fixing the PCI allocation algorithm (which was a pre-existing issue).
> 
> Therefore, I recommend that v9 gets respun to v10 to address the current
> comments, and accepted.  Afterwards, the PCI allocation algorithm gets
> worked on as a bugfix activity, to pro actively cater for the risk of
> regression.
> 

Agree with this recommendation. 

Thanks
Kevin

Re: Requesting for freeze exception for RMRR

[Jan Beulich]

>>> On 13.07.15 at 08:31, <tiejun.chen@intel.com> wrote:
> Hi Wei,
> 
> Here I'm trying to request the freeze exception for RMRR.
> 
> 1. clarify the state of patch series / feature.
> 
> Reviewed	Acked	RMRR series v7
> Y		Y	[v7][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map
> Y		Y	[v7][PATCH 02/16] xen/vtd: create RMRR mapping
> Y		N	[v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm 
> reservation policy

I can't seem to find any such Reviewed-by on the list (and the patch
itself doesn't carry one either).

> Y		Y	[v7][PATCH 04/16] xen: enable XENMEM_memory_map in hvm
> Y		N	[v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]
> Y		N	[v7][PATCH 06/16] hvmloader/pci: skip reserved ranges

Same here.

> Y		N	[v7][PATCH 07/16] hvmloader/e820: construct guest e820 table

And again.

> Y		Y	[v7][PATCH 08/16] tools/libxc: Expose new hypercall 
> xc_reserved_device_memory_map
> Y		Y	[v7][PATCH 09/16] tools: extend xc_assign_device() to support rdm 
> reservation policy
> Y		Y	[v7][PATCH 10/16] tools: introduce some new parameters to set rdm 
> policy
> Y		Y	[v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
> Y		Y	[v7][PATCH 12/16] tools: introduce a new parameter to set a 
> predefined rdm boundary
> Y		Y	[v7][PATCH 13/16] libxl: construct e820 map with RDM information 
> for HVM guest
> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with 
> shared rmrr

And yet again for these two. Please avoid giving a false impression
of the state of the series to the release manager (and others).

Jan

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

>> 1. clarify the state of patch series / feature.
>>
>> Reviewed	Acked	RMRR series v7
>> Y		Y	[v7][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map
>> Y		Y	[v7][PATCH 02/16] xen/vtd: create RMRR mapping
>> Y		N	[v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm
>> reservation policy
>
> I can't seem to find any such Reviewed-by on the list (and the patch
> itself doesn't carry one either).
>
>> Y		Y	[v7][PATCH 04/16] xen: enable XENMEM_memory_map in hvm
>> Y		N	[v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]
>> Y		N	[v7][PATCH 06/16] hvmloader/pci: skip reserved ranges
>
> Same here.
>
>> Y		N	[v7][PATCH 07/16] hvmloader/e820: construct guest e820 table
>
> And again.

Sorry this is my fault to these hv patches.

>
>> Y		Y	[v7][PATCH 08/16] tools/libxc: Expose new hypercall
>> xc_reserved_device_memory_map
>> Y		Y	[v7][PATCH 09/16] tools: extend xc_assign_device() to support rdm
>> reservation policy
>> Y		Y	[v7][PATCH 10/16] tools: introduce some new parameters to set rdm
>> policy
>> Y		Y	[v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
>> Y		Y	[v7][PATCH 12/16] tools: introduce a new parameter to set a
>> predefined rdm boundary
>> Y		Y	[v7][PATCH 13/16] libxl: construct e820 map with RDM information
>> for HVM guest
>> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
>> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with
>> shared rmrr
>
> And yet again for these two. Please avoid giving a false impression

But these two patches really won Kevin's Ack, and also I wrote this line

Acked-by: Kevin Tian <kevin.tian@intel.com>

both in these two patches.

Thanks
Tiejun

> of the state of the series to the release manager (and others).
>
> Jan
>
>

Re: Requesting for freeze exception for RMRR

[Jan Beulich]

>>> On 14.07.15 at 02:26, <tiejun.chen@intel.com> wrote:
>> > 1. clarify the state of patch series / feature.
>>>
>>> Reviewed	Acked	RMRR series v7
>>> Y		Y	[v7][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map
>>> Y		Y	[v7][PATCH 02/16] xen/vtd: create RMRR mapping
>>> Y		N	[v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm
>>> reservation policy
>>
>> I can't seem to find any such Reviewed-by on the list (and the patch
>> itself doesn't carry one either).
>>
>>> Y		Y	[v7][PATCH 04/16] xen: enable XENMEM_memory_map in hvm
>>> Y		N	[v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[]
>>> Y		N	[v7][PATCH 06/16] hvmloader/pci: skip reserved ranges
>>
>> Same here.
>>
>>> Y		N	[v7][PATCH 07/16] hvmloader/e820: construct guest e820 table
>>
>> And again.
> 
> Sorry this is my fault to these hv patches.
> 
>>
>>> Y		Y	[v7][PATCH 08/16] tools/libxc: Expose new hypercall
>>> xc_reserved_device_memory_map
>>> Y		Y	[v7][PATCH 09/16] tools: extend xc_assign_device() to support rdm
>>> reservation policy
>>> Y		Y	[v7][PATCH 10/16] tools: introduce some new parameters to set rdm
>>> policy
>>> Y		Y	[v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
>>> Y		Y	[v7][PATCH 12/16] tools: introduce a new parameter to set a
>>> predefined rdm boundary
>>> Y		Y	[v7][PATCH 13/16] libxl: construct e820 map with RDM information
>>> for HVM guest
>>> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
>>> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with
>>> shared rmrr
>>
>> And yet again for these two. Please avoid giving a false impression
> 
> But these two patches really won Kevin's Ack, and also I wrote this line
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> 
> both in these two patches.

But talk here is about their review status, not who ack-ed them (and
an ack by other than  a maintainer of the affected code is not very
meaningful anyway).

Jan

Re: Requesting for freeze exception for RMRR

[Ian Campbell]

On Tue, 2015-07-14 at 10:18 +0100, Jan Beulich wrote:

> >>> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment

diffstat:
 xen/drivers/passthrough/vtd/dmar.h  |  1 -
 xen/drivers/passthrough/vtd/iommu.c | 11 ++---------
 xen/drivers/passthrough/vtd/utils.c |  7 -------
 3 files changed, 2 insertions(+), 17 deletions(-)

> >>> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with
> >>> shared rmrr

xen/drivers/passthrough/vtd/iommu.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

> >>
> >> And yet again for these two. Please avoid giving a false impression
> > 
> > But these two patches really won Kevin's Ack, and also I wrote this line
> > 
> > Acked-by: Kevin Tian <kevin.tian@intel.com>
> > 
> > both in these two patches.
> 
> But talk here is about their review status, not who ack-ed them (and
> an ack by other than  a maintainer of the affected code is not very
> meaningful anyway).

Kevin is maintainer of that code though, isn't he?

$ ./scripts/get_maintainer.pl -f xen/drivers/passthrough/vtd/dmar.h
Yang Zhang <yang.z.zhang@intel.com>
Kevin Tian <kevin.tian@intel.com>
xen-devel@lists.xen.org

(same for the other files)

>From MAINTAINERS I suppose that is this entry:

INTEL(R) VT FOR DIRECTED I/O (VT-D)
M:      Yang Zhang <yang.z.zhang@intel.com>
M:      Kevin Tian <kevin.tian@intel.com>
S:      Supported
F:      xen/drivers/passthrough/vtd/

Ian.

Re: Requesting for freeze exception for RMRR

[Jan Beulich]

>>> On 14.07.15 at 11:25, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-07-14 at 10:18 +0100, Jan Beulich wrote:
> 
>> >>> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
> 
> diffstat:
>  xen/drivers/passthrough/vtd/dmar.h  |  1 -
>  xen/drivers/passthrough/vtd/iommu.c | 11 ++---------
>  xen/drivers/passthrough/vtd/utils.c |  7 -------
>  3 files changed, 2 insertions(+), 17 deletions(-)
> 
>> >>> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with
>> >>> shared rmrr
> 
> xen/drivers/passthrough/vtd/iommu.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
>> >>
>> >> And yet again for these two. Please avoid giving a false impression
>> > 
>> > But these two patches really won Kevin's Ack, and also I wrote this line
>> > 
>> > Acked-by: Kevin Tian <kevin.tian@intel.com>
>> > 
>> > both in these two patches.
>> 
>> But talk here is about their review status, not who ack-ed them (and
>> an ack by other than  a maintainer of the affected code is not very
>> meaningful anyway).
> 
> Kevin is maintainer of that code though, isn't he?

Oh, right, I got mislead by the "hv" in the middle of Tiejun's
earlier reply, assuming (base on remembering the ordering of the
series) what follows it are the hvmloader patches (without looking
at the actual titles again). Indeed, the part of my reply above in
parentheses was misguided. The patches not having been
reviewed by anyone still is a fact (as an ack is not a review iirc).

Jan

Re: Requesting for freeze exception for RMRR

[Chen, Tiejun]

>>>> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
>>>> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with
>>>> shared rmrr
>>>
>>> And yet again for these two. Please avoid giving a false impression
>>
>> But these two patches really won Kevin's Ack, and also I wrote this line
>>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>
>> both in these two patches.
>
> But talk here is about their review status, not who ack-ed them (and
> an ack by other than  a maintainer of the affected code is not very
> meaningful anyway).

Isn't Kevin the key maintainer specific to IOMMU subsystem?

Thanks
Tiejun

Re: Requesting for freeze exception for RMRR

[Jan Beulich]

>>> On 14.07.15 at 11:27, <tiejun.chen@intel.com> wrote:
>>>>> Y		Y	[v7][PATCH 14/16] xen/vtd: enable USB device assignment
>>>>> Y		Y	[v7][PATCH 15/16] xen/vtd: prevent from assign the device with
>>>>> shared rmrr
>>>>
>>>> And yet again for these two. Please avoid giving a false impression
>>>
>>> But these two patches really won Kevin's Ack, and also I wrote this line
>>>
>>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>>
>>> both in these two patches.
>>
>> But talk here is about their review status, not who ack-ed them (and
>> an ack by other than  a maintainer of the affected code is not very
>> meaningful anyway).
> 
> Isn't Kevin the key maintainer specific to IOMMU subsystem?

Yes, he is (for VT-d to be precise). See my reply just sent to Ian's
similar response.

Jan