[Qemu-devel] spapr_pci.c:spapr_pci_msi_init() creates memory region whose size is host-dependent

[Qemu-devel] spapr_pci.c:spapr_pci_msi_init() creates memory region whose size is host-dependent

[Peter Maydell]

spapr_pci_msi_init() does this:

    memory_region_init_io(&spapr->msiwindow, NULL, &spapr_msi_ops, spapr,
                          "msi", getpagesize());

That means this device's memory region size will depend on
the host OS CPU and configuration, which seems like a bad idea,
especially if this machine is supposed to work with TCG.
It also means that on Win32 the compiler complains:

  CC    ppc64-softmmu/hw/ppc/spapr_pci.o
cc1: warnings being treated as errors
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c: In
function ‘spapr_pci_msi_init’:
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c:482:
warning: implicit declaration of function ‘getpagesize’
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c:482:
warning: nested extern declaration of ‘getpagesize’

since getpagesize() doesn't exist there.

Not sure which of the following is best:
 * use a fixed size for the memory region (eg "worst
   case page size for target CPU")
 * query the target CPU for its page size rather than the
   host OS/CPU
 * guard with suitable ifdefs if this code can't actually
   be used except with KVM
 * abstract out the "how do I find my page size on $OS?"
   check to an os-*.c file
 * something else

Any suggestions?

It would be nice to fix this because I think this is the last
Win32 compiler warning.

thanks
-- PMM

Re: [Qemu-devel] spapr_pci.c:spapr_pci_msi_init() creates memory region whose size is host-dependent

[Alexander Graf]

On 20.02.2014, at 20:58, Peter Maydell <peter.maydell@linaro.org> wrote:

> spapr_pci_msi_init() does this:
> 
>    memory_region_init_io(&spapr->msiwindow, NULL, &spapr_msi_ops, spapr,
>                          "msi", getpagesize());
> 
> That means this device's memory region size will depend on
> the host OS CPU and configuration, which seems like a bad idea,
> especially if this machine is supposed to work with TCG.
> It also means that on Win32 the compiler complains:
> 
>  CC    ppc64-softmmu/hw/ppc/spapr_pci.o
> cc1: warnings being treated as errors
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c: In
> function ‘spapr_pci_msi_init’:
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c:482:
> warning: implicit declaration of function ‘getpagesize’
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/ppc/spapr_pci.c:482:
> warning: nested extern declaration of ‘getpagesize’
> 
> since getpagesize() doesn't exist there.
> 
> Not sure which of the following is best:
> * use a fixed size for the memory region (eg "worst
>   case page size for target CPU")
> * query the target CPU for its page size rather than the
>   host OS/CPU
> * guard with suitable ifdefs if this code can't actually
>   be used except with KVM
> * abstract out the "how do I find my page size on $OS?"
>   check to an os-*.c file
> * something else
> 
> Any suggestions?

I think this should just be wrapped in a kvm special case and default to 4k otherwise (we can't use TARGET_PAGE_SIZE here, right?). That should get optimized out on win32 and fix your build :).

I'll post a proper patch any minute.


Alex

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 66ddf10..a3af75c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -469,6 +469,8 @@ static const MemoryRegionOps spapr_msi_ops = {

 void spapr_pci_msi_init(sPAPREnvironment *spapr, hwaddr addr)
 {
+    uint64_t window_size = 4096;
+
     /*
      * As MSI/MSIX interrupts trigger by writing at MSI/MSIX vectors,
      * we need to allocate some memory to catch those writes coming
@@ -476,10 +478,17 @@ void spapr_pci_msi_init(sPAPREnvironment *spapr, hwaddr addr)
      * As MSIMessage:addr is going to be the same and MSIMessage:data
      * is going to be a VIRQ number, 4 bytes of the MSI MR will only
      * be used.
+     *
+     * For KVM we want to ensure that this memory is a full page so that
+     * our memory slot is of page size granularity.
      */
+    if (kvm_enabled()) {
+        window_size = getpagesize();
+    }
+
     spapr->msi_win_addr = addr;
     memory_region_init_io(&spapr->msiwindow, NULL, &spapr_msi_ops, spapr,
-                          "msi", getpagesize());
+                          "msi", window_size);
     memory_region_add_subregion(get_system_memory(), spapr->msi_win_addr,
                                 &spapr->msiwindow);
 }