| Message ID | 5948DA980200007800164510@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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,
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
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,
>>> 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
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,
--- 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);
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> 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);