qemu: make default_write_config use mask table

Add routines to manage PCI capability list. First user will be MSI-X.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h |   18 +++++++++++-
 2 files changed, 106 insertions(+), 10 deletions(-)

hw/pci.c +0 -1

index 361d741..ed011b5 100644
@@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
     int version = s->cap_present ? 3 : 2;
     int i;
 
-    qemu_put_be32(f, version); /* PCI device version */

hw/pci.c +0 -0

 int pci_device_load(PCIDevice *s, QEMUFile *f)

@@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     version_id = qemu_get_be32(f);
     if (version_id > 3)

hw/pci.c +4 -0

@@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     if (s->cap_present & ~s->cap_supported)
         return -EINVAL;
 
+    qemu_get_buffer(f, s->config, 256);
+    pci_update_mappings(s);
+
+    if (version_id >= 2)

hw/pci.c +19 -0

@@ -870,3 +877,76 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 
     return (PCIDevice *)dev;
 }
+
+static int pci_find_space(PCIDevice *pdev, uint8_t size)
+{
+    int offset = PCI_CONFIG_HEADER_SIZE;
+    int i;
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        if (pdev->used[i])
+            offset = i + 1;
+        else if (i - offset + 1 == size)
+            return offset;
+    return 0;
+}
+
+static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
+                                        uint8_t *prev_p)
+{
+    uint8_t next, prev;
+
+    if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))

hw/pci.c +44 -0

+
+/* Reserve space and add capability to the linked list in pci config space */
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t offset = pci_find_space(pdev, size);
+    uint8_t *config = pdev->config + offset;
+    if (!offset)
+        return -ENOSPC;
+    config[PCI_CAP_LIST_ID] = cap_id;
+    config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
+    pdev->config[PCI_CAPABILITY_LIST] = offset;
+    pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
+    memset(pdev->used + offset, 0xFF, size);
+    /* Make capability read-only by default */
+    memset(pdev->wmask + offset, 0, size);
+    return offset;
+}
+
+/* Unlink capability from the pci config space. */
+void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, &prev);
+    if (!offset)
+        return;
+    pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT];
+    /* Make capability writeable again */
+    memset(pdev->wmask + offset, 0xff, size);
+    memset(pdev->used + offset, 0, size);
+
+    if (!pdev->config[PCI_CAPABILITY_LIST])
+        pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
+}
+
+/* Reserve space for capability at a known offset (to call after load). */
+void pci_reserve_capability(PCIDevice *pdev, uint8_t offset, uint8_t size)
+{
+    memset(pdev->used + offset, 0xff, size);
+}
+
+uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
+{
+    uint8_t prev;
+    return pci_find_capability_list(pdev, cap_id, &prev);
+}

hw/pci.h +17 -1

index 6f0803f..4838c59 100644
@@ -123,6 +123,10 @@ typedef struct PCIIORegion {
 #define PCI_MIN_GNT		0x3e	/* 8 bits */
 #define PCI_MAX_LAT		0x3f	/* 8 bits */
 
+/* Capability lists */
+#define PCI_CAP_LIST_ID		0	/* Capability ID */
+#define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
+
 #define PCI_REVISION            0x08    /* obsolete, use PCI_REVISION_ID */
 #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
 #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */

@@ -130,7 +134,7 @@ typedef struct PCIIORegion {
 /* Bits in the PCI Status Register (PCI 2.3 spec) */
 #define PCI_STATUS_RESERVED1	0x007
 #define PCI_STATUS_INT_STATUS	0x008
-#define PCI_STATUS_CAPABILITIES	0x010
+#define PCI_STATUS_CAP_LIST	0x010
 #define PCI_STATUS_66MHZ	0x020
 #define PCI_STATUS_RESERVED2	0x040
 #define PCI_STATUS_FAST_BACK	0x080

@@ -160,6 +164,9 @@ struct PCIDevice {
     /* Used to implement R/W bytes */
     uint8_t wmask[PCI_CONFIG_SPACE_SIZE];
 
+    /* Used to allocate config space for capabilities. */
+    uint8_t used[PCI_CONFIG_SPACE_SIZE];
+
     /* the following fields are read only */
     PCIBus *bus;
     int devfn;

@@ -194,6 +201,15 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
                             uint32_t size, int type,
                             PCIMapIORegionFunc *map_func);
 
+int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+
+void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+
+void pci_reserve_capability(PCIDevice *pci_dev, uint8_t offset, uint8_t size);
+
+uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
+
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len);
 void pci_default_write_config(PCIDevice *d,

Signature · 1 line

1.6.3.1.56.g79e1.dirty

Quoted 17 lines · Michael S. Tsirkin

Add routines to manage PCI capability list. First user will be MSI-X.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h |   18 +++++++++++-
 2 files changed, 106 insertions(+), 10 deletions(-)

hw/pci.c +0 -1

index 361d741..ed011b5 100644
@@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
     int version = s->cap_present ? 3 : 2;
     int i;
 
-    qemu_put_be32(f, version); /* PCI device version */

hw/pci.c void pci_device_save(PCIDevice *s, QEMUFile *f) · line 140

Quoted 10 lines

hw/pci.c +4 -2

+    /* PCI device version and capabilities */
+    qemu_put_be32(f, version);
+    if (version >= 3)
+        qemu_put_be32(f, s->cap_present);
     qemu_put_buffer(f, s->config, 256);
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, s->irq_state[i]);
-    if (version >= 3)
-        qemu_put_be32(f, s->cap_present);
 }

What is it doing here?
You should just do it right in the first patch, instead of doing in one way there, and fixing here.

Quoted 5 lines

hw/pci.c +0 -0

 int pci_device_load(PCIDevice *s, QEMUFile *f)

@@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     version_id = qemu_get_be32(f);
     if (version_id > 3)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 152

Quoted 10 lines

hw/pci.c +0 -6

         return -EINVAL;
-    qemu_get_buffer(f, s->config, 256);
-    pci_update_mappings(s);
-
-    if (version_id >= 2)
-        for (i = 0; i < 4; i ++)
-            s->irq_state[i] = qemu_get_be32(f);
     if (version_id >= 3)
         s->cap_present = qemu_get_be32(f);
     else

ditto.

Quoted 4 lines

hw/pci.c +0 -0

@@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     if (s->cap_present & ~s->cap_supported)
         return -EINVAL;
 

Quoted 4 lines

hw/pci.c +4 -0

+    qemu_get_buffer(f, s->config, 256);
+    pci_update_mappings(s);
+
+    if (version_id >= 2)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 171

Quoted 10 lines

hw/pci.c +8 -0

+        for (i = 0; i < 4; i ++)
+            s->irq_state[i] = qemu_get_be32(f);
+    /* Clear wmask and used bits for capabilities.
+       Must be restored separately, since capabilities can
+       be placed anywhere in config space. */
+    memset(s->used, 0, PCI_CONFIG_SPACE_SIZE);
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        s->wmask[i] = 0xff;
     return 0;
 }

Sorry, I don't exactly understand it. Although it can be anywhere, what do we actually lose by keeping it at the same place in config space?

Quoted 24 lines

hw/pci.c +19 -0

@@ -870,3 +877,76 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 
     return (PCIDevice *)dev;
 }
+
+static int pci_find_space(PCIDevice *pdev, uint8_t size)
+{
+    int offset = PCI_CONFIG_HEADER_SIZE;
+    int i;
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        if (pdev->used[i])
+            offset = i + 1;
+        else if (i - offset + 1 == size)
+            return offset;
+    return 0;
+}
+
+static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
+                                        uint8_t *prev_p)
+{
+    uint8_t next, prev;
+
+    if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))

hw/pci.c PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name) · line 908

Quoted 10 lines

hw/pci.c +10 -0

+        return 0;
+
+    for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
+         prev = next + PCI_CAP_LIST_NEXT)
+        if (pdev->config[next + PCI_CAP_LIST_ID] == cap_id)
+            break;
+
+    *prev_p = prev;
+    return next;
+}

I'd prefer to do:

	if (prev_p)
		*prev_p = prev;

so we don't have to always pass a prev_p pointer. You have yourself a user where you don't need it in this very patch.

Quoted 99 lines

hw/pci.c +44 -0

+
+/* Reserve space and add capability to the linked list in pci config space */
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t offset = pci_find_space(pdev, size);
+    uint8_t *config = pdev->config + offset;
+    if (!offset)
+        return -ENOSPC;
+    config[PCI_CAP_LIST_ID] = cap_id;
+    config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
+    pdev->config[PCI_CAPABILITY_LIST] = offset;
+    pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
+    memset(pdev->used + offset, 0xFF, size);
+    /* Make capability read-only by default */
+    memset(pdev->wmask + offset, 0, size);
+    return offset;
+}
+
+/* Unlink capability from the pci config space. */
+void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, &prev);
+    if (!offset)
+        return;
+    pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT];
+    /* Make capability writeable again */
+    memset(pdev->wmask + offset, 0xff, size);
+    memset(pdev->used + offset, 0, size);
+
+    if (!pdev->config[PCI_CAPABILITY_LIST])
+        pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
+}
+
+/* Reserve space for capability at a known offset (to call after load). */
+void pci_reserve_capability(PCIDevice *pdev, uint8_t offset, uint8_t size)
+{
+    memset(pdev->used + offset, 0xff, size);
+}
+
+uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
+{
+    uint8_t prev;
+    return pci_find_capability_list(pdev, cap_id, &prev);
+}

hw/pci.h +17 -1

index 6f0803f..4838c59 100644
@@ -123,6 +123,10 @@ typedef struct PCIIORegion {
 #define PCI_MIN_GNT		0x3e	/* 8 bits */
 #define PCI_MAX_LAT		0x3f	/* 8 bits */
 
+/* Capability lists */
+#define PCI_CAP_LIST_ID		0	/* Capability ID */
+#define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
+
 #define PCI_REVISION            0x08    /* obsolete, use PCI_REVISION_ID */
 #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
 #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */

@@ -130,7 +134,7 @@ typedef struct PCIIORegion {
 /* Bits in the PCI Status Register (PCI 2.3 spec) */
 #define PCI_STATUS_RESERVED1	0x007
 #define PCI_STATUS_INT_STATUS	0x008
-#define PCI_STATUS_CAPABILITIES	0x010
+#define PCI_STATUS_CAP_LIST	0x010
 #define PCI_STATUS_66MHZ	0x020
 #define PCI_STATUS_RESERVED2	0x040
 #define PCI_STATUS_FAST_BACK	0x080

@@ -160,6 +164,9 @@ struct PCIDevice {
     /* Used to implement R/W bytes */
     uint8_t wmask[PCI_CONFIG_SPACE_SIZE];
 
+    /* Used to allocate config space for capabilities. */
+    uint8_t used[PCI_CONFIG_SPACE_SIZE];
+
     /* the following fields are read only */
     PCIBus *bus;
     int devfn;

@@ -194,6 +201,15 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
                             uint32_t size, int type,
                             PCIMapIORegionFunc *map_func);
 
+int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+
+void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+
+void pci_reserve_capability(PCIDevice *pci_dev, uint8_t offset, uint8_t size);
+
+uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
+
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len);
 void pci_default_write_config(PCIDevice *d,

Signature · 1 line

1.6.3.1.56.g79e1.dirty

Quoted 17 lines · Michael S. Tsirkin

Add routines to manage PCI capability list. First user will be MSI-X.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h |   18 +++++++++++-
 2 files changed, 106 insertions(+), 10 deletions(-)

hw/pci.c +0 -1

index 361d741..ed011b5 100644
@@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
     int version = s->cap_present ? 3 : 2;
     int i;
 
-    qemu_put_be32(f, version); /* PCI device version */

hw/pci.c void pci_device_save(PCIDevice *s, QEMUFile *f) · line 140

Quoted 10 lines

hw/pci.c +4 -2

+    /* PCI device version and capabilities */
+    qemu_put_be32(f, version);
+    if (version >= 3)
+        qemu_put_be32(f, s->cap_present);
     qemu_put_buffer(f, s->config, 256);
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, s->irq_state[i]);
-    if (version >= 3)
-        qemu_put_be32(f, s->cap_present);
 }

What is it doing here?
You should just do it right in the first patch, instead of doing in one way there, and fixing here.

Quoted 5 lines

hw/pci.c +0 -0

 int pci_device_load(PCIDevice *s, QEMUFile *f)

@@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     version_id = qemu_get_be32(f);
     if (version_id > 3)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 152

Quoted 10 lines

hw/pci.c +0 -6

         return -EINVAL;
-    qemu_get_buffer(f, s->config, 256);
-    pci_update_mappings(s);
-
-    if (version_id >= 2)
-        for (i = 0; i < 4; i ++)
-            s->irq_state[i] = qemu_get_be32(f);
     if (version_id >= 3)
         s->cap_present = qemu_get_be32(f);
     else

ditto.

Quoted 4 lines

hw/pci.c +0 -0

@@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     if (s->cap_present & ~s->cap_supported)
         return -EINVAL;
 

Quoted 4 lines

hw/pci.c +4 -0

+    qemu_get_buffer(f, s->config, 256);
+    pci_update_mappings(s);
+
+    if (version_id >= 2)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 171

Quoted 10 lines

hw/pci.c +8 -0

+        for (i = 0; i < 4; i ++)
+            s->irq_state[i] = qemu_get_be32(f);
+    /* Clear wmask and used bits for capabilities.
+       Must be restored separately, since capabilities can
+       be placed anywhere in config space. */
+    memset(s->used, 0, PCI_CONFIG_SPACE_SIZE);
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        s->wmask[i] = 0xff;
     return 0;
 }

Sorry, I don't exactly understand it. Although it can be anywhere, what do we actually lose by keeping it at the same place in config space?

Quoted 24 lines

hw/pci.c +19 -0

@@ -870,3 +877,76 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 
     return (PCIDevice *)dev;
 }
+
+static int pci_find_space(PCIDevice *pdev, uint8_t size)
+{
+    int offset = PCI_CONFIG_HEADER_SIZE;
+    int i;
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        if (pdev->used[i])
+            offset = i + 1;
+        else if (i - offset + 1 == size)
+            return offset;
+    return 0;
+}
+
+static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
+                                        uint8_t *prev_p)
+{
+    uint8_t next, prev;
+
+    if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))

hw/pci.c PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name) · line 908

Quoted 10 lines

hw/pci.c +10 -0

+        return 0;
+
+    for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
+         prev = next + PCI_CAP_LIST_NEXT)
+        if (pdev->config[next + PCI_CAP_LIST_ID] == cap_id)
+            break;
+
+    *prev_p = prev;
+    return next;
+}

I'd prefer to do:

	if (prev_p)
		*prev_p = prev;

so we don't have to always pass a prev_p pointer. You have yourself a user where you don't need it in this very patch.

Quoted 99 lines

hw/pci.c +44 -0

+
+/* Reserve space and add capability to the linked list in pci config space */
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t offset = pci_find_space(pdev, size);
+    uint8_t *config = pdev->config + offset;
+    if (!offset)
+        return -ENOSPC;
+    config[PCI_CAP_LIST_ID] = cap_id;
+    config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
+    pdev->config[PCI_CAPABILITY_LIST] = offset;
+    pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
+    memset(pdev->used + offset, 0xFF, size);
+    /* Make capability read-only by default */
+    memset(pdev->wmask + offset, 0, size);
+    return offset;
+}
+
+/* Unlink capability from the pci config space. */
+void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t prev, offset = pci_find_capability_list(pdev, cap_id, &prev);
+    if (!offset)
+        return;
+    pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT];
+    /* Make capability writeable again */
+    memset(pdev->wmask + offset, 0xff, size);
+    memset(pdev->used + offset, 0, size);
+
+    if (!pdev->config[PCI_CAPABILITY_LIST])
+        pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
+}
+
+/* Reserve space for capability at a known offset (to call after load). */
+void pci_reserve_capability(PCIDevice *pdev, uint8_t offset, uint8_t size)
+{
+    memset(pdev->used + offset, 0xff, size);
+}
+
+uint8_t pci_find_capability(PCIDevice *pdev, uint8_t cap_id)
+{
+    uint8_t prev;
+    return pci_find_capability_list(pdev, cap_id, &prev);
+}

hw/pci.h +17 -1

index 6f0803f..4838c59 100644
@@ -123,6 +123,10 @@ typedef struct PCIIORegion {
 #define PCI_MIN_GNT		0x3e	/* 8 bits */
 #define PCI_MAX_LAT		0x3f	/* 8 bits */
 
+/* Capability lists */
+#define PCI_CAP_LIST_ID		0	/* Capability ID */
+#define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
+
 #define PCI_REVISION            0x08    /* obsolete, use PCI_REVISION_ID */
 #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
 #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */

@@ -130,7 +134,7 @@ typedef struct PCIIORegion {
 /* Bits in the PCI Status Register (PCI 2.3 spec) */
 #define PCI_STATUS_RESERVED1	0x007
 #define PCI_STATUS_INT_STATUS	0x008
-#define PCI_STATUS_CAPABILITIES	0x010
+#define PCI_STATUS_CAP_LIST	0x010
 #define PCI_STATUS_66MHZ	0x020
 #define PCI_STATUS_RESERVED2	0x040
 #define PCI_STATUS_FAST_BACK	0x080

@@ -160,6 +164,9 @@ struct PCIDevice {
     /* Used to implement R/W bytes */
     uint8_t wmask[PCI_CONFIG_SPACE_SIZE];
 
+    /* Used to allocate config space for capabilities. */
+    uint8_t used[PCI_CONFIG_SPACE_SIZE];
+
     /* the following fields are read only */
     PCIBus *bus;
     int devfn;

@@ -194,6 +201,15 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
                             uint32_t size, int type,
                             PCIMapIORegionFunc *map_func);
 
+int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+
+void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+
+void pci_reserve_capability(PCIDevice *pci_dev, uint8_t offset, uint8_t size);
+
+uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
+
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len);
 void pci_default_write_config(PCIDevice *d,

Signature · 1 line

1.6.3.1.56.g79e1.dirty

Quoted 52 lines · Glauber Costa

Quoted 17 lines · Michael S. Tsirkin

Add routines to manage PCI capability list. First user will be MSI-X.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h |   18 +++++++++++-
 2 files changed, 106 insertions(+), 10 deletions(-)

hw/pci.c +0 -1

index 361d741..ed011b5 100644
@@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
     int version = s->cap_present ? 3 : 2;
     int i;
 
-    qemu_put_be32(f, version); /* PCI device version */

hw/pci.c void pci_device_save(PCIDevice *s, QEMUFile *f) · line 140

Quoted 10 lines

hw/pci.c +4 -2

+    /* PCI device version and capabilities */
+    qemu_put_be32(f, version);
+    if (version >= 3)
+        qemu_put_be32(f, s->cap_present);
     qemu_put_buffer(f, s->config, 256);
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, s->irq_state[i]);
-    if (version >= 3)
-        qemu_put_be32(f, s->cap_present);
 }

What is it doing here?
You should just do it right in the first patch, instead of doing in one way there, and fixing here.

Quoted 5 lines

hw/pci.c +0 -0

 int pci_device_load(PCIDevice *s, QEMUFile *f)

@@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     version_id = qemu_get_be32(f);
     if (version_id > 3)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 152

Quoted 10 lines

hw/pci.c +0 -6

         return -EINVAL;
-    qemu_get_buffer(f, s->config, 256);
-    pci_update_mappings(s);
-
-    if (version_id >= 2)
-        for (i = 0; i < 4; i ++)
-            s->irq_state[i] = qemu_get_be32(f);
     if (version_id >= 3)
         s->cap_present = qemu_get_be32(f);
     else

ditto.

Quoted 4 lines

hw/pci.c +0 -0

@@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     if (s->cap_present & ~s->cap_supported)
         return -EINVAL;
 

Quoted 16 lines

Quoted 4 lines

hw/pci.c +4 -0

+    qemu_get_buffer(f, s->config, 256);
+    pci_update_mappings(s);
+
+    if (version_id >= 2)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 171

Quoted 10 lines

hw/pci.c +8 -0

+        for (i = 0; i < 4; i ++)
+            s->irq_state[i] = qemu_get_be32(f);
+    /* Clear wmask and used bits for capabilities.
+       Must be restored separately, since capabilities can
+       be placed anywhere in config space. */
+    memset(s->used, 0, PCI_CONFIG_SPACE_SIZE);
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        s->wmask[i] = 0xff;
     return 0;
 }

Sorry, I don't exactly understand it. Although it can be anywhere, what do we actually lose by keeping it at the same place in config space?

We lose the ability to let user control the capabilities exposed by the device.

And generally, I dislike arbitrary limitations. The PCI spec says the capability can be anywhere, implementing a linked list of caps is simple enough to not invent abritrary restrictions.

Quoted 70 lines · Michael S. Tsirkin

Quoted 68 lines · Glauber Costa

Quoted 17 lines · Michael S. Tsirkin

Add routines to manage PCI capability list. First user will be MSI-X.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h |   18 +++++++++++-
 2 files changed, 106 insertions(+), 10 deletions(-)

hw/pci.c +0 -1

index 361d741..ed011b5 100644
@@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
     int version = s->cap_present ? 3 : 2;
     int i;
 
-    qemu_put_be32(f, version); /* PCI device version */

hw/pci.c void pci_device_save(PCIDevice *s, QEMUFile *f) · line 140

Quoted 10 lines

hw/pci.c +4 -2

+    /* PCI device version and capabilities */
+    qemu_put_be32(f, version);
+    if (version >= 3)
+        qemu_put_be32(f, s->cap_present);
     qemu_put_buffer(f, s->config, 256);
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, s->irq_state[i]);
-    if (version >= 3)
-        qemu_put_be32(f, s->cap_present);
 }

What is it doing here?
You should just do it right in the first patch, instead of doing in one way there, and fixing here.

Quoted 5 lines

hw/pci.c +0 -0

 int pci_device_load(PCIDevice *s, QEMUFile *f)

@@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     version_id = qemu_get_be32(f);
     if (version_id > 3)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 152

Quoted 10 lines

hw/pci.c +0 -6

         return -EINVAL;
-    qemu_get_buffer(f, s->config, 256);
-    pci_update_mappings(s);
-
-    if (version_id >= 2)
-        for (i = 0; i < 4; i ++)
-            s->irq_state[i] = qemu_get_be32(f);
     if (version_id >= 3)
         s->cap_present = qemu_get_be32(f);
     else

ditto.

Quoted 8 lines

hw/pci.c +4 -0

@@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     if (s->cap_present & ~s->cap_supported)
         return -EINVAL;
 
+    qemu_get_buffer(f, s->config, 256);
+    pci_update_mappings(s);
+
+    if (version_id >= 2)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 171

Quoted 10 lines

hw/pci.c +8 -0

+        for (i = 0; i < 4; i ++)
+            s->irq_state[i] = qemu_get_be32(f);
+    /* Clear wmask and used bits for capabilities.
+       Must be restored separately, since capabilities can
+       be placed anywhere in config space. */
+    memset(s->used, 0, PCI_CONFIG_SPACE_SIZE);
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        s->wmask[i] = 0xff;
     return 0;
 }

Sorry, I don't exactly understand it. Although it can be anywhere, what do we actually lose by keeping it at the same place in config space?

Quoted 6 lines

We lose the ability to let user control the capabilities exposed by the device.

And generally, I dislike arbitrary limitations. The PCI spec says the capability can be anywhere, implementing a linked list of caps is simple enough to not invent abritrary restrictions.

yes, but this is migration time, right?

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Quoted 70 lines · Michael S. Tsirkin

Quoted 68 lines · Glauber Costa

Quoted 17 lines · Michael S. Tsirkin

Add routines to manage PCI capability list. First user will be MSI-X.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h |   18 +++++++++++-
 2 files changed, 106 insertions(+), 10 deletions(-)

hw/pci.c +0 -1

index 361d741..ed011b5 100644
@@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
     int version = s->cap_present ? 3 : 2;
     int i;
 
-    qemu_put_be32(f, version); /* PCI device version */

hw/pci.c void pci_device_save(PCIDevice *s, QEMUFile *f) · line 140

Quoted 10 lines

hw/pci.c +4 -2

+    /* PCI device version and capabilities */
+    qemu_put_be32(f, version);
+    if (version >= 3)
+        qemu_put_be32(f, s->cap_present);
     qemu_put_buffer(f, s->config, 256);
     for (i = 0; i < 4; i++)
         qemu_put_be32(f, s->irq_state[i]);
-    if (version >= 3)
-        qemu_put_be32(f, s->cap_present);
 }

What is it doing here?
You should just do it right in the first patch, instead of doing in one way there, and fixing here.

Quoted 5 lines

hw/pci.c +0 -0

 int pci_device_load(PCIDevice *s, QEMUFile *f)

@@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     version_id = qemu_get_be32(f);
     if (version_id > 3)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 152

Quoted 10 lines

hw/pci.c +0 -6

         return -EINVAL;
-    qemu_get_buffer(f, s->config, 256);
-    pci_update_mappings(s);
-
-    if (version_id >= 2)
-        for (i = 0; i < 4; i ++)
-            s->irq_state[i] = qemu_get_be32(f);
     if (version_id >= 3)
         s->cap_present = qemu_get_be32(f);
     else

ditto.

Quoted 8 lines

hw/pci.c +4 -0

@@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     if (s->cap_present & ~s->cap_supported)
         return -EINVAL;
 
+    qemu_get_buffer(f, s->config, 256);
+    pci_update_mappings(s);
+
+    if (version_id >= 2)

hw/pci.c int pci_device_load(PCIDevice *s, QEMUFile *f) · line 171

Quoted 10 lines

hw/pci.c +8 -0

+        for (i = 0; i < 4; i ++)
+            s->irq_state[i] = qemu_get_be32(f);
+    /* Clear wmask and used bits for capabilities.
+       Must be restored separately, since capabilities can
+       be placed anywhere in config space. */
+    memset(s->used, 0, PCI_CONFIG_SPACE_SIZE);
+    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
+        s->wmask[i] = 0xff;
     return 0;
 }

Sorry, I don't exactly understand it. Although it can be anywhere, what do we actually lose by keeping it at the same place in config space?

Quoted 6 lines

We lose the ability to let user control the capabilities exposed by the device.

And generally, I dislike arbitrary limitations. The PCI spec says the capability can be anywhere, implementing a linked list of caps is simple enough to not invent abritrary restrictions.

yes, but this is migration time, right?

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

Quoted 9 lines

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

We should not be loading state into a different device (or a similar device with a different set of capabilities).

If you want to provide backwards compatibility then you should do that by creating a device that is the same as the original. As I mentioned in my earlier mail, loading a snapshot should never do anything that can not be achieved through normal operation.

Paul

Quoted 9 lines

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

We should not be loading state into a different device (or a similar device with a different set of capabilities).

If you want to provide backwards compatibility then you should do that by creating a device that is the same as the original. As I mentioned in my earlier mail, loading a snapshot should never do anything that can not be achieved through normal operation.

Paul

Quoted 9 lines

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

We should not be loading state into a different device (or a similar device with a different set of capabilities).

If you want to provide backwards compatibility then you should do that by creating a device that is the same as the original. As I mentioned in my earlier mail, loading a snapshot should never do anything that can not be achieved through normal operation.

Quoted 9 lines

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

We should not be loading state into a different device (or a similar device with a different set of capabilities).

If you want to provide backwards compatibility then you should do that by creating a device that is the same as the original. As I mentioned in my earlier mail, loading a snapshot should never do anything that can not be achieved through normal operation.

Quoted 10 lines · Paul Brook

Quoted 9 lines

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

Quoted 7 lines

We should not be loading state into a different device (or a similar device with a different set of capabilities).

If you want to provide backwards compatibility then you should do that by creating a device that is the same as the original. As I mentioned in my earlier mail, loading a snapshot should never do anything that can not be achieved through normal operation.

If you can create a machine be restoring a snapshot which you can't create by normally starting QEMU, then you'll soon have guests which work fine from their snapshots, but which cannot be booted without a snapshot because there's no way to boot the right machine for the guest.

Ssomeone might even have guests like that for years without noticing, because they always save and restore guest state using snapshots, then one day they simply want to boot the guest from it's disk image and find there's no way to do it with any QEMU which runs on their host platform.

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

-- Jamie

Quoted 10 lines · Paul Brook

Quoted 9 lines

Quoted 4 lines

caps can be anywhere, but we don't expect it to change during machine execution lifetime.

Or I am just confused by the name "pci_device_load" ?

Right. So I want to load an image and it has capability X at offset Y. wmask has to match. I don't want to assume that we never change Y for the device without breaking old images, so I clear wmask here and set it up again after looking up capabilities that I loaded.

Quoted 7 lines

We should not be loading state into a different device (or a similar device with a different set of capabilities).

If you want to provide backwards compatibility then you should do that by creating a device that is the same as the original. As I mentioned in my earlier mail, loading a snapshot should never do anything that can not be achieved through normal operation.

If you can create a machine be restoring a snapshot which you can't create by normally starting QEMU, then you'll soon have guests which work fine from their snapshots, but which cannot be booted without a snapshot because there's no way to boot the right machine for the guest.

Ssomeone might even have guests like that for years without noticing, because they always save and restore guest state using snapshots, then one day they simply want to boot the guest from it's disk image and find there's no way to do it with any QEMU which runs on their host platform.

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

-- Jamie

Quoted 3 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

Quoted 3 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

= Solution - Separate configuration from compat hints =

As I suggested before:

- Allow the VM manager to dump compat hints; this would be an opaque file format, more like the savevm format than a config file

- Use defaults where compat hints are not available; e.g. if the VM manager specifies a device config, but no compat hints are supplied for it, then just use default values

- Make the config override compat hints; e.g. if there are compat hints specified for a device not included in the machine config, just ignore those hints

Cheers,
Mark.

= Solution - Separate configuration from compat hints =

As I suggested before:

- Allow the VM manager to dump compat hints; this would be an opaque file format, more like the savevm format than a config file

- Use defaults where compat hints are not available; e.g. if the VM manager specifies a device config, but no compat hints are supplied for it, then just use default values

- Make the config override compat hints; e.g. if there are compat hints specified for a device not included in the machine config, just ignore those hints

Cheers,
Mark.

Quoted 6 lines · Mark McLoughlin

= Solution - Separate configuration from compat hints =

As I suggested before:

- Allow the VM manager to dump compat hints; this would be an opaque file format, more like the savevm format than a config file

Why make it "like the savevm" format then?
If they are opaque anyway, compat hints could be part of savevm format.

Quoted 8 lines

- Use defaults where compat hints are not available; e.g. if the VM manager specifies a device config, but no compat hints are supplied for it, then just use default values

- Make the config override compat hints; e.g. if there are compat hints specified for a device not included in the machine config, just ignore those hints

Quoted 2 lines

Cheers,
Mark.

If compat hints are opaque and only editable by qemu, we can get into a situation where one can't create a specific setup with a new qemu.

Quoted 6 lines · Mark McLoughlin

= Solution - Separate configuration from compat hints =

As I suggested before:

- Allow the VM manager to dump compat hints; this would be an opaque file format, more like the savevm format than a config file

Why make it "like the savevm" format then?
If they are opaque anyway, compat hints could be part of savevm format.

Quoted 8 lines

- Use defaults where compat hints are not available; e.g. if the VM manager specifies a device config, but no compat hints are supplied for it, then just use default values

- Make the config override compat hints; e.g. if there are compat hints specified for a device not included in the machine config, just ignore those hints

Quoted 2 lines

Cheers,
Mark.

If compat hints are opaque and only editable by qemu, we can get into a situation where one can't create a specific setup with a new qemu.

Quoted 12 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 4 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 7 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

Quoted 12 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 4 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 7 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

Quoted 37 lines · Mark McLoughlin

Quoted 12 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 4 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 7 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

Quoted 4 lines

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

What's the problem with parsing the device config and modifying it? Is it just complexity?

If we provided a mechanism to simplify manipulating a device config, would that eliminate the concern here?

Quoted 37 lines · Mark McLoughlin

Quoted 12 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 4 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 7 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

Quoted 4 lines

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

What's the problem with parsing the device config and modifying it? Is it just complexity?

If we provided a mechanism to simplify manipulating a device config, would that eliminate the concern here?

Quoted 43 lines · Anthony Liguori

Quoted 41 lines · Mark McLoughlin

Quoted 15 lines · Jamie Lokier

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 8 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

Quoted 8 lines

Quoted 5 lines

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

What's the problem with parsing the device config and modifying it? Is it just complexity?

Yes, complexity is the issue.

Quoted 3 lines

If we provided a mechanism to simplify manipulating a device config, would that eliminate the concern here?

In libvirt's case, a lot of the complexity would come from needing to figure out what to change.

Quoted 43 lines · Anthony Liguori

Quoted 41 lines · Mark McLoughlin

Quoted 15 lines · Jamie Lokier

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 8 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

Quoted 7 lines

Quoted 5 lines

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

What's the problem with parsing the device config and modifying it? Is it just complexity?

Yes, complexity is the issue.

Quoted 2 lines

If we provided a mechanism to simplify manipulating a device config, would that eliminate the concern here?

In libvirt's case, a lot of the complexity would come from needing to figure out what to change.

Quoted 57 lines · Mark McLoughlin

Quoted 43 lines · Anthony Liguori

Quoted 41 lines · Mark McLoughlin

Quoted 15 lines · Jamie Lokier

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 8 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

Quoted 8 lines

Quoted 5 lines

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

What's the problem with parsing the device config and modifying it? Is it just complexity?

Yes, complexity is the issue.

Quoted 7 lines

Quoted 3 lines

If we provided a mechanism to simplify manipulating a device config, would that eliminate the concern here?

In libvirt's case, a lot of the complexity would come from needing to figure out what to change.

Right, libvirt wants to be able to easily say "add a scsi block device to this VM". The way I see this working is that there would be a default pc.dtc. We would still have a -drive file=foo.img,if=scsi option that would really just be a wrapper around first searching for an existing LSI controller, if one exists, attaching the lun, if not, create one, etc.

libvirt could continue to use this sort of interface. However, as it wants to do more advanced things, it may have to dive into the device tree itself.

On live migration, QEMU will save a copy of the device tree somewhere and libvirt needs to keep track of it. It can treat it as opaque. -M /path/to/foo.dtc -drive file=foo.img,if=scsi should continue working as expected IMHO.

Quoted 57 lines · Mark McLoughlin

Quoted 43 lines · Anthony Liguori

Quoted 41 lines · Mark McLoughlin

Quoted 15 lines · Jamie Lokier

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines

I think the right long term answer to all this is a way to get QEMU to dump it's current machine configuration in glorious detail as a file which can be reloaded as a machine configuration.

And then we'll have the same set of problems there.

We will, and the solution will be the same: options to create devices as they were in older versions of QEMU. It only needs to cover device features which matter to guests, not every bug fix.

Quoted 8 lines

However with a machine configuration which is generated by QEMU, there's less worry about proliferation of obscure options, compared with the command line. You don't necessarily have to document every backward-compatibility option in any detail, you just have to make sure it's written and read properly, which is much the same thing as the snapshot code does.

This is a sensible plan, but I don't think we should mix these compat options in with the VM manager supplied configuration.

There are two problems with that approach.

= Problem 1 - VM manager needs to parse qemu config =

Your proposal implies:

- VM manager supplies a basic configuration to qemu

- It then immediately asks qemu for a dump of the machine configuration in all its glorious detail and retains that config

Quoted 8 lines

Quoted 5 lines

- If the VM manager wishes to add a new device it needs to parse the qemu config and add it, rather than just generate an entirely new config

What's the problem with parsing the device config and modifying it? Is it just complexity?

Yes, complexity is the issue.

Quoted 7 lines

Quoted 3 lines

If we provided a mechanism to simplify manipulating a device config, would that eliminate the concern here?

In libvirt's case, a lot of the complexity would come from needing to figure out what to change.

Right, libvirt wants to be able to easily say "add a scsi block device to this VM". The way I see this working is that there would be a default pc.dtc. We would still have a -drive file=foo.img,if=scsi option that would really just be a wrapper around first searching for an existing LSI controller, if one exists, attaching the lun, if not, create one, etc.

libvirt could continue to use this sort of interface. However, as it wants to do more advanced things, it may have to dive into the device tree itself.

On live migration, QEMU will save a copy of the device tree somewhere and libvirt needs to keep track of it. It can treat it as opaque. -M /path/to/foo.dtc -drive file=foo.img,if=scsi should continue working as expected IMHO.

So, when libvirt creates a guest for the first time, it makes a copy of the device tree and continues to use that even if qemu is upgraded. That's enough to ensure compat is retained for all built-in devices.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ... or:

2) Don't use the command line, instead get a dump of the entire device tree (including the SCSI device) - if the device is to be removed or modified in future, libvirt would need to modify the device tree

The basic problem would be that the command line config would have very limited ability to override the device tree config.

How do you add a new attribute to the device tree and, when a supplied device tree lacking said attribute, distinguish between a device tree from an old version of qemu (i.e. use the old default) and a partial device tree from the VM manager (i.e. use the new default) ?

Quoted 2 lines

NB the device tree contains no host configuration information.

So, it wouldn't e.g. include the path to the image file for a block device? That would always be specified on the command line?

So, when libvirt creates a guest for the first time, it makes a copy of the device tree and continues to use that even if qemu is upgraded. That's enough to ensure compat is retained for all built-in devices.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ... or:

2) Don't use the command line, instead get a dump of the entire device tree (including the SCSI device) - if the device is to be removed or modified in future, libvirt would need to modify the device tree

The basic problem would be that the command line config would have very limited ability to override the device tree config.

How do you add a new attribute to the device tree and, when a supplied device tree lacking said attribute, distinguish between a device tree from an old version of qemu (i.e. use the old default) and a partial device tree from the VM manager (i.e. use the new default) ?

Quoted 2 lines

NB the device tree contains no host configuration information.

So, it wouldn't e.g. include the path to the image file for a block device? That would always be specified on the command line?

Quoted 18 lines · Mark McLoughlin

So, when libvirt creates a guest for the first time, it makes a copy of the device tree and continues to use that even if qemu is upgraded. That's enough to ensure compat is retained for all built-in devices.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ... or:

2) Don't use the command line, instead get a dump of the entire device tree (including the SCSI device) - if the device is to be removed or modified in future, libvirt would need to modify the device tree

Quoted 3 lines

The basic problem would be that the command line config would have very limited ability to override the device tree config.

After libvirt has done -drive file=foo... it should dump the machine config and use that from then on.

To combined to a single thread...

Quoted 5 lines

How do you add a new attribute to the device tree and, when a supplied device tree lacking said attribute, distinguish between a device tree from an old version of qemu (i.e. use the old default) and a partial device tree from the VM manager (i.e. use the new default) ?

Please define "attribute". I don't follow what you're asking.

Quoted 6 lines

Quoted 2 lines

NB the device tree contains no host configuration information.

So, it wouldn't e.g. include the path to the image file for a block device? That would always be specified on the command line?

No, the IDE definition would contain some sort of symbolic node name. A separate mechanism (either command line or host config file) would then link a image file to the symbolic name.

libvirt should really never worry about the machine config file for normal things unless it needs to change what devices are exposed to a guest.

Quoted 18 lines · Mark McLoughlin

So, when libvirt creates a guest for the first time, it makes a copy of the device tree and continues to use that even if qemu is upgraded. That's enough to ensure compat is retained for all built-in devices.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ... or:

2) Don't use the command line, instead get a dump of the entire device tree (including the SCSI device) - if the device is to be removed or modified in future, libvirt would need to modify the device tree

Quoted 3 lines

The basic problem would be that the command line config would have very limited ability to override the device tree config.

After libvirt has done -drive file=foo... it should dump the machine config and use that from then on.

To combined to a single thread...

Quoted 5 lines

How do you add a new attribute to the device tree and, when a supplied device tree lacking said attribute, distinguish between a device tree from an old version of qemu (i.e. use the old default) and a partial device tree from the VM manager (i.e. use the new default) ?

Please define "attribute". I don't follow what you're asking.

Quoted 6 lines

Quoted 2 lines

NB the device tree contains no host configuration information.

So, it wouldn't e.g. include the path to the image file for a block device? That would always be specified on the command line?

No, the IDE definition would contain some sort of symbolic node name. A separate mechanism (either command line or host config file) would then link a image file to the symbolic name.

libvirt should really never worry about the machine config file for normal things unless it needs to change what devices are exposed to a guest.

Quoted 19 lines · Anthony Liguori

Quoted 18 lines · Mark McLoughlin

So, when libvirt creates a guest for the first time, it makes a copy of the device tree and continues to use that even if qemu is upgraded. That's enough to ensure compat is retained for all built-in devices.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ... or:

2) Don't use the command line, instead get a dump of the entire device tree (including the SCSI device) - if the device is to be removed or modified in future, libvirt would need to modify the device tree

Quoted 6 lines

Quoted 3 lines

The basic problem would be that the command line config would have very limited ability to override the device tree config.

After libvirt has done -drive file=foo... it should dump the machine config and use that from then on.

Right - libvirt then wouldn't be able to avoid the complexity of merging any future changes into the dumped machine config.

Quoted 8 lines

To combined to a single thread...

Quoted 5 lines

How do you add a new attribute to the device tree and, when a supplied device tree lacking said attribute, distinguish between a device tree from an old version of qemu (i.e. use the old default) and a partial device tree from the VM manager (i.e. use the new default) ?

Please define "attribute". I don't follow what you're asking.

e.g. a per-device "enable MSI support" flag.

If qemu is supplied with a device tree that lacks that flag, does it enable or disable MSI?

Enable by default is bad - it could be a device tree dumped from an old version of qemu, so compat would be broken.

Disable by default is bad - it could be a simple device tree supplied by the user, and the latest features are wanted.

Maybe we want a per-device "this is a complete device description" flag and if anything is missing from a supposedly complete description, the old defaults would be used. A config dumped from qemu would have this flag set, a config generated by libvirt would not have the flag.

If the device has different behavior or different properties from guest perspective compared to the old device, it should get a new device type so that you could specify in the device tree either the old device or the new one.

If the device has different behavior or different properties from guest perspective compared to the old device, it should get a new device type so that you could specify in the device tree either the old device or the new one.

Quoted 19 lines · Anthony Liguori

Quoted 18 lines · Mark McLoughlin

So, when libvirt creates a guest for the first time, it makes a copy of the device tree and continues to use that even if qemu is upgraded. That's enough to ensure compat is retained for all built-in devices.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ... or:

2) Don't use the command line, instead get a dump of the entire device tree (including the SCSI device) - if the device is to be removed or modified in future, libvirt would need to modify the device tree

Quoted 6 lines

Quoted 3 lines

The basic problem would be that the command line config would have very limited ability to override the device tree config.

After libvirt has done -drive file=foo... it should dump the machine config and use that from then on.

Right - libvirt then wouldn't be able to avoid the complexity of merging any future changes into the dumped machine config.

Quoted 8 lines

To combined to a single thread...

Quoted 5 lines

How do you add a new attribute to the device tree and, when a supplied device tree lacking said attribute, distinguish between a device tree from an old version of qemu (i.e. use the old default) and a partial device tree from the VM manager (i.e. use the new default) ?

Please define "attribute". I don't follow what you're asking.

e.g. a per-device "enable MSI support" flag.

If qemu is supplied with a device tree that lacks that flag, does it enable or disable MSI?

Enable by default is bad - it could be a device tree dumped from an old version of qemu, so compat would be broken.

Disable by default is bad - it could be a simple device tree supplied by the user, and the latest features are wanted.

Maybe we want a per-device "this is a complete device description" flag and if anything is missing from a supposedly complete description, the old defaults would be used. A config dumped from qemu would have this flag set, a config generated by libvirt would not have the flag.

Quoted 2 lines

After libvirt has done -drive file=foo... it should dump the machine config and use that from then on.

Right - libvirt then wouldn't be able to avoid the complexity of merging any future changes into the dumped machine config.

Quoted 2 lines

After libvirt has done -drive file=foo... it should dump the machine config and use that from then on.

Right - libvirt then wouldn't be able to avoid the complexity of merging any future changes into the dumped machine config.

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ...

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ...

Quoted 4 lines · Mark McLoughlin

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

Quoted 4 lines

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ...

This last option makes sense to me: in a real world the user has control over where he places the device on the bus, so why not with qemu?

Quoted 4 lines · Mark McLoughlin

However, in order to retain compat for that SCSI device (e.g. ensuring the PCI address doesn't change as other devices are added an removed), we're back to the same problem ... either:

Quoted 4 lines

1) Use '-drive file=foo.img,if=scsi,pci_addr=foo'; in order to figure out what address to use, libvirt would need to query qemu for what address was originally allocated to device or it would do all the PCI address allocation itself ...

This last option makes sense to me: in a real world the user has control over where he places the device on the bus, so why not with qemu?

Quoted 4 lines

This last option makes sense to me: in a real world the user has control over where he places the device on the bus, so why not with qemu?

Yep, most people seem to agree that it makes sense to allow this, but some believe it should only be via a machine description file, not the command line.

However, the first problem is that it isn't a solution to the guest ABI problem more generally.

And the second problem is that for e.g. libvirt to use it, it would have to be possible to query qemu for what PCI slots were assigned to the devices - libvirt would need to be able to parse 'info pci' and match the devices listed with the devices specified on the command line.

Quoted 4 lines

This last option makes sense to me: in a real world the user has control over where he places the device on the bus, so why not with qemu?

Yep, most people seem to agree that it makes sense to allow this, but some believe it should only be via a machine description file, not the command line.

However, the first problem is that it isn't a solution to the guest ABI problem more generally.

And the second problem is that for e.g. libvirt to use it, it would have to be possible to query qemu for what PCI slots were assigned to the devices - libvirt would need to be able to parse 'info pci' and match the devices listed with the devices specified on the command line.

Quoted 9 lines · Mark McLoughlin

Quoted 4 lines

This last option makes sense to me: in a real world the user has control over where he places the device on the bus, so why not with qemu?

Yep, most people seem to agree that it makes sense to allow this, but some believe it should only be via a machine description file, not the command line.

I don't understand this opposition. It's clear a machine config file is a long way in our future. It's also clear lack of stable PCI addresses hurts us now.

Quoted 3 lines

However, the first problem is that it isn't a solution to the guest ABI problem more generally.

pci_addr was never meant to bring world peace, just stable PCI addresses. The other issues should be addressed separately.

Quoted 5 lines

And the second problem is that for e.g. libvirt to use it, it would have to be possible to query qemu for what PCI slots were assigned to the devices - libvirt would need to be able to parse 'info pci' and match the devices listed with the devices specified on the command line.

If all devices (including vga, ide) are set up with pci_addr, then this is unneeded.

You do need to export available slot numbers from qemu.

Quoted 9 lines · Mark McLoughlin

Quoted 4 lines

This last option makes sense to me: in a real world the user has control over where he places the device on the bus, so why not with qemu?

Yep, most people seem to agree that it makes sense to allow this, but some believe it should only be via a machine description file, not the command line.

I don't understand this opposition. It's clear a machine config file is a long way in our future. It's also clear lack of stable PCI addresses hurts us now.

Quoted 3 lines

However, the first problem is that it isn't a solution to the guest ABI problem more generally.

pci_addr was never meant to bring world peace, just stable PCI addresses. The other issues should be addressed separately.

Quoted 5 lines

And the second problem is that for e.g. libvirt to use it, it would have to be possible to query qemu for what PCI slots were assigned to the devices - libvirt would need to be able to parse 'info pci' and match the devices listed with the devices specified on the command line.

If all devices (including vga, ide) are set up with pci_addr, then this is unneeded.

You do need to export available slot numbers from qemu.

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Because it does not exist?

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Because it does not exist?

Quoted 6 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Because it does not exist?

We can create a slot with any number, can't we?

Quoted 6 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Because it does not exist?

We can create a slot with any number, can't we?

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Why would a slot be unavailable?

Because it does not exist?

We can create a slot with any number, can't we?

What do you mean? If the mobo has 4 slots you can't create fifth. KVM describes 32 slots in the BIOS.

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 2 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Why would a slot be unavailable?

Because it does not exist?

We can create a slot with any number, can't we?

What do you mean? If the mobo has 4 slots you can't create fifth. KVM describes 32 slots in the BIOS.

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Because it does not exist?

We can create a slot with any number, can't we?

What do you mean? If the mobo has 4 slots you can't create fifth. KVM describes 32 slots in the BIOS.

Do you mean the KVM kernel module here? I don't know much about the

BIOS. Can't qemu control the number of slots declared?

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Because it does not exist?

We can create a slot with any number, can't we?

What do you mean? If the mobo has 4 slots you can't create fifth. KVM describes 32 slots in the BIOS.

Do you mean the KVM kernel module here? I don't know much about the

BIOS. Can't qemu control the number of slots declared?

Quoted 14 lines · Michael S. Tsirkin

Quoted 12 lines · Gleb Natapov

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Because it does not exist?

We can create a slot with any number, can't we?

What do you mean? If the mobo has 4 slots you can't create fifth. KVM describes 32 slots in the BIOS.

Quoted 1 line

Do you mean the KVM kernel module here? I don't know much about the

No I don't mean KVM kernel module here.

Quoted 2 lines

BIOS. Can't qemu control the number of slots declared?

Qemu represents HW, BIOS drives this HW. They should be in sync on such important issues like pci slot configuration.

Even if QEMU can control the number of slots declared (which it can't easily do), it will be able to do it only on startup (before BIOS runs).

The way to have dynamic
number of slots may be pci bridge emulation. Not sure what is needed from BIOS for that.

--
Gleb.

Quoted 14 lines · Michael S. Tsirkin

Quoted 12 lines · Gleb Natapov

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Because it does not exist?

We can create a slot with any number, can't we?

What do you mean? If the mobo has 4 slots you can't create fifth. KVM describes 32 slots in the BIOS.

Quoted 1 line

Do you mean the KVM kernel module here? I don't know much about the

No I don't mean KVM kernel module here.

Quoted 2 lines

BIOS. Can't qemu control the number of slots declared?

Qemu represents HW, BIOS drives this HW. They should be in sync on such important issues like pci slot configuration.

Even if QEMU can control the number of slots declared (which it can't easily do), it will be able to do it only on startup (before BIOS runs).

The way to have dynamic
number of slots may be pci bridge emulation. Not sure what is needed from BIOS for that.

--
Gleb.

Quoted 2 lines

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Quoted 2 lines

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

A slot needs to be configured in ACPI,

and not be taken by onboard chips
(piix takes slot 0, for example).

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

You do need to export available slot numbers from qemu.

Why would a slot be unavailable?

A slot needs to be configured in ACPI,

and not be taken by onboard chips
(piix takes slot 0, for example).

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 2 lines

A slot needs to be configured in ACPI,

Can we configure all possible 32 slots?

Quoted 3 lines

and not be taken by onboard chips
(piix takes slot 0, for example).

piix is the root complex, isn't it? Are there other examples? If not, we could teach management about the root complex being special ...

Quoted 2 lines

A slot needs to be configured in ACPI,

Can we configure all possible 32 slots?

Quoted 3 lines

and not be taken by onboard chips
(piix takes slot 0, for example).

piix is the root complex, isn't it? Are there other examples? If not, we could teach management about the root complex being special ...

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

A slot needs to be configured in ACPI,

Can we configure all possible 32 slots?

That's what we do. But one is always taken. In the future, perhaps more.

Quoted 4 lines

Quoted 3 lines

and not be taken by onboard chips
(piix takes slot 0, for example).

Quoted 3 lines

piix is the root complex, isn't it? Are there other examples? If not, we could teach management about the root complex being special ...

We should just tell the user which slots are open.

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

A slot needs to be configured in ACPI,

Can we configure all possible 32 slots?

That's what we do. But one is always taken. In the future, perhaps more.

Quoted 4 lines

Quoted 3 lines

and not be taken by onboard chips
(piix takes slot 0, for example).

Quoted 3 lines

piix is the root complex, isn't it? Are there other examples? If not, we could teach management about the root complex being special ...

We should just tell the user which slots are open.

We should just tell the user which slots are open.

This might be tricky if the config is passed in with the command line flags.

We should just tell the user which slots are open.

This might be tricky if the config is passed in with the command line flags.

Quoted 5 lines · Michael S. Tsirkin

We should just tell the user which slots are open.

This might be tricky if the config is passed in with the command line flags.

qemu -show-available-pci-slots