on the arm64 platform, the PAGESIZE is 64k, the default qxl rom
bar size is 8k(QXL_ROM_SZ), in the case memory size less than
one page size, kvm_align_section return zero,  the memory section
did not commit kvm.

Signed-off-by: Cong Liu <liucong2@kylinos.cn>
---
 accel/kvm/kvm-all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 27864dfaea..f57cab811b 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -318,6 +318,7 @@ static hwaddr kvm_align_section(MemoryRegionSection *section,
                                 hwaddr *start)
 {
     hwaddr size = int128_get64(section->size);
+    size = ROUND_UP(size, qemu_real_host_page_size);
     hwaddr delta, aligned;
 
     /* kvm works in page size chunks, but the function may be called
-- 
2.25.1

On Fri, 18 Mar 2022 at 14:24, Cong Liu <liucong2@kylinos.cn> wrote:
>
> on the arm64 platform, the PAGESIZE is 64k, the default qxl rom
> bar size is 8k(QXL_ROM_SZ), in the case memory size less than
> one page size, kvm_align_section return zero,  the memory section
> did not commit kvm.

Can you give more details on how this happens? The only place
we use QXL_ROM_SZ is in the qxl_rom_size() function, and that
rounds up the value it returns to the qemu_real_host_page_size.
That change was added in commit ce7015d9e8669e, exagctly to
fix what sounds like the same problem you're hitting where
KVM is in use and the host page size is larger than 8K.
Are you using an old version of QEMU that doesn't have that fix ?

> Signed-off-by: Cong Liu <liucong2@kylinos.cn>
> ---
>  accel/kvm/kvm-all.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 27864dfaea..f57cab811b 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -318,6 +318,7 @@ static hwaddr kvm_align_section(MemoryRegionSection *section,
>                                  hwaddr *start)
>  {
>      hwaddr size = int128_get64(section->size);
> +    size = ROUND_UP(size, qemu_real_host_page_size);
>      hwaddr delta, aligned;
>
>      /* kvm works in page size chunks, but the function may be called

The comment we can just see starting here says:

    /* kvm works in page size chunks, but the function may be called
       with sub-page size and unaligned start address. Pad the start
       address to next and truncate size to previous page boundary. */

but your change means that's no longer true.

More generally, rounding up the size here seems dubious -- there
is no guarantee that whatever follows the small lump of RAM
in the address space is sensible to treat as really being
part of the same thing.

thanks
-- PMM

On Fri, 25 Mar 2022 at 14:42, <liucong2@kylinos.cn> wrote:
> I found this issue on qmeu 4.2 with host linux 4.19, I want to
> use qxl on arm64. on arm64, default page size is 64k, and the
> qxl_rom_size is fixed 8192.

OK, so the fix to this is "use a newer QEMU".

> but when I read qxl_rom region in guest, guest os stopped and
> I can see error message "load/store instruction decodeing not
> implemented" in host side. it is because qxl rom bar memory
> region didn't commit to kvm.

> I only try qemu 6.0 rather than the latest version because
>
> I meet some compile issue. commit ce7015d9e8669e
>
> start v6.1.0-rc0, it will change the default qxl rom bar size
> to 64k on my platform. then my problem disappear. but when
> others create a memory region with the size less than one
> page. when it run into kvm_align_section, it return 0
> again.

This is correct behaviour. If the memory region is less than
a complete host page then it is not possible for KVM to
map it into the guest as directly accessible memory,
because that can only be done in host-page sized chunks,
and if the MR is a RAM region smaller than the page then
there simply is not enough backing RAM there to map without
incorrectly exposing to the guest whatever comes after the
contents of the MR.

For memory regions smaller than a page, KVM and QEMU will
fall back to "treat like MMIO device access". As long as the
guest is using simple load/store instructions to access the
memory region (ie loading or storing a single general
purpose register with no writeback, no acquire/release
semantics, no load-store exclusives) this will work fine.
KVM will drop out to QEMU, which will do the load or store
and return the data to KVM, which will simulate the instruction
execution and resume the guest.

If you see the message about "load/store instruction
decoding not implemented", that means the guest was trying
to access the region with something other than a simple
load/store. In this case you need to either:
 (1) change the device model to use a page-sized memory region
 (2) change the guest to use a simple load/store instruction
     to access it

Which of these is the right thing will depend on exactly
what the device and memory region is.

thanks
-- PMM

On 2022/3/25 23:00, Peter Maydell wrote:
> On Fri, 25 Mar 2022 at 14:42, <liucong2@kylinos.cn> wrote:
>> I found this issue on qmeu 4.2 with host linux 4.19, I want to
>> use qxl on arm64. on arm64, default page size is 64k, and the
>> qxl_rom_size is fixed 8192.
> 
> OK, so the fix to this is "use a newer QEMU".
> 
>> but when I read qxl_rom region in guest, guest os stopped and
>> I can see error message "load/store instruction decodeing not
>> implemented" in host side. it is because qxl rom bar memory
>> region didn't commit to kvm.
> 
>> I only try qemu 6.0 rather than the latest version because
>>
>> I meet some compile issue. commit ce7015d9e8669e
>>
>> start v6.1.0-rc0, it will change the default qxl rom bar size
>> to 64k on my platform. then my problem disappear. but when
>> others create a memory region with the size less than one
>> page. when it run into kvm_align_section, it return 0
>> again.
> 
> This is correct behaviour. If the memory region is less than
> a complete host page then it is not possible for KVM to
> map it into the guest as directly accessible memory,
> because that can only be done in host-page sized chunks,
> and if the MR is a RAM region smaller than the page then
> there simply is not enough backing RAM there to map without
> incorrectly exposing to the guest whatever comes after the
> contents of the MR.

actually, even with fixed 8192 qxl rom bar size, the RAMBlock
size corresponding to MemoryRegion will also be 64k. so it can
map into the guest as directly accessible memory. now it failed
just because we use the wrong size. ROUND_UP(n, d) requires
that d be a power of 2, it is faster than QEMU_ALIGN_UP().
and the qemu_real_host_page_size should always a power of 2.
seems we can use this patch and no need to fall back to "treat
like MMIO device access".

> 
> For memory regions smaller than a page, KVM and QEMU will
> fall back to "treat like MMIO device access". As long as the
I don't understand how it works, can you help explain or tell me
which part of the code I should read to understand?

I add some test code on qemu 6.2.0, add a new bar(rom_test) in
qxl with 2048 bytes. it followed with qxl rom bar. in the guest
kernel, map rom_test bar and printf rom_test->magic and rom_test->id.
this mr with size of 2048 bytes will not commit to kvm but I
still read the content I write in qemu side. So it should be the 
mechanism you mentioned that took effect.

the test code appended.
it works with some differences between arm64 and x86. in x86, it
printf rom_test->magic and rom_test->id correctly, but in arm64.
it printf rom_test->magic correctly. when I try to print the
rom_test->id. I get "load/store instruction decoding not
implemented" error message.

> guest is using simple load/store instructions to access the
> memory region (ie loading or storing a single general
> purpose register with no writeback, no acquire/release
> semantics, no load-store exclusives) this will work fine.
> KVM will drop out to QEMU, which will do the load or store
> and return the data to KVM, which will simulate the instruction
> execution and resume the guest.
> 
> If you see the message about "load/store instruction
> decoding not implemented", that means the guest was trying
> to access the region with something other than a simple
> load/store. In this case you need to either:
>   (1) change the device model to use a page-sized memory region
>   (2) change the guest to use a simple load/store instruction
>       to access it
>
> Which of these is the right thing will depend on exactly
> what the device and memory region is.
> 
> thanks
> -- PMM


+static void init_qxl_rom_test(PCIQXLDevice *d)
+{
+    QXLRom_test *rom = memory_region_get_ram_ptr(&d->rom_bar_test);
+
+    memset(rom, 0, 2048);
+
+    rom->magic      = cpu_to_le32(SPICE_MAGIC_CONST("abcd"));
+    rom->id         = cpu_to_le32(SPICE_MAGIC_CONST("ABCD"));
+}
+
  static void init_qxl_rom(PCIQXLDevice *d)
  {
      QXLRom *rom = memory_region_get_ram_ptr(&d->rom_bar);
@@ -2113,7 +2123,10 @@ static void qxl_realize_common(PCIQXLDevice *qxl, 
Error **errp)
      qxl->rom_size = qxl_rom_size();
      memory_region_init_rom(&qxl->rom_bar, OBJECT(qxl), "qxl.vrom",
                             qxl->rom_size, &error_fatal);
+    memory_region_init_rom(&qxl->rom_bar_test, OBJECT(qxl), 
"qxl.vrom_test",
+                           2048, &error_fatal);
      init_qxl_rom(qxl);
+    init_qxl_rom_test(qxl);
      init_qxl_ram(qxl);

      qxl->guest_surfaces.cmds = g_new0(QXLPHYSICAL, qxl->ssd.num_surfaces);
@@ -2136,6 +2149,9 @@ static void qxl_realize_common(PCIQXLDevice *qxl, 
Error **errp)
      pci_register_bar(&qxl->pci, QXL_ROM_RANGE_INDEX,
                       PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->rom_bar);

+    pci_register_bar(&qxl->pci, 4,
+                     PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->rom_bar_test);
+
      pci_register_bar(&qxl->pci, QXL_RAM_RANGE_INDEX,
                       PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->vga.vram);

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index 30d21f4d0b..3690edf17b 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -100,6 +100,7 @@ struct PCIQXLDevice {
      QXLModes           *modes;
      uint32_t           rom_size;
      MemoryRegion       rom_bar;
+    MemoryRegion       rom_bar_test;
  #if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
      uint16_t           max_outputs;
  #endif



Regards,
Cong.

On Mon, 28 Mar 2022 at 10:42, Cong Liu <liucong2@kylinos.cn> wrote:
> On 2022/3/25 23:00, Peter Maydell wrote:
> > This is correct behaviour. If the memory region is less than
> > a complete host page then it is not possible for KVM to
> > map it into the guest as directly accessible memory,
> > because that can only be done in host-page sized chunks,
> > and if the MR is a RAM region smaller than the page then
> > there simply is not enough backing RAM there to map without
> > incorrectly exposing to the guest whatever comes after the
> > contents of the MR.
>
> actually, even with fixed 8192 qxl rom bar size, the RAMBlock
> size corresponding to MemoryRegion will also be 64k.

Where does this rounding up happen? In any case, it would
still be wrong -- if the ROM bar is 8192 large then the
guest should get a fault writing to bytes past 8191, not
reads-as-written.

> so it can
> map into the guest as directly accessible memory. now it failed
> just because we use the wrong size. ROUND_UP(n, d) requires
> that d be a power of 2, it is faster than QEMU_ALIGN_UP().
> and the qemu_real_host_page_size should always a power of 2.
> seems we can use this patch and no need to fall back to "treat
> like MMIO device access".
>
> >
> > For memory regions smaller than a page, KVM and QEMU will
> > fall back to "treat like MMIO device access". As long as the

> I don't understand how it works, can you help explain or tell me
> which part of the code I should read to understand?

The KVM code in the kernel takes a fault because there is
nothing mapped at that address in the stage 2 page tables.
This results in kvm_handle_guest_abort() being called.
This function sorts out various cases it can handle
(eg "this is backed by host RAM which we need to page in")
and cases which are always errors (eg "the guest tried to
fetch an instruction from non-RAM"). For the cases of
"treat like MMIO device access" it calls io_mem_abort().
In io_mem_abort() we check whether the guest instruction that
did the load/store was a sensible one (this is the
kvm_vcpu_dabt_isvalid() check). Assuming that it was, then
we fill in some kvm_run struct fields with the parameters like
access size, address, etc (which the host CPU tells us in the
ESR_ELx syndrome register) cause an exit to userspace with
KVM_EXIT_MMIO as the reason.

In QEMU, the code in kvm_cpu_exec() has a case for the
KVM_EXIT_MMIO code. It just calls address_space_rw()
using the address, length, etc parameters that the kernel
gave us. If this is a load then the loaded data is filled
in in the kvm_run struct. Then it loops back around to do a
KVM_RUN ioctl, handing control back to the kernel.

In the kernel, in the arm64 kvm_arch_vcpu_ioctl_run()
we check whether we've just come back from a KVM_EXIT_MMIO
exit, and if so call kvm_handle_mmio_return(). If the
faulting instruction was a load, we read the data from
the kvm_run struct, sign extend as appropriate, and write
to the appropriate guest register. Then we increment the
guest program counter. Finally we start execution in the
guest in the normal way.

> the test code appended.
> it works with some differences between arm64 and x86. in x86, it
> printf rom_test->magic and rom_test->id correctly, but in arm64.
> it printf rom_test->magic correctly. when I try to print the
> rom_test->id. I get "load/store instruction decoding not
> implemented" error message.

You don't show the guest code, which is the thing that matters
here. In any case for the QXL ROM we already have the fix,
which is to make the ROM as big as the host page size.

-- PMM

On Mon, 28 Mar 2022 at 13:17, <liucong2@kylinos.cn> wrote:
>
> thanks for you explain, I will learn it later.
>
>
> in the scenario of rom bar size 8k, page size 64k, the value of
>
> 'size = ROUND_UP(size, qemu_real_host_page_size)' is 64k, kvm_align_section
>
> also return 64k bytes.  just the same size as the size of RAMBlock. I still
>
> did not understand why it is wrong.

Because if the size of the memory region is 8K, not 64K, then
we should not map into the guest 64K, but only 8K.

I'm still not sure what exactly you're trying to fix here.
The specific case of the QXL ROM BAR we already have the fix for.
Is there another specific case you are running into ?

-- PMM