Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-08-04 at 10:34 +0100, Wei Liu wrote:
> On Mon, Aug 03, 2015 at 06:05:43PM +0100, Andrew Cooper wrote:
> > It is not used, and can cause a spurious failure of the set_gdt() 
> > hypercall in
> > low memory situations.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > ---
> 
> This array does appear to be write-only and never gets read.
> 
> FWIW
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

The use was removed by:

commit 5889a3e1d123bdad4a3d150310d647db55459966
Author: Tim Deegan <tim@xen.org>
Date:   Thu May 17 10:24:54 2012 +0100

    x86/mm: Use get_page_from_gfn() instead of get_gfn()/put_gfn.
    
    Signed-off-by: Tim Deegan <tim@xen.org>
    Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

after it was introduced by:

commit 51032ca058e43fbd37ea1f7c7c003496f6451340
Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Date:   Fri Nov 11 18:11:34 2011 +0000

    Modify naming of queries into the p2m
    [...]
    Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
    Acked-by: Tim Deegan <tim@xen.org>
    Committed-by: Keir Fraser <keir@xen.org>

> It's obviously correct and fixes a problem, so it can be applied to 4.6
> tree.

FWIW I think so too, but I'm hesitant to apply without a maintainer(/Jan)'s
ack. I think this can probably wait until next week? 

> >  xen/arch/x86/mm.c |    9 +--------

George is the current "X86 MEMORY MANAGEMENT" maintainer, which is
xen/arch/x86/mm/*, should xen/arch/x86/mm.c be included there too?

> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 63aa666..4b76587 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4439,20 +4439,15 @@ long set_gdt(struct vcpu *v,
> >      l1_pgentry_t *pl1e;
> >      /* NB. There are 512 8-byte entries per GDT page. */
> >      int i, nr_pages = (entries + 511) / 512;
> > -    unsigned long *pfns;
> >  
> >      if ( entries > FIRST_RESERVED_GDT_ENTRY )
> >          return -EINVAL;
> >  
> > -    pfns = xmalloc_array(unsigned long, nr_pages);
> > -    if ( !pfns )
> > -        return -ENOMEM;
> > -
> >      /* Check the pages in the new GDT. */
> >      for ( i = 0; i < nr_pages; i++ )
> >      {
> >          struct page_info *page;
> > -        pfns[i] = frames[i];
> > +
> >          page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
> >          if ( !page )
> >              goto fail;
> > @@ -4476,7 +4471,6 @@ long set_gdt(struct vcpu *v,
> >          l1e_write(&pl1e[i], l1e_from_pfn(frames[i], 
> > __PAGE_HYPERVISOR_RW));
> >      }
> >  
> > -    xfree(pfns);
> >      return 0;
> >  
> >   fail:
> > @@ -4484,7 +4478,6 @@ long set_gdt(struct vcpu *v,
> >      {
> >          put_page_and_type(mfn_to_page(frames[i]));
> >      }
> > -    xfree(pfns);
> >      return -EINVAL;
> >  }
> >