>>> 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
|
Home