| Message ID | 20140723200925.9787.75225.stgit@joelaarch64.amd.com |
|---|---|
| State | New |
| Headers | show |
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
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
> 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).
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.
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; }
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(-)