[PATCH] memory: don't suppress P2M update in populate_physmap()

[PATCH] memory: don't suppress P2M update in populate_physmap()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

Commit d18627583d ("memory: don't hand MFN info to translated guests")
wrongly added a null-handle check there - just like stated in its
description for memory_exchange(), the array is also an input for
populate_physmap() (and hence can't reasonably be null). I have no idea
how I've managed to overlook this.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -248,8 +248,7 @@ static void populate_physmap(struct memo
 
             guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
 
-            if ( !paging_mode_translate(d) &&
-                 !guest_handle_is_null(a->extent_list) )
+            if ( !paging_mode_translate(d) )
             {
                 for ( j = 0; j < (1U << a->extent_order); j++ )
                     set_gpfn_from_mfn(mfn + j, gpfn + j);




[-- Attachment #2: memory-populate-set-p2m.patch --]
[-- Type: text/plain, Size: 925 bytes --]

memory: don't suppress P2M update in populate_physmap()

Commit d18627583d ("memory: don't hand MFN info to translated guests")
wrongly added a null-handle check there - just like stated in its
description for memory_exchange(), the array is also an input for
populate_physmap() (and hence can't reasonably be null). I have no idea
how I've managed to overlook this.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -248,8 +248,7 @@ static void populate_physmap(struct memo
 
             guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
 
-            if ( !paging_mode_translate(d) &&
-                 !guest_handle_is_null(a->extent_list) )
+            if ( !paging_mode_translate(d) )
             {
                 for ( j = 0; j < (1U << a->extent_order); j++ )
                     set_gpfn_from_mfn(mfn + j, gpfn + j);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] memory: don't suppress P2M update in populate_physmap()

[Andrew Cooper]

On 20/06/17 07:19, Jan Beulich wrote:
> Commit d18627583d ("memory: don't hand MFN info to translated guests")
> wrongly added a null-handle check there - just like stated in its
> description for memory_exchange(), the array is also an input for
> populate_physmap() (and hence can't reasonably be null). I have no idea
> how I've managed to overlook this.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH] memory: don't suppress P2M update in populate_physmap()

[Julien Grall]

Hi,

On 06/20/2017 09:37 AM, Andrew Cooper wrote:
> On 20/06/17 07:19, Jan Beulich wrote:
>> Commit d18627583d ("memory: don't hand MFN info to translated guests")
>> wrongly added a null-handle check there - just like stated in its
>> description for memory_exchange(), the array is also an input for
>> populate_physmap() (and hence can't reasonably be null). I have no idea
>> how I've managed to overlook this.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Am I correct that this is not a bug and only a pointless check?

Cheers,

-- 
Julien Grall

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

Re: [PATCH] memory: don't suppress P2M update in populate_physmap()

[Andrew Cooper]

On 20/06/17 13:39, Julien Grall wrote:
> Hi,
>
> On 06/20/2017 09:37 AM, Andrew Cooper wrote:
>> On 20/06/17 07:19, Jan Beulich wrote:
>>> Commit d18627583d ("memory: don't hand MFN info to translated guests")
>>> wrongly added a null-handle check there - just like stated in its
>>> description for memory_exchange(), the array is also an input for
>>> populate_physmap() (and hence can't reasonably be null). I have no idea
>>> how I've managed to overlook this.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Am I correct that this is not a bug and only a pointless check?

This is a partial reversion of d18627583d and needs to be included in
4.9, to avoid a regression.

~Andrew

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

Re: [PATCH] memory: don't suppress P2M update in populate_physmap()

[Julien Grall]

Hi,

On 06/20/2017 01:40 PM, Andrew Cooper wrote:
> On 20/06/17 13:39, Julien Grall wrote:
>> Hi,
>>
>> On 06/20/2017 09:37 AM, Andrew Cooper wrote:
>>> On 20/06/17 07:19, Jan Beulich wrote:
>>>> Commit d18627583d ("memory: don't hand MFN info to translated guests")
>>>> wrongly added a null-handle check there - just like stated in its
>>>> description for memory_exchange(), the array is also an input for
>>>> populate_physmap() (and hence can't reasonably be null). I have no idea
>>>> how I've managed to overlook this.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> Am I correct that this is not a bug and only a pointless check?
> 
> This is a partial reversion of d18627583d and needs to be included in
> 4.9, to avoid a regression.

Would you mind to explain why this would introduce regression? AFAICT 
the check is just redundant, so keeping it is not that bad.

Cheers,

-- 
Julien Grall

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

Re: [PATCH] memory: don't suppress P2M update in populate_physmap()

[Jan Beulich]

>>> On 20.06.17 at 14:51, <julien.grall@arm.com> wrote:
> On 06/20/2017 01:40 PM, Andrew Cooper wrote:
>> On 20/06/17 13:39, Julien Grall wrote:
>>> On 06/20/2017 09:37 AM, Andrew Cooper wrote:
>>>> On 20/06/17 07:19, Jan Beulich wrote:
>>>>> Commit d18627583d ("memory: don't hand MFN info to translated guests")
>>>>> wrongly added a null-handle check there - just like stated in its
>>>>> description for memory_exchange(), the array is also an input for
>>>>> populate_physmap() (and hence can't reasonably be null). I have no idea
>>>>> how I've managed to overlook this.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> Am I correct that this is not a bug and only a pointless check?
>> 
>> This is a partial reversion of d18627583d and needs to be included in
>> 4.9, to avoid a regression.
> 
> Would you mind to explain why this would introduce regression? AFAICT 
> the check is just redundant, so keeping it is not that bad.

Afaict there would be a regression only if someone invoked the
hypercall with a null handle (but having valid data at address zero).
Still I agree with Andrew that we'd better include this in 4.9.

Jan


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

Re: [PATCH] memory: don't suppress P2M update in populate_physmap()

[Julien Grall]

On 06/20/2017 02:03 PM, Jan Beulich wrote:
>>>> On 20.06.17 at 14:51, <julien.grall@arm.com> wrote:
>> On 06/20/2017 01:40 PM, Andrew Cooper wrote:
>>> On 20/06/17 13:39, Julien Grall wrote:
>>>> On 06/20/2017 09:37 AM, Andrew Cooper wrote:
>>>>> On 20/06/17 07:19, Jan Beulich wrote:
>>>>>> Commit d18627583d ("memory: don't hand MFN info to translated guests")
>>>>>> wrongly added a null-handle check there - just like stated in its
>>>>>> description for memory_exchange(), the array is also an input for
>>>>>> populate_physmap() (and hence can't reasonably be null). I have no idea
>>>>>> how I've managed to overlook this.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>> Am I correct that this is not a bug and only a pointless check?
>>>
>>> This is a partial reversion of d18627583d and needs to be included in
>>> 4.9, to avoid a regression.
>>
>> Would you mind to explain why this would introduce regression? AFAICT
>> the check is just redundant, so keeping it is not that bad.
> 
> Afaict there would be a regression only if someone invoked the
> hypercall with a null handle (but having valid data at address zero).
> Still I agree with Andrew that we'd better include this in 4.9.

Hmmm. It didn't occur to me that someone would put valid data at address 
zero.

Release-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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