Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 23.10.2017 11:41, Jan Beulich wrote:
 On 23.10.17 at 10:34,  wrote:
> 
>>
>> On 23.10.2017 11:10, Jan Beulich wrote:
>> On 20.10.17 at 18:32,  wrote:
 On 10/20/2017 07:15 PM, Wei Liu wrote:
> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
>> From: Razvan Cojocaru 
>>
>> For the default EPT view we have xc_set_mem_access_multi(), which
>> is able to set an array of pages to an array of access rights with
>> a single hypercall. However, this functionality was lacking for the
>> altp2m subsystem, which could only set page restrictions for one
>> page at a time. This patch addresses the gap.
>>
>> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed 
>> to a
>> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
>> (and
>> hence with the original altp2m design, where domains are allowed - with 
>> the
>> proper altp2m access rights - to alter these settings), in the absence 
>> of an
>> official position on the issue from the original altp2m designers.
>>
>> Signed-off-by: Razvan Cojocaru 
>> Signed-off-by: Petre Pircalabu 
>>
>
> The title is a bit misleading -- this patch actually contains changes to
> hypervisor as well.

 Sorry, I have assumed that the hypervisor changes are implied. We're
 happy to change it. Would "x86/altp2m: Added
 xc_altp2m_set_mem_access_multi() and hypervisor support" be better?
>>>
>>> But please not again "Added" - we've had this discussion before.
>>> The title is supposed to tell what a patch does, not what the state
>>> of the code is after it was applied.
>>
>> Will do, how does "{xen,libxc}/altp2m: support for setting restrictions
>> for an array of pages" sound?
> 
> The text is fine, but I'm not sure the {xen,libxc} part of the prefix
> is really very useful.

I was hoping to address Wei's comment with it - 'xen' would stand for
the hypervisor part, and 'libxc' for the toolstack part. However, you're
right: for one, the 'x86' part was useful, and then the problem before
was not so much that it didn't explicitly specify 'xen', but that it
implied that the changes have more to do with libxc (because it
mentioned xc_altp2m_set_mem_access_multi()).

"x86/altp2m: support for setting restrictions for an array of pages" it
is then. :) Sorry for causing confusion!


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 23.10.17 at 10:34,  wrote:

> 
> On 23.10.2017 11:10, Jan Beulich wrote:
> On 20.10.17 at 18:32,  wrote:
>>> On 10/20/2017 07:15 PM, Wei Liu wrote:
 On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
> From: Razvan Cojocaru 
>
> For the default EPT view we have xc_set_mem_access_multi(), which
> is able to set an array of pages to an array of access rights with
> a single hypercall. However, this functionality was lacking for the
> altp2m subsystem, which could only set page restrictions for one
> page at a time. This patch addresses the gap.
>
> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed 
> to a
> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
> (and
> hence with the original altp2m design, where domains are allowed - with 
> the
> proper altp2m access rights - to alter these settings), in the absence of 
> an
> official position on the issue from the original altp2m designers.
>
> Signed-off-by: Razvan Cojocaru 
> Signed-off-by: Petre Pircalabu 
>

 The title is a bit misleading -- this patch actually contains changes to
 hypervisor as well.
>>>
>>> Sorry, I have assumed that the hypervisor changes are implied. We're
>>> happy to change it. Would "x86/altp2m: Added
>>> xc_altp2m_set_mem_access_multi() and hypervisor support" be better?
>> 
>> But please not again "Added" - we've had this discussion before.
>> The title is supposed to tell what a patch does, not what the state
>> of the code is after it was applied.
> 
> Will do, how does "{xen,libxc}/altp2m: support for setting restrictions
> for an array of pages" sound?

The text is fine, but I'm not sure the {xen,libxc} part of the prefix
is really very useful.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 23.10.2017 11:10, Jan Beulich wrote:
 On 20.10.17 at 18:32,  wrote:
>> On 10/20/2017 07:15 PM, Wei Liu wrote:
>>> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
 From: Razvan Cojocaru 

 For the default EPT view we have xc_set_mem_access_multi(), which
 is able to set an array of pages to an array of access rights with
 a single hypercall. However, this functionality was lacking for the
 altp2m subsystem, which could only set page restrictions for one
 page at a time. This patch addresses the gap.

 HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to 
 a
 DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
 (and
 hence with the original altp2m design, where domains are allowed - with the
 proper altp2m access rights - to alter these settings), in the absence of 
 an
 official position on the issue from the original altp2m designers.

 Signed-off-by: Razvan Cojocaru 
 Signed-off-by: Petre Pircalabu 

>>>
>>> The title is a bit misleading -- this patch actually contains changes to
>>> hypervisor as well.
>>
>> Sorry, I have assumed that the hypervisor changes are implied. We're
>> happy to change it. Would "x86/altp2m: Added
>> xc_altp2m_set_mem_access_multi() and hypervisor support" be better?
> 
> But please not again "Added" - we've had this discussion before.
> The title is supposed to tell what a patch does, not what the state
> of the code is after it was applied.

Will do, how does "{xen,libxc}/altp2m: support for setting restrictions
for an array of pages" sound?

We'll change the title as soon as we have comments to address for a new
version.


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 20.10.17 at 18:32,  wrote:
> On 10/20/2017 07:15 PM, Wei Liu wrote:
>> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
>>> From: Razvan Cojocaru 
>>>
>>> For the default EPT view we have xc_set_mem_access_multi(), which
>>> is able to set an array of pages to an array of access rights with
>>> a single hypercall. However, this functionality was lacking for the
>>> altp2m subsystem, which could only set page restrictions for one
>>> page at a time. This patch addresses the gap.
>>>
>>> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
>>> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
>>> (and
>>> hence with the original altp2m design, where domains are allowed - with the
>>> proper altp2m access rights - to alter these settings), in the absence of an
>>> official position on the issue from the original altp2m designers.
>>>
>>> Signed-off-by: Razvan Cojocaru 
>>> Signed-off-by: Petre Pircalabu 
>>>
>> 
>> The title is a bit misleading -- this patch actually contains changes to
>> hypervisor as well.
> 
> Sorry, I have assumed that the hypervisor changes are implied. We're
> happy to change it. Would "x86/altp2m: Added
> xc_altp2m_set_mem_access_multi() and hypervisor support" be better?

But please not again "Added" - we've had this discussion before.
The title is supposed to tell what a patch does, not what the state
of the code is after it was applied.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 10/20/2017 07:39 PM, Wei Liu wrote:
> On Fri, Oct 20, 2017 at 07:32:50PM +0300, Razvan Cojocaru wrote:
>> On 10/20/2017 07:15 PM, Wei Liu wrote:
>>> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
 From: Razvan Cojocaru 

 For the default EPT view we have xc_set_mem_access_multi(), which
 is able to set an array of pages to an array of access rights with
 a single hypercall. However, this functionality was lacking for the
 altp2m subsystem, which could only set page restrictions for one
 page at a time. This patch addresses the gap.

 HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to 
 a
 DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
 (and
 hence with the original altp2m design, where domains are allowed - with the
 proper altp2m access rights - to alter these settings), in the absence of 
 an
 official position on the issue from the original altp2m designers.

 Signed-off-by: Razvan Cojocaru 
 Signed-off-by: Petre Pircalabu 

>>>
>>> The title is a bit misleading -- this patch actually contains changes to
>>> hypervisor as well.
>>
>> Sorry, I have assumed that the hypervisor changes are implied.
> 
> And to expound my thought on this -- change in hypervisor is not
> implied. We have had cases in which only toolstack change was needed
> because hypervisor code was already there. Getting the title correct
> will help reviewers identify patches they need to review.

Got it. We'll change the title in the next iteration.


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Wei Liu]

On Fri, Oct 20, 2017 at 07:32:50PM +0300, Razvan Cojocaru wrote:
> On 10/20/2017 07:15 PM, Wei Liu wrote:
> > On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
> >> From: Razvan Cojocaru 
> >>
> >> For the default EPT view we have xc_set_mem_access_multi(), which
> >> is able to set an array of pages to an array of access rights with
> >> a single hypercall. However, this functionality was lacking for the
> >> altp2m subsystem, which could only set page restrictions for one
> >> page at a time. This patch addresses the gap.
> >>
> >> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to 
> >> a
> >> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
> >> (and
> >> hence with the original altp2m design, where domains are allowed - with the
> >> proper altp2m access rights - to alter these settings), in the absence of 
> >> an
> >> official position on the issue from the original altp2m designers.
> >>
> >> Signed-off-by: Razvan Cojocaru 
> >> Signed-off-by: Petre Pircalabu 
> >>
> > 
> > The title is a bit misleading -- this patch actually contains changes to
> > hypervisor as well.
> 
> Sorry, I have assumed that the hypervisor changes are implied.

And to expound my thought on this -- change in hypervisor is not
implied. We have had cases in which only toolstack change was needed
because hypervisor code was already there. Getting the title correct
will help reviewers identify patches they need to review.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Wei Liu]

On Fri, Oct 20, 2017 at 07:32:50PM +0300, Razvan Cojocaru wrote:
> On 10/20/2017 07:15 PM, Wei Liu wrote:
> > On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
> >> From: Razvan Cojocaru 
> >>
> >> For the default EPT view we have xc_set_mem_access_multi(), which
> >> is able to set an array of pages to an array of access rights with
> >> a single hypercall. However, this functionality was lacking for the
> >> altp2m subsystem, which could only set page restrictions for one
> >> page at a time. This patch addresses the gap.
> >>
> >> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to 
> >> a
> >> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart 
> >> (and
> >> hence with the original altp2m design, where domains are allowed - with the
> >> proper altp2m access rights - to alter these settings), in the absence of 
> >> an
> >> official position on the issue from the original altp2m designers.
> >>
> >> Signed-off-by: Razvan Cojocaru 
> >> Signed-off-by: Petre Pircalabu 
> >>
> > 
> > The title is a bit misleading -- this patch actually contains changes to
> > hypervisor as well.
> 
> Sorry, I have assumed that the hypervisor changes are implied. We're
> happy to change it. Would "x86/altp2m: Added
> xc_altp2m_set_mem_access_multi() and hypervisor support" be better?
> 

That's fine.

I sent my previous mail because I thought hypervisor maintainer missed
this patch.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 10/20/2017 07:15 PM, Wei Liu wrote:
> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
>> From: Razvan Cojocaru 
>>
>> For the default EPT view we have xc_set_mem_access_multi(), which
>> is able to set an array of pages to an array of access rights with
>> a single hypercall. However, this functionality was lacking for the
>> altp2m subsystem, which could only set page restrictions for one
>> page at a time. This patch addresses the gap.
>>
>> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
>> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
>> hence with the original altp2m design, where domains are allowed - with the
>> proper altp2m access rights - to alter these settings), in the absence of an
>> official position on the issue from the original altp2m designers.
>>
>> Signed-off-by: Razvan Cojocaru 
>> Signed-off-by: Petre Pircalabu 
>>
> 
> The title is a bit misleading -- this patch actually contains changes to
> hypervisor as well.

Sorry, I have assumed that the hypervisor changes are implied. We're
happy to change it. Would "x86/altp2m: Added
xc_altp2m_set_mem_access_multi() and hypervisor support" be better?


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Wei Liu]

On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote:
> From: Razvan Cojocaru 
> 
> For the default EPT view we have xc_set_mem_access_multi(), which
> is able to set an array of pages to an array of access rights with
> a single hypercall. However, this functionality was lacking for the
> altp2m subsystem, which could only set page restrictions for one
> page at a time. This patch addresses the gap.
> 
> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
> hence with the original altp2m design, where domains are allowed - with the
> proper altp2m access rights - to alter these settings), in the absence of an
> official position on the issue from the original altp2m designers.
> 
> Signed-off-by: Razvan Cojocaru 
> Signed-off-by: Petre Pircalabu 
> 

The title is a bit misleading -- this patch actually contains changes to
hypervisor as well.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel