[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 1/1] Add pci_hole_min_size

On 06/27/14 04:34, Jan Beulich wrote:
On 26.06.14 at 19:47, <dslutz@xxxxxxxxxxx> wrote:
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -53,7 +53,6 @@ extern struct bios_config ovmf_config;
  #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
/* MMIO hole: Hardcoded defaults, which can be dynamically expanded. */
-#define PCI_MEM_START       0xf0000000
  #define PCI_MEM_END         0xfc000000
This pair already shows that ... [skip next comment for continuation]

--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -28,9 +28,10 @@
  #include <xen/memory.h>
  #include <xen/hvm/ioreq.h>
  #include <xen/hvm/hvm_xs_strings.h>
+#include <xen/hvm/e820.h>
  #include <stdbool.h>
-unsigned long pci_mem_start = PCI_MEM_START;
+unsigned long pci_mem_start = HVM_BELOW_4G_RAM_END;
Perhaps (also related to the comment below) better to use
HVM_BELOW_4G_MMIO_START here, even if right now both evaluate
to the same number.

Will do.

@@ -237,6 +243,26 @@ void pci_setup(void)
          pci_writew(devfn, PCI_COMMAND, cmd);
+ if ( pci_hole_min_size )
+    {
+        uint64_t max_ram_below_4g = (1ULL << 32) - pci_hole_min_size;
... this isn't really correct: The space immediately below 4G is used for
MMIO unrelated to PCI. Hence, to not confuse people, I'd really like
the variable to be properly named (whether the xl config option should
also get renamed I'm not sure - the xl maintainers will know), i.e.

I think it is better to have at most 2 names.  So either
s/pci_hole_min_size/mmio_hole_min_size/ or switch to

+        if ( max_ram_below_4g >= HVM_BELOW_4G_RAM_END )
+        {
+            printf("max_ram_below_4g=0x"PRIllx
+                   " too big for pci_hole_min_size=0x"PRIllx"\n",
+                   PRIllx_arg(max_ram_below_4g),
+                   PRIllx_arg(pci_hole_min_size));
Probably worth making clear in the logged message that the option
therefore is being ignored?


+        }
+        else
+        {
+            pci_mem_start = max_ram_below_4g;
+            printf("pci_mem_start=0x%lx (was 0x%x) for 
+                   pci_mem_start, HVM_BELOW_4G_RAM_END,
+                   (long)pci_hole_min_size);
+        }
+    }
Furthermore the whole concept collides with the auto-growing of
the hole immediately following this code. Both logically (both having
a similar purpose) and, what's worse, functionally in that the
shifting done there requires that ((1ULL << 32) - pci_mem_start)
be a power of 2. An option might be to skip this auto-sizing if the
option was given.

I am fine with disabling the auto-growing (since as far as I know
this is bug #28 (

 #28 - support PCI hole resize in qemu-xen


Which this change is a kind of work around.

I would also drop the _min part of the name (i.e. use mmio_hole_size
or max_ram_below_4g).

    -Don Slutz


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.