From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH 2 of 4] x86: make the pv-only e820 array be dynamic Date: Wed, 13 Apr 2011 09:46:15 +0100 Message-ID: References: <20110412172140.GA13007@dumpdata.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="B_3385532782_92981266" Return-path: In-Reply-To: <20110412172140.GA13007@dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Konrad Rzeszutek Wilk Cc: xen-devel@lists.xensource.com, Ian Jackson , Ian.Campbell@citrix.com, Tim.Deegan@citrix.com List-Id: xen-devel@lists.xenproject.org > This message is in MIME format. Since your mail reader does not understand this format, some or all of this message may not be legible. --B_3385532782_92981266 Content-type: text/plain; charset="US-ASCII" Content-transfer-encoding: 7bit On 12/04/2011 18:21, "Konrad Rzeszutek Wilk" wrote: > On Tue, Apr 12, 2011 at 02:06:05PM +0100, Keir Fraser wrote: >> On 12/04/2011 13:53, "Konrad Rzeszutek Wilk" 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 Your patch is not really quite right. I've attached a modified version for you to try. -- Keir > (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 > (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) [] vmac+0x5da/0x927 > (XEN) [] copy_from_user+0x72/0x9f > (XEN) [] arch_memory_op+0x7cc/0xf05 > (XEN) [] do_memory_op+0x1dca/0x1df2 > (XEN) [] 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 > #include > #include > +#include > #include > #include > #include > @@ -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 { > + 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; > }; > --B_3385532782_92981266 Content-type: application/octet-stream; name="00-xen-dynamic-e820" Content-disposition: attachment; filename="00-xen-dynamic-e820" Content-transfer-encoding: base64 ZGlmZiAtciBhYjE1YjY4NTMzMDAgeGVuL2FyY2gveDg2L2RvbWFpbi5jCi0tLSBhL3hlbi9h cmNoL3g4Ni9kb21haW4uYwlXZWQgQXByIDEzIDA5OjE4OjEwIDIwMTEgKzAxMDAKKysrIGIv eGVuL2FyY2gveDg2L2RvbWFpbi5jCVdlZCBBcHIgMTMgMDk6NDQ6MjMgMjAxMSArMDEwMApA QCAtNjU3LDYgKzY1Nyw4IEBACiAgICAgICAgIC8qIDMyLWJpdCBQViBndWVzdCBieSBkZWZh dWx0IG9ubHkgaWYgWGVuIGlzIG5vdCA2NC1iaXQuICovCiAgICAgICAgIGQtPmFyY2guaXNf MzJiaXRfcHYgPSBkLT5hcmNoLmhhc18zMmJpdF9zaGluZm8gPQogICAgICAgICAgICAgKENP TkZJR19QQUdJTkdfTEVWRUxTICE9IDQpOworCisgICAgICAgIHNwaW5fbG9ja19pbml0KCZk LT5hcmNoLnB2X2RvbWFpbi5lODIwX2xvY2spOwogICAgIH0KIAogICAgIC8qIGluaXRpYWxp emUgZGVmYXVsdCB0c2MgYmVoYXZpb3IgaW4gY2FzZSB0b29scyBkb24ndCAqLwpAQCAtNjk2 LDYgKzY5OCw4IEBACiAKICAgICBpZiAoIGlzX2h2bV9kb21haW4oZCkgKQogICAgICAgICBo dm1fZG9tYWluX2Rlc3Ryb3koZCk7CisgICAgZWxzZQorICAgICAgICB4ZnJlZShkLT5hcmNo LnB2X2RvbWFpbi5lODIwKTsKIAogICAgIHZtY2VfZGVzdHJveV9tc3IoZCk7CiAgICAgcGNp X3JlbGVhc2VfZGV2aWNlcyhkKTsKZGlmZiAtciBhYjE1YjY4NTMzMDAgeGVuL2FyY2gveDg2 L21tLmMKLS0tIGEveGVuL2FyY2gveDg2L21tLmMJV2VkIEFwciAxMyAwOToxODoxMCAyMDEx ICswMTAwCisrKyBiL3hlbi9hcmNoL3g4Ni9tbS5jCVdlZCBBcHIgMTMgMDk6NDQ6MjMgMjAx MSArMDEwMApAQCAtMTAwLDYgKzEwMCw3IEBACiAjaW5jbHVkZSA8eGVuL2lvY2FwLmg+CiAj aW5jbHVkZSA8eGVuL2d1ZXN0X2FjY2Vzcy5oPgogI2luY2x1ZGUgPHhlbi9wZm4uaD4KKyNp bmNsdWRlIDx4ZW4veG1hbGxvYy5oPgogI2luY2x1ZGUgPGFzbS9wYWdpbmcuaD4KICNpbmNs dWRlIDxhc20vc2hhZG93Lmg+CiAjaW5jbHVkZSA8YXNtL3BhZ2UuaD4KQEAgLTQ3MTMsMTEg KzQ3MTQsMTIgQEAKICAgICB7CiAgICAgICAgIHN0cnVjdCB4ZW5fZm9yZWlnbl9tZW1vcnlf bWFwIGZtYXA7CiAgICAgICAgIHN0cnVjdCBkb21haW4gKmQ7CisgICAgICAgIHN0cnVjdCBl ODIwZW50cnkgKmU4MjA7CiAKICAgICAgICAgaWYgKCBjb3B5X2Zyb21fZ3Vlc3QoJmZtYXAs IGFyZywgMSkgKQogICAgICAgICAgICAgcmV0dXJuIC1FRkFVTFQ7CiAKLSAgICAgICAgaWYg KCBmbWFwLm1hcC5ucl9lbnRyaWVzID4gQVJSQVlfU0laRShkLT5hcmNoLnB2X2RvbWFpbi5l ODIwKSApCisgICAgICAgIGlmICggZm1hcC5tYXAubnJfZW50cmllcyA+IEU4MjBNQVggKQog ICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7CiAKICAgICAgICAgcmMgPSByY3VfbG9ja190 YXJnZXRfZG9tYWluX2J5X2lkKGZtYXAuZG9taWQsICZkKTsKQEAgLTQ3MzcsOSArNDczOSwy NSBAQAogICAgICAgICAgICAgcmV0dXJuIC1FUEVSTTsKICAgICAgICAgfQogCi0gICAgICAg IHJjID0gY29weV9mcm9tX2d1ZXN0KGQtPmFyY2gucHZfZG9tYWluLmU4MjAsIGZtYXAubWFw LmJ1ZmZlciwKLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZm1hcC5tYXAubnJfZW50 cmllcykgPyAtRUZBVUxUIDogMDsKKyAgICAgICAgZTgyMCA9IHhtYWxsb2NfYXJyYXkoZTgy MGVudHJ5X3QsIGZtYXAubWFwLm5yX2VudHJpZXMpOworICAgICAgICBpZiAoIGU4MjAgPT0g TlVMTCApCisgICAgICAgIHsKKyAgICAgICAgICAgIHJjdV91bmxvY2tfZG9tYWluKGQpOwor ICAgICAgICAgICAgcmV0dXJuIC1FTk9NRU07CisgICAgICAgIH0KKyAgICAgICAgCisgICAg ICAgIGlmICggY29weV9mcm9tX2d1ZXN0KGU4MjAsIGZtYXAubWFwLmJ1ZmZlciwgZm1hcC5t YXAubnJfZW50cmllcykgKQorICAgICAgICB7CisgICAgICAgICAgICB4ZnJlZShlODIwKTsK KyAgICAgICAgICAgIHJjdV91bmxvY2tfZG9tYWluKGQpOworICAgICAgICAgICAgcmV0dXJu IC1FRkFVTFQ7CisgICAgICAgIH0KKworICAgICAgICBzcGluX2xvY2soJmQtPmFyY2gucHZf ZG9tYWluLmU4MjBfbG9jayk7CisgICAgICAgIHhmcmVlKGQtPmFyY2gucHZfZG9tYWluLmU4 MjApOworICAgICAgICBkLT5hcmNoLnB2X2RvbWFpbi5lODIwID0gZTgyMDsKICAgICAgICAg ZC0+YXJjaC5wdl9kb21haW4ubnJfZTgyMCA9IGZtYXAubWFwLm5yX2VudHJpZXM7CisgICAg ICAgIHNwaW5fdW5sb2NrKCZkLT5hcmNoLnB2X2RvbWFpbi5lODIwX2xvY2spOwogCiAgICAg ICAgIHJjdV91bmxvY2tfZG9tYWluKGQpOwogICAgICAgICByZXR1cm4gcmM7CkBAIC00NzUw LDE5ICs0NzY4LDI5IEBACiAgICAgICAgIHN0cnVjdCB4ZW5fbWVtb3J5X21hcCBtYXA7CiAg ICAgICAgIHN0cnVjdCBkb21haW4gKmQgPSBjdXJyZW50LT5kb21haW47CiAKLSAgICAgICAg LyogQmFja3dhcmRzIGNvbXBhdGliaWxpdHkuICovCi0gICAgICAgIGlmICggZC0+YXJjaC5w dl9kb21haW4ubnJfZTgyMCA9PSAwICkKLSAgICAgICAgICAgIHJldHVybiAtRU5PU1lTOwot CiAgICAgICAgIGlmICggY29weV9mcm9tX2d1ZXN0KCZtYXAsIGFyZywgMSkgKQogICAgICAg ICAgICAgcmV0dXJuIC1FRkFVTFQ7CiAKKyAgICAgICAgc3Bpbl9sb2NrKCZkLT5hcmNoLnB2 X2RvbWFpbi5lODIwX2xvY2spOworCisgICAgICAgIC8qIEJhY2t3YXJkcyBjb21wYXRpYmls aXR5LiAqLworICAgICAgICBpZiAoIChkLT5hcmNoLnB2X2RvbWFpbi5ucl9lODIwID09IDAp IHx8CisgICAgICAgICAgICAgKGQtPmFyY2gucHZfZG9tYWluLmU4MjAgPT0gTlVMTCkgKQor ICAgICAgICB7CisgICAgICAgICAgICBzcGluX3VubG9jaygmZC0+YXJjaC5wdl9kb21haW4u ZTgyMF9sb2NrKTsKKyAgICAgICAgICAgIHJldHVybiAtRU5PU1lTOworICAgICAgICB9CisK ICAgICAgICAgbWFwLm5yX2VudHJpZXMgPSBtaW4obWFwLm5yX2VudHJpZXMsIGQtPmFyY2gu cHZfZG9tYWluLm5yX2U4MjApOwogICAgICAgICBpZiAoIGNvcHlfdG9fZ3Vlc3QobWFwLmJ1 ZmZlciwgZC0+YXJjaC5wdl9kb21haW4uZTgyMCwKICAgICAgICAgICAgICAgICAgICAgICAg ICAgIG1hcC5ucl9lbnRyaWVzKSB8fAogICAgICAgICAgICAgIGNvcHlfdG9fZ3Vlc3QoYXJn LCAmbWFwLCAxKSApCisgICAgICAgIHsKKyAgICAgICAgICAgIHNwaW5fdW5sb2NrKCZkLT5h cmNoLnB2X2RvbWFpbi5lODIwX2xvY2spOwogICAgICAgICAgICAgcmV0dXJuIC1FRkFVTFQ7 Ci0KKyAgICAgICAgfQorCisgICAgICAgIHNwaW5fdW5sb2NrKCZkLT5hcmNoLnB2X2RvbWFp bi5lODIwX2xvY2spOwogICAgICAgICByZXR1cm4gMDsKICAgICB9CiAKZGlmZiAtciBhYjE1 YjY4NTMzMDAgeGVuL2luY2x1ZGUvYXNtLXg4Ni9kb21haW4uaAotLS0gYS94ZW4vaW5jbHVk ZS9hc20teDg2L2RvbWFpbi5oCVdlZCBBcHIgMTMgMDk6MTg6MTAgMjAxMSArMDEwMAorKysg Yi94ZW4vaW5jbHVkZS9hc20teDg2L2RvbWFpbi5oCVdlZCBBcHIgMTMgMDk6NDQ6MjMgMjAx MSArMDEwMApAQCAtMjQxLDcgKzI0MSw4IEBACiAgICAgdW5zaWduZWQgbG9uZyBwaXJxX2Vv aV9tYXBfbWZuOwogCiAgICAgLyogUHNldWRvcGh5c2ljYWwgZTgyMCBtYXAgKFhFTk1FTV9t ZW1vcnlfbWFwKS4gICovCi0gICAgc3RydWN0IGU4MjBlbnRyeSBlODIwWzNdOworICAgIHNw aW5sb2NrX3QgZTgyMF9sb2NrOworICAgIHN0cnVjdCBlODIwZW50cnkgKmU4MjA7CiAgICAg dW5zaWduZWQgaW50IG5yX2U4MjA7CiB9OwogCg== --B_3385532782_92981266 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --B_3385532782_92981266--