QEMU disaggregation

With this option, QEMU knows it's ID and can retrieve it's configuration from XenStore.

Quoted 2 lines · Julien Grall

With this option, QEMU knows it's ID and can retrieve it's configuration from XenStore.

Isn't this better modeled as a (Xen) machine option? I'd like to avoid more "special" command line switch proliferation.

Add interface for the new xen hypercalls

Signed-off-by Julien Grall <[email protected]>

---
 hw/xen.h   |    3 +++
 xen-all.c  |    2 ++
 xen-stub.c |   13 +++++++++++++
 3 files changed, 18 insertions(+), 0 deletions(-)

hw/xen.h +0 -0

index b056b13..a76616f 100644

QEMU will now register all memory range (PIO and MMIO) in Xen. We distinct two phases in memory registered :
- initialization
- running

For all range registered during the initialization, QEMU will check with XenStore if it is authorized to use them. After the initialization, QEMU can register all range. Indeed, the new ranges will be for PCI Bar.

Signed-off-by Julien Grall <[email protected]>

---
 exec.c    |    9 ++++++
 ioport.c  |   17 ++++++++++++
 xen-all.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 0 deletions(-)

exec.c +5 -0

index 780f63f..42d8c56 100644
@@ -3557,12 +3557,21 @@ static void core_commit(MemoryListener *listener)
 static void core_region_add(MemoryListener *listener,
                             MemoryRegionSection *section)
 {
+    if (xen_enabled()) {
+       xen_map_iorange(section->offset_within_address_space,
+                       section->size, 1);
+    }
+
     cpu_register_physical_memory_log(section, section->readonly);

exec.c +0 -0

 static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
 #include "ioport.h"
 #include "trace.h"
 #include "memory.h"
+#include "hw/xen.h"
 
 /***********************************************************/
 /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int length, int size,
                      i);
         ioport_opaque[i] = opaque;
     }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
     return 0;
 }
 

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int length, int size,
                      i);
         ioport_opaque[i] = opaque;
     }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
     return 0;
+
 }
 
 static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int length)
         ioport_destructor_table[start](ioport_opaque[start]);

Quoted 39 lines

static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
  #include "ioport.h"
  #include "trace.h"
  #include "memory.h"
+#include "hw/xen.h"
 
  /***********************************************************/
  /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
  }
 

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
+
  }
 

Quoted 4 lines

ioport.c +0 -0

  static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int length)
          ioport_destructor_table[start](ioport_opaque[start]);

ioport.c void isa_unassign_ioport(pio_addr_t start, int length) · line 282

Quoted 10 lines

ioport.c +5 -0

          ioport_destructor_table[start] = NULL;
      }
+
+    if (xen_enabled()) {
+        xen_unmap_iorange(start, length, 0);
+    }
+
      for(i = start; i<  start + length; i++) {
          ioport_read_table[0][i] = NULL;
          ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

Even if that is not yet powerful enough, tuning the hooks is usually better than open-coding.

Jan

Quoted 41 lines · Jan Kiszka

Quoted 41 lines

static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
  #include "ioport.h"
  #include "trace.h"
  #include "memory.h"
+#include "hw/xen.h"
 
  /***********************************************************/
  /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
  }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
+
  }
 

Quoted 17 lines

Quoted 13 lines

static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

ioport.c +5 -0

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
 length)
          ioport_destructor_table[start](ioport_opaque[start]);
          ioport_destructor_table[start] = NULL;
      }
+
+    if (xen_enabled()) {
+        xen_unmap_iorange(start, length, 0);
+    }
+
      for(i = start; i<  start + length; i++) {
          ioport_read_table[0][i] = NULL;

Quoted 2 lines

          ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

QEMU doesn't seem to call region_add/region_del for ioport. Moreover, some of ioport are directly register without using memory hook (for example cirrus vga).

What is the best way to do it ?

Quoted 63 lines · Julien Grall

Quoted 41 lines · Jan Kiszka

Quoted 41 lines

static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
  #include "ioport.h"
  #include "trace.h"
  #include "memory.h"
+#include "hw/xen.h"
 
  /***********************************************************/
  /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
  }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
+
  }
 

Quoted 17 lines

Quoted 13 lines

static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

ioport.c +5 -0

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
 length)
          ioport_destructor_table[start](ioport_opaque[start]);
          ioport_destructor_table[start] = NULL;
      }
+
+    if (xen_enabled()) {
+        xen_unmap_iorange(start, length, 0);
+    }
+
      for(i = start; i<  start + length; i++) {
          ioport_read_table[0][i] = NULL;

Quoted 2 lines

          ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

QEMU doesn't seem to call region_add/region_del for ioport. Moreover, some of ioport are directly register without using memory hook (for example cirrus vga).

Quoted 1 line

What is the best way to do it ?

I haven't looked at details. Maybe it is just a combination of "use case not yet considered, but can easily be added" and "need to switch legacy code to new scheme". Then this still remains the better option than this hook. Avi?

Quoted 66 lines · Jan Kiszka

Quoted 64 lines · Julien Grall

Quoted 41 lines · Jan Kiszka

Quoted 41 lines

static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
  #include "ioport.h"
  #include "trace.h"
  #include "memory.h"
+#include "hw/xen.h"
 
  /***********************************************************/
  /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
  }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
+
  }
 

Quoted 17 lines

Quoted 13 lines

static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

ioport.c +5 -0

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
 length)
          ioport_destructor_table[start](ioport_opaque[start]);
          ioport_destructor_table[start] = NULL;
      }
+
+    if (xen_enabled()) {
+        xen_unmap_iorange(start, length, 0);
+    }
+
      for(i = start; i<  start + length; i++) {
          ioport_read_table[0][i] = NULL;

Quoted 2 lines

          ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

QEMU doesn't seem to call region_add/region_del for ioport. Moreover, some of ioport are directly register without using memory hook (for example cirrus vga).

What is the best way to do it ?

Quoted 4 lines

I haven't looked at details. Maybe it is just a combination of "use case not yet considered, but can easily be added" and "need to switch legacy code to new scheme". Then this still remains the better option than this hook. Avi?

Just the second - region_add/del will be called, but only for ioports registered via the MemoryRegion APIs.

Quoted 67 lines · Avi Kivity

Quoted 66 lines · Jan Kiszka

Quoted 64 lines · Julien Grall

Quoted 41 lines · Jan Kiszka

Quoted 41 lines

static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
  #include "ioport.h"
  #include "trace.h"
  #include "memory.h"
+#include "hw/xen.h"
 
  /***********************************************************/
  /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
  }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
 length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
+
  }
 

Quoted 17 lines

Quoted 13 lines

static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

ioport.c +5 -0

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
 length)
          ioport_destructor_table[start](ioport_opaque[start]);
          ioport_destructor_table[start] = NULL;
      }
+
+    if (xen_enabled()) {
+        xen_unmap_iorange(start, length, 0);
+    }
+
      for(i = start; i<  start + length; i++) {
          ioport_read_table[0][i] = NULL;

Quoted 2 lines

          ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

QEMU doesn't seem to call region_add/region_del for ioport. Moreover, some of ioport are directly register without using memory hook (for example cirrus vga).

What is the best way to do it ?

Quoted 7 lines

Quoted 4 lines

I haven't looked at details. Maybe it is just a combination of "use case not yet considered, but can easily be added" and "need to switch legacy code to new scheme". Then this still remains the better option than this hook. Avi?

Just the second - region_add/del will be called, but only for ioports registered via the MemoryRegion APIs.

It looks like there are quite a few register_ioport_read/write left around, especially in the following files:

hw/acpi_piix4.c
hw/cirrus_vga.c
hw/serial.c
hw/pckbd.c
hw/pc.c

I guess they should all be converted to memory_region_init_io, right?

Quoted 90 lines · Stefano Stabellini

Quoted 71 lines · Avi Kivity

Quoted 69 lines · Jan Kiszka

Quoted 67 lines · Julien Grall

Quoted 49 lines · Jan Kiszka

Quoted 49 lines

static void core_region_nop(MemoryListener *listener,

ioport.c +12 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
   #include "ioport.h"
   #include "trace.h"
   #include "memory.h"
+#include "hw/xen.h"
 
   /***********************************************************/
   /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
 length, int size,
                        i);
           ioport_opaque[i] = opaque;
       }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
       return 0;
   }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
 length, int size,
                        i);
           ioport_opaque[i] = opaque;
       }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
       return 0;
+
   }
 

static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)

ioport.c +3 -0

@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
 length)
           ioport_destructor_table[start](ioport_opaque[start]);
           ioport_destructor_table[start] = NULL;
       }
+
+    if (xen_enabled()) {
+        xen_unmap_iorange(start, length, 0);

Quoted 10 lines

Quoted 7 lines

ioport.c +2 -0

+    }
+
       for(i = start; i<   start + length; i++) {
           ioport_read_table[0][i] = NULL;

           ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

QEMU doesn't seem to call region_add/region_del for ioport. Moreover, some of ioport are directly register without using memory hook (for example cirrus vga).

What is the best way to do it ?

Quoted 8 lines

Quoted 5 lines

I haven't looked at details. Maybe it is just a combination of "use case not yet considered, but can easily be added" and "need to switch legacy code to new scheme". Then this still remains the better option than this hook. Avi?

Just the second - region_add/del will be called, but only for ioports registered via the MemoryRegion APIs.

It looks like there are quite a few register_ioport_read/write left around, especially in the following files:

hw/acpi_piix4.c
hw/cirrus_vga.c
hw/serial.c
hw/pckbd.c
hw/pc.c

Quoted 2 lines

I guess they should all be converted to memory_region_init_io, right?

Right

Quoted 9 lines

Quoted 9 lines

It looks like there are quite a few register_ioport_read/write left around, especially in the following files:

hw/acpi_piix4.c
hw/cirrus_vga.c
hw/serial.c
hw/pckbd.c
hw/pc.c

Quoted 3 lines

Quoted 2 lines

I guess they should all be converted to memory_region_init_io, right?

Right

I will modify theses files and send a different patch series.

QEMU will now register all memory range (PIO and MMIO) in Xen. We distinct two phases in memory registered :
- initialization
- running

For all range registered during the initialization, QEMU will check with XenStore if it is authorized to use them. After the initialization, QEMU can register all range. Indeed, the new ranges will be for PCI Bar.

Signed-off-by Julien Grall<[email protected]>

---
  exec.c    |    9 ++++++
  ioport.c  |   17 ++++++++++++
  xen-all.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 109 insertions(+), 0 deletions(-)

exec.c +9 -0

index 780f63f..42d8c56 100644
@@ -3557,12 +3557,21 @@ static void core_commit(MemoryListener *listener)
  static void core_region_add(MemoryListener *listener,
                              MemoryRegionSection *section)
  {
+    if (xen_enabled()) {
+       xen_map_iorange(section->offset_within_address_space,
+                       section->size, 1);
+    }
+
      cpu_register_physical_memory_log(section, section->readonly);
  }
 
  static void core_region_del(MemoryListener *listener,
                              MemoryRegionSection *section)
  {
+    if (xen_enabled()) {
+       xen_unmap_iorange(section->offset_within_address_space,
+                       section->size, 1);
+    }
  }
 
  static void core_region_nop(MemoryListener *listener,

ioport.c +6 -0

index 78a3b89..073ed75 100644
@@ -28,6 +28,7 @@
  #include "ioport.h"
  #include "trace.h"
  #include "memory.h"
+#include "hw/xen.h"
 
  /***********************************************************/
  /* IO Port */

@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int length, int size,
                       i);
          ioport_opaque[i] = opaque;
      }
+
+    if (xen_enabled()) {
+        xen_map_iorange(start, length, 0);
+    }
+
      return 0;
  }
 

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int length, int size,
                       i);

QEMU will now register PCI in Xen. It will usefull to forward IO config space to the right QEMU.

Before to register a PCI device, QEMU will check with XenStore if it is autorized to register the PCI associate to a given BDF.

Signed-off-by Julien Grall <[email protected]>

---
 hw/pci.c  |    6 +++++
 xen-all.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

hw/pci.c +1 -0

index bf046bf..4df4449 100644
@@ -31,6 +31,7 @@
 #include "loader.h"
 #include "range.h"
 #include "qmp-commands.h"
+#include "xen.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

QEMU will now register PCI in Xen. It will usefull to forward IO config space to the right QEMU.

Before to register a PCI device, QEMU will check with XenStore if it is autorized to register the PCI associate to a given BDF.

Signed-off-by Julien Grall<[email protected]>

---
  hw/pci.c  |    6 +++++
  xen-all.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 76 insertions(+), 0 deletions(-)

hw/pci.c +6 -0

index bf046bf..4df4449 100644
@@ -31,6 +31,7 @@
  #include "loader.h"
  #include "range.h"
  #include "qmp-commands.h"
+#include "xen.h"
 
  //#define DEBUG_PCI
  #ifdef DEBUG_PCI

@@ -764,6 +765,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
      pci_dev->devfn = devfn;
      pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
      pci_dev->irq_state = 0;
+
+    if (xen_enabled()&&  xen_register_pcidev(pci_dev)) {
+        return NULL;
+    }
+
      pci_config_alloc(pci_dev);
 
      pci_config_set_vendor_id(pci_dev->config, pc->vendor_id);

xen-all.c +70 -0

index 366bafe..2f5405c 100644
@@ -107,6 +107,76 @@ void xen_piix3_set_irq(void *opaque, int irq_num, int level)
                                irq_num&  3, level);
  }
 
+static uint32_t str_to_bdf(const char *str)
+{
+    /* We assume that bdf is valid */
+    char *buf;
+    uint32_t bdf;
+    uint32_t tmp;
+
+    bdf = strtol(str,&buf, 16);
+    buf++;
+
+    str = buf;
+    tmp = strtol(str,&buf, 16);
+    bdf = (bdf<<  16) | (tmp<<  11);
+    buf++;
+
+    str = buf;
+    tmp = strtol(str,&buf, 16);
+    bdf = bdf | (tmp<<  8);
+
+    return bdf;
+}
+
+int xen_register_pcidev(PCIDevice *pci_dev)
+{
+    struct xs_handle *xs = NULL;
+    uint32_t bdf = 0;
+    uint32_t allowed_bdf = 0;
+    char **dir;
+    char path[50];
+    unsigned int nb;
+    unsigned int i;
+    unsigned int len;
+    char *str;
+    int rc = 0;
+
+
+    /* Fix : missing bus id to be more generic */
+    bdf |= pci_dev->devfn<<  8;
+
+    xs = xs_open(0);
+    if (!xs) {
+        fprintf(stderr, "pci_register: Unable to open xenstore\n");
+        return 1;
+    }
+
+    snprintf(path, sizeof (path), "/local/domain/%u/image/dms/%u/pci",
+             xen_domid, xen_dmid);
+
+    dir = xs_directory(xs, XBT_NULL, path,&nb);
+    if (dir) {
+        for (i = 0; i<  nb; i++) {
+            snprintf(path, sizeof (path), "/local/domain/%u/image/dms/%u/pci/%s",
+                     xen_domid, xen_dmid, dir[i]);
+            str = xs_read(xs, XBT_NULL, path,&len);
+            allowed_bdf = str_to_bdf(str);
+            free(str);
+            if (bdf == allowed_bdf)
+            {
+                rc = xc_hvm_register_pcidev(xen_xc, xen_domid, serverid, bdf);
+                break;
+            }
+        }
+        free(dir);
+    }
+
+    xs_close(xs);
+
+    return rc;
+}
+
  void xen_piix_pci_write_config_client(uint32_t address, uint32_t val, int len)
  {
      int i;

Quoted 7 lines · Julien Grall

QEMU will now register PCI in Xen. It will usefull to forward IO config space to the right QEMU.

Before to register a PCI device, QEMU will check with XenStore if it is autorized to register the PCI associate to a given BDF.

Signed-off-by Julien Grall<[email protected]>

As a general rule, any time you call to XenStore from QEMU, you're doing something wrong.

You should explicitly launch QEMU with the PCI devices that you want it to have.
If there are platform devices you don't want it to have, then you need to
construct a machine that lacks those devices.

Random hooks in non-Xen code that call into XenStore are always wrong.

Maybe the best thing to do is to have a set of machine specific options to select what devices need to be built in the machine.
Most devices already can be dynamically selected: NICs, usb, acpi, cirrus, etc.
We already have a Xen machine (xenfv_machine) that uses pc_init1 to initialize it.
We could have a simple bitmask to determine what devices need to be enabled, then in pc_init1 we could have something like:

if (devices & VGA_ENABLE) {
    pc_vga_init();
}

Given the number of enable variable already present in the code (pci_enabled, acpi_enabled, usb_enabled, xen_enabled,
cirrus_vga_enabled, ...), it might end up actually making the code more
readable. The flexibility could end up being useful in the generic case as well, for testing if nothing else.

We would still need to call xc_hvm_register_pcidev to register PCI devices in Xen, but we wouldn't expect to fail unless there was a misconfiguration somewhere. In that case we could just exit.

Now the problem is: there isn't a simple way to specify the BDF where you want to create the device; pci_create_simple takes a devfn but most of the higher level functions (pc_vga_init, pci_nic_init_nofail, ...) don't export the parameter at the moment.
We would need to be able to tell pc_vga_init where to create the card, so we would have to export the devfn as a parameter.

Quoted 25 lines · Stefano Stabellini

Maybe the best thing to do is to have a set of machine specific options to select what devices need to be built in the machine.
Most devices already can be dynamically selected: NICs, usb, acpi, cirrus, etc.
We already have a Xen machine (xenfv_machine) that uses pc_init1 to initialize it.
We could have a simple bitmask to determine what devices need to be enabled, then in pc_init1 we could have something like:

if (devices & VGA_ENABLE) {
    pc_vga_init();
}

Given the number of enable variable already present in the code (pci_enabled, acpi_enabled, usb_enabled, xen_enabled,
cirrus_vga_enabled, ...), it might end up actually making the code more
readable. The flexibility could end up being useful in the generic case as well, for testing if nothing else.

We would still need to call xc_hvm_register_pcidev to register PCI devices in Xen, but we wouldn't expect to fail unless there was a misconfiguration somewhere. In that case we could just exit.

Quoted 7 lines

Now the problem is: there isn't a simple way to specify the BDF where you want to create the device; pci_create_simple takes a devfn but most of the higher level functions (pc_vga_init, pci_nic_init_nofail, ...) don't export the parameter at the moment.
We would need to be able to tell pc_vga_init where to create the card, so we would have to export the devfn as a parameter.

You already have total flexibility with the -device foo parameter. It allows you to create any device, anywhere, with whatever configuration you want. Use in conjunction with -nodefconfig.

You may want your own host/pci bridge that lacks the device 0 configuration space.

Quoted 10 lines

Quoted 9 lines

Now the problem is: there isn't a simple way to specify the BDF where you want to create the device; pci_create_simple takes a devfn but most of the higher level functions (pc_vga_init, pci_nic_init_nofail, ...) don't export the parameter at the moment.
We would need to be able to tell pc_vga_init where to create the card, so we would have to export the devfn as a parameter.

Quoted 3 lines

You already have total flexibility with the -device foo parameter. It allows you to create any device, anywhere, with whatever configuration you want. Use in conjunction with -nodefconfig.

Thanks, -device looks exactly like what we need!
However I think that the option to suppress the defaults is -nodefaults.

Quoted 4 lines

You may want your own host/pci bridge that lacks the device 0 configuration space.

In order not to disrupt the emulated machine in QEMU too much, I was thinking to let QEMU create the default device 0 and device 1:

00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)

and then have only the first QEMU register itself for IO events in Xen related to these devices. That means that only the first QEMU would actually receive any events to handle while the other QEMUs would never receive any events for these devices.

Then everything else would go through -device: a device is created only if the command line option is passed and in that case QEMU also registers itself as the handler of this specific device in Xen.

There is supposed to be no overlaps in the configuration, so if two QEMUs both register for the same device Xen would return error and QEMU would exit.

The reason for doing this is that I am not sure that all OSes would be able to cope with the ISA bridge being at a location different than 00:01.0 or the IDE controller being on a different device from the ISA bridge, considering that they are supposed to be two functions of the same device (Intel PIIX southbridge).
So at that point we might as well leave them as they are and try to disrupt the basic config at little as possible.

Quoted 25 lines

Quoted 2 lines

You may want your own host/pci bridge that lacks the device 0 configuration space.

In order not to disrupt the emulated machine in QEMU too much, I was thinking to let QEMU create the default device 0 and device 1:

00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)

and then have only the first QEMU register itself for IO events in Xen related to these devices. That means that only the first QEMU would actually receive any events to handle while the other QEMUs would never receive any events for these devices.

Then everything else would go through -device: a device is created only if the command line option is passed and in that case QEMU also registers itself as the handler of this specific device in Xen.

There is supposed to be no overlaps in the configuration, so if two QEMUs both register for the same device Xen would return error and QEMU would exit.

Quoted 7 lines

The reason for doing this is that I am not sure that all OSes would be able to cope with the ISA bridge being at a location different than 00:01.0 or the IDE controller being on a different device from the ISA bridge, considering that they are supposed to be two functions of the same device (Intel PIIX southbridge).
So at that point we might as well leave them as they are and try to disrupt the basic config at little as possible.

Yes, but won't all qemus have those 00:01.0 devices and try to register for them?

What about if two BARs (from different devices) are configured for the same address ranges?

Quoted 33 lines · Avi Kivity

Quoted 25 lines

Quoted 2 lines

You may want your own host/pci bridge that lacks the device 0 configuration space.

In order not to disrupt the emulated machine in QEMU too much, I was thinking to let QEMU create the default device 0 and device 1:

00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)

and then have only the first QEMU register itself for IO events in Xen related to these devices. That means that only the first QEMU would actually receive any events to handle while the other QEMUs would never receive any events for these devices.

Then everything else would go through -device: a device is created only if the command line option is passed and in that case QEMU also registers itself as the handler of this specific device in Xen.

There is supposed to be no overlaps in the configuration, so if two QEMUs both register for the same device Xen would return error and QEMU would exit.

Quoted 7 lines

The reason for doing this is that I am not sure that all OSes would be able to cope with the ISA bridge being at a location different than 00:01.0 or the IDE controller being on a different device from the ISA bridge, considering that they are supposed to be two functions of the same device (Intel PIIX southbridge).
So at that point we might as well leave them as they are and try to disrupt the basic config at little as possible.

Quoted 2 lines

Yes, but won't all qemus have those 00:01.0 devices and try to register for them?

Yes, this is where it becomes ugly again.

One possibility would be to have a special option, maybe on xenstore, to tell QEMU "(do not)register North and South Bridge".
In the register_device callback that we'll have in xen-all.c, we'll have a brief list of "special devices" that we might want to ignore even if they are being emulated.
If this approach turns out to be too ugly from the code point of view, we might have to make 00:00.0 and 00:01.X configurable too.

Quoted 2 lines

What about if two BARs (from different devices) are configured for the same address ranges?

I think that it should have the same chance of happening as if there was just one QEMU, because from the guest OS and firmware POV the emulated hardware is the same. In any case Xen would return an error and QEMU can either exit or try to cope with it.

Quoted 7 lines

Quoted 2 lines

What about if two BARs (from different devices) are configured for the same address ranges?

I think that it should have the same chance of happening as if there was just one QEMU, because from the guest OS and firmware POV the emulated hardware is the same. In any case Xen would return an error and QEMU can either exit or try to cope with it.

How can qemu cope? In a normal situation it's aware of all the devices, here it's not aware of any device (except the one it is managing).

You're trying to convert a hierarchical problem into a flat one with no communication. What happens if one of the devices is a PCI-PCI bridge and it turns off its PCI window? The devices behind it should no longer respond, yet they know nothing about it.

I think you need to preserve the hierarchy. The host-pci bridge needs to talk to devices behind it, (as does a pci-pci bridge).

* Register QEMU in Xen as server
* Retrieve it's own shared pages
* Check if the page is already mapping before to populate

Signed-off-by Julien Grall <[email protected]>

---
 xen-all.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 59 insertions(+), 3 deletions(-)

xen-all.c +0 -0

index 2d001b8..6b7acd7 100644

xen-all.c +25 -0

+static inline unsigned long xen_buffered_iopage()
+{
+    unsigned long pfn;
+
+    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_PFN, &pfn);
+
+    return pfn;
+}
+
+static inline unsigned long xen_iopage(void)
+{
+    unsigned long pfn;
+
+    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_PFN, &pfn);
+
+    return pfn;
+}
+#else
+static inline unsigned long xen_buffered_iopage(void)
+{
+    unsigned long pfn;
+
+    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IO_PFN_FIRST, &pfn);
+    pfn += (serverid - 1) * 2 + 2;
+    return pfn;

xen-all.c +16 -3

     trace_xen_ram_alloc(ram_addr, size);
 
     nr_pfn = size >> TARGET_PAGE_BITS;

@@ -1046,7 +1089,14 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
         exit(1);
     }
 
-    snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
+    if (!xen_dmid) {
+        snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
+    }
+    else {
+        snprintf(path, sizeof (path), "/local/domain/0/dms/%u/%u/state",
+                 xen_domid, xen_dmid);
+    }
+
     if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
         fprintf(stderr, "error recording dm state\n");
         exit(1);

@@ -1077,6 +1127,7 @@ static void xen_change_state_handler(void *opaque, int running,
                                      RunState state)
 {
     if (running) {
+        is_running = 1;
         /* record state running */
         xenstore_record_dm_state(xenstore, "running");
     }

@@ -1137,7 +1188,12 @@ int xen_hvm_init(void)
     state->suspend.notify = xen_suspend_notifier;
     qemu_register_suspend_notifier(&state->suspend);
 
-    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn);
+    rc = xc_hvm_register_ioreq_server(xen_xc, xen_domid, &serverid);
+
+    if (rc)
+        hw_error("registered server returned error %d", rc);
+
+    ioreq_pfn = xen_iopage();
     DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
     state->shared_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE,
                                               PROT_READ|PROT_WRITE, ioreq_pfn);

@@ -1146,7 +1202,7 @@ int xen_hvm_init(void)
                  errno, xen_xc);
     }
 
-    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_PFN, &ioreq_pfn);
+    ioreq_pfn = xen_buffered_iopage();
     DPRINTF("buffered io page at pfn %lx\n", ioreq_pfn);
     state->buffered_io_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE,
                                                    PROT_READ|PROT_WRITE, ioreq_pfn);

Signature · 1 line

Julien Grall