[Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Andreas Färber]

Hello Michael and Gerd,

As discussed, this series attempts to clean up PCIe devices fiddling with
individual PCIe VMState fields. It is based on my qom-next queue, containing
type and field changes for PCIe devices.

Patch 1 assumes the following are equivalent:
a) - Struct A
     - Field Aa
     ...
   - Field X
     ...
b) - Struct A
     - Field Aa
     ...
     - Field X

Patch 2 relies on XHCI not being released yet, thus no compatibiliy concerns.

Patch 3 assumes the following are equivalent:
a) - Struct A
     - Field Aa
     ...
     - Field X
b) - Struct A
     - Field Aa
     ...
     - Subsection Ax
       - Field X

CC'ing Juan to verify which of these are correct/safe.

Regards,
Andreas

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>

Andreas Färber (3):
  pcie: Move AER log into VMSTATE_PCIE_DEVICE()
  pci: Add MSIX subsection for vmstate_pcie_device
  pci: Move AER log into VMStateSubsection

 hw/pci-bridge/ioh3420.c            |  2 --
 hw/pci-bridge/xio3130_downstream.c |  2 --
 hw/pci-bridge/xio3130_upstream.c   |  2 --
 hw/pci/pci.c                       | 36 ++++++++++++++++++++++++++++++++++++
 hw/pci/pcie_aer.c                  |  1 +
 hw/usb/hcd-xhci.c                  |  1 -
 6 files changed, 37 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH RFC for-1.6 qom-next 1/3] pcie: Move AER log into VMSTATE_PCIE_DEVICE()

[Andreas Färber]

VMSTATE_PCIE_DEVICE() currently has the following users:
* ioh3420
* xio3130-downstream
* x3130-upstream
* nec-usb-xhci

All except for XHCI have VMSTATE_STRUCT(....exp.aer_log, ...) following
VMSTATE_PCIE_DEVICE(), and XHCI was marked unmigratable in v1.5.0.

Therefore move this recurring PCIe-only field into vmstate_pcie_device
while we still can without breaking migration compatibility.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/pci-bridge/ioh3420.c            | 2 --
 hw/pci-bridge/xio3130_downstream.c | 2 --
 hw/pci-bridge/xio3130_upstream.c   | 2 --
 hw/pci/pci.c                       | 2 ++
 4 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index e07c7e8..296ed9f 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -188,8 +188,6 @@ static const VMStateDescription vmstate_ioh3420 = {
     .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
-        VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log,
-                       PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 2c84b1a..f58717f 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -155,8 +155,6 @@ static const VMStateDescription vmstate_xio3130_downstream = {
     .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
-        VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log,
-                       PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 82add15..fd78966 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -134,8 +134,6 @@ static const VMStateDescription vmstate_xio3130_upstream = {
     .minimum_version_id_old = 1,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort),
-        VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort, 0,
-                       vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4c004f5..51ec4f7 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -490,6 +490,8 @@ const VMStateDescription vmstate_pcie_device = {
         VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
 				   vmstate_info_pci_irq_state,
 				   PCI_NUM_PINS * sizeof(int32_t)),
+        VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
+                       vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC for-1.6 qom-next 2/3] pci: Add MSIX subsection for vmstate_pcie_device

[Andreas Färber]

Instead of having devices like XHCI manually add VMSTATE_MSIX() fields,
reuse vmstate_msix as subsection in VMSTATE_PCIE_DEVICE(), dependent on
msix_present().

XHCI is the only user of VMSTATE_MSIX() and the only PCIe user of
msix_init[_exclusive_bar](). XHCI was marked unmigratable in v1.5.0,
so perform this change before 1.6.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/pci/pci.c      | 14 ++++++++++++++
 hw/usb/hcd-xhci.c |  1 -
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 51ec4f7..b69421a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -477,6 +477,13 @@ const VMStateDescription vmstate_pci_device = {
     }
 };
 
+static bool pcie_msix_needed(void *opaque)
+{
+    PCIDevice *s = opaque;
+
+    return msix_present(s);
+}
+
 const VMStateDescription vmstate_pcie_device = {
     .name = "PCIEDevice",
     .version_id = 2,
@@ -493,6 +500,13 @@ const VMStateDescription vmstate_pcie_device = {
         VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
                        vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_msix,
+            .needed = pcie_msix_needed,
+        }, {
+        }
     }
 };
 
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 9ba3e3e..c9c29b7 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3545,7 +3545,6 @@ static const VMStateDescription vmstate_xhci = {
     .post_load = usb_xhci_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
-        VMSTATE_MSIX(parent_obj, XHCIState),
 
         VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1,
                                      vmstate_xhci_port, XHCIPort),
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC for-1.6 qom-next 3/3] pci: Move AER log into VMStateSubsection

[Andreas Färber]

XHCI does not call pcie_aer_init(), so use allocation of the AER log as
indicator for whether vmstate_pcie_aer_log is needed.

Go safe and zero the log pointer to support unrealizing and re-realizing
a PCIDevice without AER.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/pci/pci.c      | 24 ++++++++++++++++++++++--
 hw/pci/pcie_aer.c |  1 +
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b69421a..4f373f9 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -477,6 +477,25 @@ const VMStateDescription vmstate_pci_device = {
     }
 };
 
+static bool pcie_aer_log_needed(void *opaque)
+{
+    PCIDevice *s = opaque;
+
+    return s->exp.aer_log.log != NULL;
+}
+
+static const VMStateDescription vmstate_pcie_device_aer_log = {
+    .name = "PCIEDevice/AER_Log",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
+                       vmstate_pcie_aer_log, PCIEAERLog),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool pcie_msix_needed(void *opaque)
 {
     PCIDevice *s = opaque;
@@ -497,12 +516,13 @@ const VMStateDescription vmstate_pcie_device = {
         VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
 				   vmstate_info_pci_irq_state,
 				   PCI_NUM_PINS * sizeof(int32_t)),
-        VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
-                       vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (VMStateSubsection[]) {
         {
+            .vmsd = &vmstate_pcie_device_aer_log,
+            .needed = pcie_aer_log_needed,
+        }, {
             .vmsd = &vmstate_msix,
             .needed = pcie_msix_needed,
         }, {
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index ca762ab..2767ebc 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -166,6 +166,7 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
 void pcie_aer_exit(PCIDevice *dev)
 {
     g_free(dev->exp.aer_log.log);
+    dev->exp.aer_log.log = NULL;
 }
 
 static void pcie_aer_update_uncor_status(PCIDevice *dev)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Andreas Färber]

Am 28.07.2013 03:41, schrieb Andreas Färber:
> Patch 1 assumes the following are equivalent:
> a) - Struct A
>      - Field Aa
>      ...
>    - Field X
>      ...
> b) - Struct A
>      - Field Aa
>      ...
>      - Field X
> 
> Patch 2 relies on XHCI not being released yet, thus no compatibiliy concerns.
> 
> Patch 3 assumes the following are equivalent:
> a) - Struct A
>      - Field Aa
>      ...
>      - Field X
> b) - Struct A
>      - Field Aa
>      ...
>      - Subsection Ax
>        - Field X
> 
> CC'ing Juan to verify which of these are correct/safe.

If the answer is "doing both as subsections will work fine" then this
series could be postponed post-1.6, of course.

Looking deeper at PCI devices, I notice that while XHCI was the only
device to use VMSTATE_MSIX() macro, other devices were using
msix_init*() as well, namely
* nvme (unmigratable)
* pci-assign (unmigratable)
* vfio (unmigratable)
* vmxnet3 - does an extra register_savevm() just for msix_save()
* ivshmem - calling msix_save() conditionally after pci_device_save()
* virtio-pci - calling msix_save() conditionally after pci_device_save()
* megasas - #ifdef USE_MSIX'ed out, will need changes

CC'ing net and scsi maintainers and Hannes.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Michael S. Tsirkin]

On Sun, Jul 28, 2013 at 03:41:07AM +0200, Andreas Färber wrote:
> Hello Michael and Gerd,
> 
> As discussed, this series attempts to clean up PCIe devices fiddling with
> individual PCIe VMState fields. It is based on my qom-next queue, containing
> type and field changes for PCIe devices.
> 
> Patch 1 assumes the following are equivalent:
> a) - Struct A
>      - Field Aa
>      ...
>    - Field X
>      ...
> b) - Struct A
>      - Field Aa
>      ...
>      - Field X
> 
> Patch 2 relies on XHCI not being released yet, thus no compatibiliy concerns.
> 
> Patch 3 assumes the following are equivalent:
> a) - Struct A
>      - Field Aa
>      ...
>      - Field X
> b) - Struct A
>      - Field Aa
>      ...
>      - Subsection Ax
>        - Field X
> 
> CC'ing Juan to verify which of these are correct/safe.
> 
> Regards,
> Andreas

As far as I know 2a and 2b are not equivalent: subsections
are serialized in the file.
You really should test by migrating to file (e.g. before
starting the guest) and checking.
Also, migrate to file from before patch and load after patch.

Re: [Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Andreas Färber]

Am 28.07.2013 15:44, schrieb Andreas Färber:
> Am 28.07.2013 03:41, schrieb Andreas Färber:
>> Patch 1 assumes the following are equivalent:
>> a) - Struct A
>>      - Field Aa
>>      ...
>>    - Field X
>>      ...
>> b) - Struct A
>>      - Field Aa
>>      ...
>>      - Field X
>>
>> Patch 2 relies on XHCI not being released yet, thus no compatibiliy concerns.
>>
>> Patch 3 assumes the following are equivalent:
>> a) - Struct A
>>      - Field Aa
>>      ...
>>      - Field X
>> b) - Struct A
>>      - Field Aa
>>      ...
>>      - Subsection Ax
>>        - Field X
>>
>> CC'ing Juan to verify which of these are correct/safe.
> 
> If the answer is "doing both as subsections will work fine" then this
> series could be postponed post-1.6, of course.
> 
> Looking deeper at PCI devices, I notice that while XHCI was the only
> device to use VMSTATE_MSIX() macro, other devices were using
> msix_init*() as well, namely
> * nvme (unmigratable)
> * pci-assign (unmigratable)
> * vfio (unmigratable)
> * vmxnet3 - does an extra register_savevm() just for msix_save()
> * ivshmem - calling msix_save() conditionally after pci_device_save()
> * virtio-pci - calling msix_save() conditionally after pci_device_save()

Sorry, unconditionally, but msix_save() is no-op if !msix_present(), so
effect is the same - stored immediately after for some devices, with
ivshmem using its own qdev property condition hopefully equivalent to
msix_present() and vmxnet3 breaking that scheme.

While pci_device_save() internally reuses vmstate_pci[e]_device, neither
ivshmem nor virtio-pci set PCIDeviceClass::is_express, so are unaffected
by changes of vmstate_pcie_device in this series.

Stefan, do you see bumping vmxnet3 version_id post-1.6 as an acceptable
solution? Then we could make MSI-X a vmstate_pci_device subsection, too,
if I'm not making a thinko.

Andreas

> * megasas - #ifdef USE_MSIX'ed out, will need changes
> 
> CC'ing net and scsi maintainers and Hannes.
> 
> Regards,
> Andreas
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Andreas Färber]

Am 28.07.2013 16:08, schrieb Michael S. Tsirkin:
> On Sun, Jul 28, 2013 at 03:41:07AM +0200, Andreas Färber wrote:
>> Hello Michael and Gerd,
>>
>> As discussed, this series attempts to clean up PCIe devices fiddling with
>> individual PCIe VMState fields. It is based on my qom-next queue, containing
>> type and field changes for PCIe devices.
>>
>> Patch 1 assumes the following are equivalent:
>> a) - Struct A
>>      - Field Aa
>>      ...
>>    - Field X
>>      ...
>> b) - Struct A
>>      - Field Aa
>>      ...
>>      - Field X
>>
>> Patch 2 relies on XHCI not being released yet, thus no compatibiliy concerns.
>>
>> Patch 3 assumes the following are equivalent:
>> a) - Struct A
>>      - Field Aa
>>      ...
>>      - Field X
>> b) - Struct A
>>      - Field Aa
>>      ...
>>      - Subsection Ax
>>        - Field X
>>
>> CC'ing Juan to verify which of these are correct/safe.
> 
> As far as I know 2a and 2b are not equivalent: subsections
> are serialized in the file.

I'm guessing you mean scenarios 3a and 3b. Too bad.

But when we want to unify PCI and PCIe as you suggested elsewhere,
introducing a PCIe-only subsection in patch 2 seems a bad idea.

In that case we could use VMSTATE_STRUCT_TEST(), I'll look into that.
Using that I think we can postpone this beyond 1.6, since XHCI is not
initializing AER.

Series formally retracted.

> You really should test by migrating to file (e.g. before
> starting the guest) and checking.
> Also, migrate to file from before patch and load after patch.

OK. For virtio and NICs that's trivial and for the ioh there's
docs/q35-chipset.cfg.

How do I use and test those xio upstream/downstream PCIe devices?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFC for-1.6 qom-next 0/3] PCIe VMState cleanups for 1.6

[Michael S. Tsirkin]

On Sun, Jul 28, 2013 at 04:59:18PM +0200, Andreas Färber wrote:
> Am 28.07.2013 16:08, schrieb Michael S. Tsirkin:
> > On Sun, Jul 28, 2013 at 03:41:07AM +0200, Andreas Färber wrote:
> >> Hello Michael and Gerd,
> >>
> >> As discussed, this series attempts to clean up PCIe devices fiddling with
> >> individual PCIe VMState fields. It is based on my qom-next queue, containing
> >> type and field changes for PCIe devices.
> >>
> >> Patch 1 assumes the following are equivalent:
> >> a) - Struct A
> >>      - Field Aa
> >>      ...
> >>    - Field X
> >>      ...
> >> b) - Struct A
> >>      - Field Aa
> >>      ...
> >>      - Field X
> >>
> >> Patch 2 relies on XHCI not being released yet, thus no compatibiliy concerns.
> >>
> >> Patch 3 assumes the following are equivalent:
> >> a) - Struct A
> >>      - Field Aa
> >>      ...
> >>      - Field X
> >> b) - Struct A
> >>      - Field Aa
> >>      ...
> >>      - Subsection Ax
> >>        - Field X
> >>
> >> CC'ing Juan to verify which of these are correct/safe.
> > 
> > As far as I know 2a and 2b are not equivalent: subsections
> > are serialized in the file.
> 
> I'm guessing you mean scenarios 3a and 3b. Too bad.
> 
> But when we want to unify PCI and PCIe as you suggested elsewhere,
> introducing a PCIe-only subsection in patch 2 seems a bad idea.
> 
> In that case we could use VMSTATE_STRUCT_TEST(), I'll look into that.

Right.

> Using that I think we can postpone this beyond 1.6, since XHCI is not
> initializing AER.
> 
> Series formally retracted.
> 
> > You really should test by migrating to file (e.g. before
> > starting the guest) and checking.
> > Also, migrate to file from before patch and load after patch.
> 
> OK. For virtio and NICs that's trivial and for the ioh there's
> docs/q35-chipset.cfg.
> 
> How do I use and test those xio upstream/downstream PCIe devices?
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg