* [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest @ 2014-07-23 20:09 Joel Schopp 2014-08-01 11:28 ` Peter Maydell 0 siblings, 1 reply; 6+ messages in thread From: Joel Schopp @ 2014-07-23 20:09 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell kvm_set_phys_mem doesn't work on arm64 with memory > 1GB. It exits with: kvm_set_phys_mem: error registering slot: Invalid argument An example of the failing address and size are start_addr == 0x90011000 and size=0xaffef000. As you can see both of these are 4K aligned, not 64K aligned. At 1024MB or smaller qemu only makes one call to kvm_set_user_memory_region, so the start_addr and size are aligned by accident and the bug doesn't happen. The following patch makes things work for me on an arm64 SOC. I also smoke tested the patch on an x86-64 box and qemu seemed to still run fine there with the patch applied. Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Joel Schopp <joel.schopp@amd.com> --- kvm-all.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kvm-all.c b/kvm-all.c index 1402f4f..1975862 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -618,14 +618,14 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add) /* kvm works in page size chunks, but the function may be called with sub-page size and unaligned start address. */ - delta = TARGET_PAGE_ALIGN(size) - size; + delta = HOST_PAGE_ALIGN(start_addr) - start_addr; if (delta > size) { return; } start_addr += delta; size -= delta; - size &= TARGET_PAGE_MASK; - if (!size || (start_addr & ~TARGET_PAGE_MASK)) { + size &= qemu_host_page_mask; + if (!size || (start_addr & ~qemu_host_page_mask)) { return; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest 2014-07-23 20:09 [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest Joel Schopp @ 2014-08-01 11:28 ` Peter Maydell 2014-08-01 11:41 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2014-08-01 11:28 UTC (permalink / raw) To: Joel Schopp; +Cc: Paolo Bonzini, QEMU Developers On 23 July 2014 21:09, Joel Schopp <joel.schopp@amd.com> wrote: > kvm_set_phys_mem doesn't work on arm64 with memory > 1GB. It exits with: > kvm_set_phys_mem: error registering slot: Invalid argument > > An example of the failing address and size are start_addr == 0x90011000 > and size=0xaffef000. As you can see both of these are 4K aligned, not > 64K aligned. > > At 1024MB or smaller qemu only makes one call to kvm_set_user_memory_region, > so the start_addr and size are aligned by accident and the bug doesn't happen. > > The following patch makes things work for me on an arm64 SOC. I also smoke > tested the patch on an x86-64 box and qemu seemed to still run fine there > with the patch applied. > > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Joel Schopp <joel.schopp@amd.com> > --- > kvm-all.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 1402f4f..1975862 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -618,14 +618,14 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add) > > /* kvm works in page size chunks, but the function may be called > with sub-page size and unaligned start address. */ > - delta = TARGET_PAGE_ALIGN(size) - size; > + delta = HOST_PAGE_ALIGN(start_addr) - start_addr; > if (delta > size) { > return; > } > start_addr += delta; > size -= delta; > - size &= TARGET_PAGE_MASK; > - if (!size || (start_addr & ~TARGET_PAGE_MASK)) { > + size &= qemu_host_page_mask; > + if (!size || (start_addr & ~qemu_host_page_mask)) { > return; > } > > Paolo: can you review this? Do we also need to do something about the use of TARGET_PAGE_BITS in kvm_physical_sync_dirty_bitmap? Is it really OK to define PAGE_SIZE to TARGET_PAGE_SIZE (it's certainly really misleading and suggests the kernel headers could be more helpful). Basically I think kvm-all.c should almost certainly not be using any of the TARGET_PAGE_* constants anywhere. thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest 2014-08-01 11:28 ` Peter Maydell @ 2014-08-01 11:41 ` Paolo Bonzini 2014-08-01 14:02 ` Joel Schopp 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2014-08-01 11:41 UTC (permalink / raw) To: Peter Maydell, Joel Schopp; +Cc: QEMU Developers Il 01/08/2014 13:28, Peter Maydell ha scritto: > Paolo: can you review this? Do we also need to do something > about the use of TARGET_PAGE_BITS in > kvm_physical_sync_dirty_bitmap? No, it should be the host page size, matching cpu_physical_memory_set_dirty_lebitmap. > Is it really OK to define > PAGE_SIZE to TARGET_PAGE_SIZE (it's certainly really > misleading and suggests the kernel headers could be more > helpful). No, it's wrong. > Basically I think kvm-all.c should almost certainly not be > using any of the TARGET_PAGE_* constants anywhere. I agree. I think the patch is right but, besides these considerations, does this bug still manifest itself after Andrew fixed the start address of the device at 0x90010000 (IIRC it was the pl031)? Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest 2014-08-01 11:41 ` Paolo Bonzini @ 2014-08-01 14:02 ` Joel Schopp 2014-08-01 14:19 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Joel Schopp @ 2014-08-01 14:02 UTC (permalink / raw) To: Paolo Bonzini, Peter Maydell; +Cc: QEMU Developers > I agree. > > I think the patch is right but, besides these considerations, does this > bug still manifest itself after Andrew fixed the start address of the > device at 0x90010000 (IIRC it was the pl031)? The device I see with that address is: hw/arm/virt.c: [VIRT_RTC] = { 0x90010000, 0x1000 }, The bug still manifests itself with that in the tree (without my patch applied). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest 2014-08-01 14:02 ` Joel Schopp @ 2014-08-01 14:19 ` Paolo Bonzini 2014-08-01 18:36 ` Joel Schopp 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2014-08-01 14:19 UTC (permalink / raw) To: Joel Schopp, Peter Maydell; +Cc: QEMU Developers Il 01/08/2014 16:02, Joel Schopp ha scritto: >> > >> > I think the patch is right but, besides these considerations, does this >> > bug still manifest itself after Andrew fixed the start address of the >> > device at 0x90010000 (IIRC it was the pl031)? > The device I see with that address is: > hw/arm/virt.c: [VIRT_RTC] = { 0x90010000, 0x1000 }, > > The bug still manifests itself with that in the tree (without my patch > applied). In 2.1-rc5 it is [VIRT_RTC] = { 0x9010000, 0x1000 }, with one zero less: commit 1373e140f0b0554a8b3aba9761cd96df49520f97 Author: Andrew Jones <drjones@redhat.com> Date: Tue Jul 29 18:32:01 2014 +0200 hw/arm/virt: fix pl031 addr typo pl031's base address should be 0x9010000, not 0x90010000, otherwise it sits in ram when configuring a guest with greater than 1G. Signed-off-by: Andrew Jones <drjones@redhat.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 405c61d..89532bd 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -104,7 +104,7 @@ static const MemMapEntry a15memmap[] = { [VIRT_GIC_DIST] = { 0x8000000, 0x10000 }, [VIRT_GIC_CPU] = { 0x8010000, 0x10000 }, [VIRT_UART] = { 0x9000000, 0x1000 }, - [VIRT_RTC] = { 0x90010000, 0x1000 }, + [VIRT_RTC] = { 0x9010000, 0x1000 }, [VIRT_MMIO] = { 0xa000000, 0x200 }, /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ /* 0x10000000 .. 0x40000000 reserved for PCI */ Paolo ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest 2014-08-01 14:19 ` Paolo Bonzini @ 2014-08-01 18:36 ` Joel Schopp 0 siblings, 0 replies; 6+ messages in thread From: Joel Schopp @ 2014-08-01 18:36 UTC (permalink / raw) To: Paolo Bonzini, Peter Maydell; +Cc: QEMU Developers On 08/01/2014 09:19 AM, Paolo Bonzini wrote: > Il 01/08/2014 16:02, Joel Schopp ha scritto: >>>> I think the patch is right but, besides these considerations, does this >>>> bug still manifest itself after Andrew fixed the start address of the >>>> device at 0x90010000 (IIRC it was the pl031)? >> The device I see with that address is: >> hw/arm/virt.c: [VIRT_RTC] = { 0x90010000, 0x1000 }, >> >> The bug still manifests itself with that in the tree (without my patch >> applied). > In 2.1-rc5 it is > > [VIRT_RTC] = { 0x9010000, 0x1000 }, > > with one zero less: > > commit 1373e140f0b0554a8b3aba9761cd96df49520f97 > Author: Andrew Jones <drjones@redhat.com> > Date: Tue Jul 29 18:32:01 2014 +0200 > > hw/arm/virt: fix pl031 addr typo > > pl031's base address should be 0x9010000, not 0x90010000, otherwise > it sits in ram when configuring a guest with greater than 1G. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 405c61d..89532bd 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -104,7 +104,7 @@ static const MemMapEntry a15memmap[] = { > [VIRT_GIC_DIST] = { 0x8000000, 0x10000 }, > [VIRT_GIC_CPU] = { 0x8010000, 0x10000 }, > [VIRT_UART] = { 0x9000000, 0x1000 }, > - [VIRT_RTC] = { 0x90010000, 0x1000 }, > + [VIRT_RTC] = { 0x9010000, 0x1000 }, > [VIRT_MMIO] = { 0xa000000, 0x200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ > /* 0x10000000 .. 0x40000000 reserved for PCI */ > > Paolo Retested with the latest master and this commit from Andrew did indeed resolve my issue. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-01 18:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-23 20:09 [Qemu-devel] [PATCH] arm64: 64K pages and > 1024MB guest Joel Schopp 2014-08-01 11:28 ` Peter Maydell 2014-08-01 11:41 ` Paolo Bonzini 2014-08-01 14:02 ` Joel Schopp 2014-08-01 14:19 ` Paolo Bonzini 2014-08-01 18:36 ` Joel Schopp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).