WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 2 of 4] x86: make the pv-only e820 array be dynam

To: "Konrad Rzeszutek Wilk" <konrad.wilk@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2 of 4] x86: make the pv-only e820 array be dynamic
From: "Jan Beulich" <JBeulich@xxxxxxxxxx>
Date: Wed, 13 Apr 2011 08:34:20 +0100
Cc: Keir Fraser <keir@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>, Ian.Campbell@xxxxxxxxxx, Tim.Deegan@xxxxxxxxxx
Delivery-date: Wed, 13 Apr 2011 00:35:50 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110412172140.GA13007@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <20110412125317.GA19198@xxxxxxxxxxxx> <C9CA0CCD.2C923%keir@xxxxxxx> <20110412172140.GA13007@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>>> On 12.04.11 at 19:21, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> On Tue, Apr 12, 2011 at 02:06:05PM +0100, Keir Fraser wrote:
>> On 12/04/2011 13:53, "Konrad Rzeszutek Wilk" <konrad.wilk@xxxxxxxxxx> wrote:
>> 
>> >> How cunning.
>> >> 
>> >> Why wouldn't you just allocate exactly the right size of array in
>> >> XENMEM_set_memory_map?
>> > 
>> > I was thinking about it, but the mm.c code did not have the
>> > xen/xmalloc.h header, nor any references to xmalloc_array.
>> > 
>> > Is it OK to make an xmalloc_array during a hypercall?
>> 
>> Yes. I think the toolstack should be able to clean up on the newly-possible
>> ENOMEM return from this hypercall.
> 
> Hm, not sure what I am hitting, but I can't seem to be able to copy over the
> contents to the newly allocated array from the guest (this works
> fine with the previous version of the patch). This is what I get
> 
> (XEN) mm.c:4734:d0 We want 8 E820 entries. We have: 1.
> (XEN) mm.c:4754:d0 Copying E820 8 entries.
> (XEN) ----[ Xen-4.2-110412  : 00000000000000a0
> (XEN) rdx: 0000000000000000   rsi: 0000000000680004   rdi: 0000000000000000
> (XEN) rbp: ffff82c48028fc68   rsp: ffff82c48028fc58   r8:  ffff82c4802c8bd0
> (XEN) r9:  0000000000000004   r10: 0000000000000004   r11: 0000000000000001
> (XEN) r12: 00000000000000a0   r13: 0000000000000000   r14: 0000000000680004
> (XEN) r15: 0000000000000008   cr0: 0000000080050033   cr4: 00000000000026f0
> (XEN) cr3: 00000001056af000   cr2: 0000000000000000

De-referencing NULL, because of ...

> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
> (XEN) Xen stack trace from rsp=ffff82c48028fc58:
> (XEN)    00000000000000a0 00000000000000a0 ffff82c48028fc98 ffff82c48017fa7d
> (XEN)    0000000000000008 0000000000000000 0000000000000008 ffff88005c51d7c0
> (XEN)    ffff82c48028fd38 ffff82c480169857 ffff82c48028fcb8 ffff82c480167d9a
> (XEN)    000000008028fd08 ffff82f60214be50 0000000000000001 0000000000000008
> (XEN)    0000000000680004 0000000000000001 ffff88005d8aa1f8 ffff8301056e0000
> (XEN)    ffff83011f9b3000 0000000000000000 ffff82c48028fe28 ffff8300cf0fd000
> (XEN)    000000000000000d 00007fff2d629060 ffff88005c51d7c0 0000000000000008
> (XEN)    ffff82c48028fef8 ffff82c48011399c ffff82c4802c9940 000000008028ff18
> (XEN)    ffff82c48028fd88 0000000000123412 ffff82f602468250 ffff82c48028ff18
> (XEN)    ffff82c48028ff18 ffff82c4802c8e38 000000008028fde8 ffff82f602468250
> (XEN)    00000000001068f6 ffff83011f9b3000 0000000000000070 0000000123412000
> (XEN)    ffff82f602468240 ffff88005d8a9220 0000000000000000 0000000000682004
> (XEN)    ffff82c48028fe28 ffff82c48016aa14 8000000127365065 000000018015fd21
> (XEN)    0000000000000000 ffff8300cf0fd1c8 ffff8300cf0fd1c8 ffff82c48028fe88
> (XEN)    aaaaaaaaaaaaaaaa ffff88005d8a9220 ffff82c48028fef8 ffff82c480113cb7
> (XEN)    ffff8300cf0fd1d0 ffff8300cf0fd000 000000018028fef8 ffff82c48028ff18
> (XEN)    ffff82c48028fe68 ffff82c48028ff18 ffff8300cf0fd000 00007fff2d6290b0
> (XEN)    0000000000000001 0000000000000008 ffff82c48028fef8 ffff82c4802038c3
> (XEN)    0000000000680000 00007fb540012cd0 0000000000000000 0000000000000000
> (XEN)    00007fb540012e60 000000000000e033 ffff82c48028fed8 ffff8300cf0fd000
> (XEN) Xen call trace:
> (XEN)    [<ffff82c48020a3c5>] vmac+0x5da/0x927
> (XEN)    [<ffff82c48017fa7d>] copy_from_user+0x72/0x9f
> (XEN)    [<ffff82c480169857>] arch_memory_op+0x7cc/0xf05
> (XEN)    [<ffff82c48011399c>] do_memory_op+0x1dca/0x1df2
> (XEN)    [<ffff82c4802082c8>] syscall_enter+0xc8/0x122
> 
> And the patch is:
> 
> diff -r 97763efc41f9 xen/arch/x86/domain.c
> --- a/xen/arch/x86/domain.c   Tue Apr 05 18:23:54 2011 +0100
> +++ b/xen/arch/x86/domain.c   Tue Apr 12 13:18:34 2011 -0400
> @@ -696,6 +696,8 @@ void arch_domain_destroy(struct domain *
>  
>      if ( is_hvm_domain(d) )
>          hvm_domain_destroy(d);
> +    else
> +      xfree(d->arch.pv_domain.e820);
>  
>      vmce_destroy_msr(d);
>      pci_release_devices(d);
> diff -r 97763efc41f9 xen/arch/x86/mm.c
> --- a/xen/arch/x86/mm.c       Tue Apr 05 18:23:54 2011 +0100
> +++ b/xen/arch/x86/mm.c       Tue Apr 12 13:18:34 2011 -0400
> @@ -100,6 +100,7 @@
>  #include <xen/iocap.h>
>  #include <xen/guest_access.h>
>  #include <xen/pfn.h>
> +#include <xen/xmalloc.h>
>  #include <asm/paging.h>
>  #include <asm/shadow.h>
>  #include <asm/page.h>
> @@ -4710,7 +4711,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
>          if ( copy_from_guest(&fmap, arg, 1) )
>              return -EFAULT;
>  
> -        if ( fmap.map.nr_entries > ARRAY_SIZE(d->arch.pv_domain.e820) )
> +        if ( fmap.map.nr_entries > E820MAX )
>              return -EINVAL;
>  
>          rc = rcu_lock_target_domain_by_id(fmap.domid, &d);
> @@ -4730,9 +4731,39 @@ long arch_memory_op(int op, XEN_GUEST_HA
>              return -EPERM;
>          }
>  
> +        gdprintk(XENLOG_INFO, "We want %d E820 entries. We have: %d.\n", 
> fmap.map.nr_entries, d->arch.pv_domain.nr_e820);
> +        if ( d->arch.pv_domain.e820 )
> +        {
> +            if ( fmap.map.nr_entries >= d->arch.pv_domain.nr_e820 )
> +            {
> +                xfree(d->arch.pv_domain.e820);
> +                d->arch.pv_domain.e820 = NULL;
> +                d->arch.pv_domain.nr_e820 = 0;
> +            }
> +        } else {

... this "else". If you have an e820 table already, and it's too small,
you want to not only free it, but also allocate a new one. So
instead of an "else" you'll need a second "if" here, recovering from
the table possibly having got freed. Since xfree(NULL) is okay, you
could actually simply use the inner conditional on the first check.

Jan

> +            d->arch.pv_domain.e820 = xmalloc_array(e820entry_t,
> +                                                   fmap.map.nr_entries + 1);
> +            if ( !d->arch.pv_domain.e820 )
> +            {
> +                rcu_unlock_domain(d);
> +                return -ENOMEM;
> +            }
> +            gdprintk(XENLOG_INFO, "Allocated E820 for %d entries.\n", 
> fmap.map.nr_entries);
> +            memset(d->arch.pv_domain.e820, 0, (fmap.map.nr_entries + 1) * 
> sizeof(e820entry_t));
> +        }
> +        gdprintk(XENLOG_INFO, "Copying E820 %d entries.\n", 
> fmap.map.nr_entries);
>          rc = copy_from_guest(d->arch.pv_domain.e820, fmap.map.buffer,
>                               fmap.map.nr_entries) ? -EFAULT : 0;
> -        d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
> +
> +        if ( rc == 0 )
> +          d->arch.pv_domain.nr_e820 = fmap.map.nr_entries;
> +        else {
> +          /* Don't free the E820 if it had been set before and we failed. */
> +          if ( !d->arch.pv_domain.nr_e820 ) {
> +            xfree(d->arch.pv_domain.e820);
> +            d->arch.pv_domain.e820 = NULL;
> +          }
> +        }
>  
>          rcu_unlock_domain(d);
>          return rc;
> @@ -4747,6 +4778,9 @@ long arch_memory_op(int op, XEN_GUEST_HA
>          if ( d->arch.pv_domain.nr_e820 == 0 )
>              return -ENOSYS;
>  
> +        if ( d->arch.pv_domain.e820 == NULL )
> +            return -ENOSYS;
> +
>          if ( copy_from_guest(&map, arg, 1) )
>              return -EFAULT;
>  
> diff -r 97763efc41f9 xen/include/asm-x86/domain.h
> --- a/xen/include/asm-x86/domain.h    Tue Apr 05 18:23:54 2011 +0100
> +++ b/xen/include/asm-x86/domain.h    Tue Apr 12 13:18:34 2011 -0400
> @@ -238,7 +238,7 @@ struct pv_domain
>      unsigned long pirq_eoi_map_mfn;
>  
>      /* Pseudophysical e820 map (XENMEM_memory_map).  */
> -    struct e820entry e820[3];
> +    struct e820entry *e820;
>      unsigned int nr_e820;
>  };
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx 
> http://lists.xensource.com/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>