[PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Leonardo Bras]

Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
set for machine types < 8.0 will cause migration to fail if the target
QEMU version is < 8.0.0 :

qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
qemu-system-x86_64: Failed to load PCIDevice:config
qemu-system-x86_64: Failed to load e1000e:parent_obj
qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
qemu-system-x86_64: load of migration failed: Invalid argument

The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
with this cmdline:

./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]

In order to fix this, property x-pcie-err-unc-mask was introduced to
control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
default, but is disabled if machine type <= 7.2.

Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 include/hw/pci/pci.h |  2 ++
 hw/core/machine.c    |  1 +
 hw/pci/pci.c         |  2 ++
 hw/pci/pcie_aer.c    | 11 +++++++----
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 935b4b91b4..e6d0574a29 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -207,6 +207,8 @@ enum {
     QEMU_PCIE_EXTCAP_INIT = (1 << QEMU_PCIE_EXTCAP_INIT_BITNR),
 #define QEMU_PCIE_CXL_BITNR 10
     QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR),
+#define QEMU_PCIE_ERR_UNC_MASK_BITNR 11
+    QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR),
 };
 
 typedef struct PCIINTxRoute {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 47a34841a5..07f763eb2e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -48,6 +48,7 @@ GlobalProperty hw_compat_7_2[] = {
     { "e1000e", "migrate-timadj", "off" },
     { "virtio-mem", "x-early-migration", "false" },
     { "migration", "x-preempt-pre-7-2", "true" },
+    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
 };
 const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8a87ccc8b0..5153ad63d6 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -79,6 +79,8 @@ static Property pci_props[] = {
     DEFINE_PROP_STRING("failover_pair_id", PCIDevice,
                        failover_pair_id),
     DEFINE_PROP_UINT32("acpi-index",  PCIDevice, acpi_index, 0),
+    DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present,
+                    QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 103667c368..374d593ead 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -112,10 +112,13 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
 
     pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
                  PCI_ERR_UNC_SUPPORTED);
-    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
-                 PCI_ERR_UNC_MASK_DEFAULT);
-    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
-                 PCI_ERR_UNC_SUPPORTED);
+
+    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
+        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
+                     PCI_ERR_UNC_MASK_DEFAULT);
+        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
+                     PCI_ERR_UNC_SUPPORTED);
+    }
 
     pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
                  PCI_ERR_UNC_SEVERITY_DEFAULT);
-- 
2.40.0

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Jonathan Cameron via]

On Tue,  2 May 2023 21:27:02 -0300
Leonardo Bras <leobras@redhat.com> wrote:

> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
> 
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
> 
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> 
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
> 
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Thanks Leo, you are a star.

LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/hw/pci/pci.h |  2 ++
>  hw/core/machine.c    |  1 +
>  hw/pci/pci.c         |  2 ++
>  hw/pci/pcie_aer.c    | 11 +++++++----
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 935b4b91b4..e6d0574a29 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -207,6 +207,8 @@ enum {
>      QEMU_PCIE_EXTCAP_INIT = (1 << QEMU_PCIE_EXTCAP_INIT_BITNR),
>  #define QEMU_PCIE_CXL_BITNR 10
>      QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR),
> +#define QEMU_PCIE_ERR_UNC_MASK_BITNR 11
> +    QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR),
>  };
>  
>  typedef struct PCIINTxRoute {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 47a34841a5..07f763eb2e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -48,6 +48,7 @@ GlobalProperty hw_compat_7_2[] = {
>      { "e1000e", "migrate-timadj", "off" },
>      { "virtio-mem", "x-early-migration", "false" },
>      { "migration", "x-preempt-pre-7-2", "true" },
> +    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
>  };
>  const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
>  
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 8a87ccc8b0..5153ad63d6 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -79,6 +79,8 @@ static Property pci_props[] = {
>      DEFINE_PROP_STRING("failover_pair_id", PCIDevice,
>                         failover_pair_id),
>      DEFINE_PROP_UINT32("acpi-index",  PCIDevice, acpi_index, 0),
> +    DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present,
> +                    QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index 103667c368..374d593ead 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -112,10 +112,13 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
>  
>      pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
>                   PCI_ERR_UNC_SUPPORTED);
> -    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> -                 PCI_ERR_UNC_MASK_DEFAULT);
> -    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> -                 PCI_ERR_UNC_SUPPORTED);
> +
> +    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
> +        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> +                     PCI_ERR_UNC_MASK_DEFAULT);
> +        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> +                     PCI_ERR_UNC_SUPPORTED);
> +    }
>  
>      pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
>                   PCI_ERR_UNC_SEVERITY_DEFAULT);

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Leonardo Bras Soares Passos]

On Wed, May 3, 2023 at 6:49 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue,  2 May 2023 21:27:02 -0300
> Leonardo Bras <leobras@redhat.com> wrote:
>
> > Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> > set for machine types < 8.0 will cause migration to fail if the target
> > QEMU version is < 8.0.0 :
> >
> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> > qemu-system-x86_64: Failed to load PCIDevice:config
> > qemu-system-x86_64: Failed to load e1000e:parent_obj
> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> > qemu-system-x86_64: load of migration failed: Invalid argument
> >
> > The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> > with this cmdline:
> >
> > ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> >
> > In order to fix this, property x-pcie-err-unc-mask was introduced to
> > control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> > default, but is disabled if machine type <= 7.2.
> >
> > Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
>
> Thanks Leo, you are a star.
>
> LGTM
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>

Thanks!

> > ---
> >  include/hw/pci/pci.h |  2 ++
> >  hw/core/machine.c    |  1 +
> >  hw/pci/pci.c         |  2 ++
> >  hw/pci/pcie_aer.c    | 11 +++++++----
> >  4 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 935b4b91b4..e6d0574a29 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -207,6 +207,8 @@ enum {
> >      QEMU_PCIE_EXTCAP_INIT = (1 << QEMU_PCIE_EXTCAP_INIT_BITNR),
> >  #define QEMU_PCIE_CXL_BITNR 10
> >      QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR),
> > +#define QEMU_PCIE_ERR_UNC_MASK_BITNR 11
> > +    QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR),
> >  };
> >
> >  typedef struct PCIINTxRoute {
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 47a34841a5..07f763eb2e 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -48,6 +48,7 @@ GlobalProperty hw_compat_7_2[] = {
> >      { "e1000e", "migrate-timadj", "off" },
> >      { "virtio-mem", "x-early-migration", "false" },
> >      { "migration", "x-preempt-pre-7-2", "true" },
> > +    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> >  };
> >  const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 8a87ccc8b0..5153ad63d6 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -79,6 +79,8 @@ static Property pci_props[] = {
> >      DEFINE_PROP_STRING("failover_pair_id", PCIDevice,
> >                         failover_pair_id),
> >      DEFINE_PROP_UINT32("acpi-index",  PCIDevice, acpi_index, 0),
> > +    DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present,
> > +                    QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >
> > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> > index 103667c368..374d593ead 100644
> > --- a/hw/pci/pcie_aer.c
> > +++ b/hw/pci/pcie_aer.c
> > @@ -112,10 +112,13 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, uint16_t offset,
> >
> >      pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
> >                   PCI_ERR_UNC_SUPPORTED);
> > -    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> > -                 PCI_ERR_UNC_MASK_DEFAULT);
> > -    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> > -                 PCI_ERR_UNC_SUPPORTED);
> > +
> > +    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
> > +        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> > +                     PCI_ERR_UNC_MASK_DEFAULT);
> > +        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> > +                     PCI_ERR_UNC_SUPPORTED);
> > +    }
> >
> >      pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
> >                   PCI_ERR_UNC_SEVERITY_DEFAULT);
>

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Peter Xu]

On Tue, May 02, 2023 at 09:27:02PM -0300, Leonardo Bras wrote:
> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
> 
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
> 
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> 
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
> 
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
>
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
>
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
>
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
>
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Peter Xu]

On Tue, May 02, 2023 at 09:27:02PM -0300, Leonardo Bras wrote:
> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
> 
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
> 
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> 
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
> 
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Since this one blocks mostly all backward migration for current master and
all the downstream trees, shall we consider pick it up sooner?

And I think we should make sure qemu-stable gets this too, I'm not pretty
sure how we normally do that and whether we need explicit CC: to stable
list, perhaps not..

Thanks!

-- 
Peter Xu

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael S. Tsirkin]

On Tue, May 09, 2023 at 07:01:53AM -0700, Peter Xu wrote:
> On Tue, May 02, 2023 at 09:27:02PM -0300, Leonardo Bras wrote:
> > Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> > set for machine types < 8.0 will cause migration to fail if the target
> > QEMU version is < 8.0.0 :
> > 
> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> > qemu-system-x86_64: Failed to load PCIDevice:config
> > qemu-system-x86_64: Failed to load e1000e:parent_obj
> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> > qemu-system-x86_64: load of migration failed: Invalid argument
> > 
> > The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> > with this cmdline:
> > 
> > ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> > 
> > In order to fix this, property x-pcie-err-unc-mask was introduced to
> > control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> > default, but is disabled if machine type <= 7.2.
> > 
> > Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> 
> Since this one blocks mostly all backward migration for current master and
> all the downstream trees, shall we consider pick it up sooner?
> 
> And I think we should make sure qemu-stable gets this too, I'm not pretty
> sure how we normally do that and whether we need explicit CC: to stable
> list, perhaps not..
> 
> Thanks!
> 
> -- 
> Peter Xu


Yes will be in the pull I'm working on, targeting end of week.

-- 
MST

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
>
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
>
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
>
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
>
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael Tokarev]

03.05.2023 03:27, Leonardo Bras пишет:
> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
> 
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
> 
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> 
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
> 
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

It looks like 8.0-stable material to me, is it not?

/mjt

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael S. Tsirkin]

On Wed, May 10, 2023 at 07:29:40PM +0300, Michael Tokarev wrote:
> 03.05.2023 03:27, Leonardo Bras пишет:
> > Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> > set for machine types < 8.0 will cause migration to fail if the target
> > QEMU version is < 8.0.0 :
> > 
> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> > qemu-system-x86_64: Failed to load PCIDevice:config
> > qemu-system-x86_64: Failed to load e1000e:parent_obj
> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> > qemu-system-x86_64: load of migration failed: Invalid argument
> > 
> > The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> > with this cmdline:
> > 
> > ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> > 
> > In order to fix this, property x-pcie-err-unc-mask was introduced to
> > control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> > default, but is disabled if machine type <= 7.2.
> > 
> > Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> 
> It looks like 8.0-stable material to me, is it not?
> 
> /mjt

Good point. Feel free to CC stable.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Michael Tokarev <mjt@tls.msk.ru> wrote:
> 03.05.2023 03:27, Leonardo Bras пишет:
>> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
>> set for machine types < 8.0 will cause migration to fail if the target
>> QEMU version is < 8.0.0 :
>> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a
>> read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
>> qemu-system-x86_64: Failed to load PCIDevice:config
>> qemu-system-x86_64: Failed to load e1000e:parent_obj
>> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
>> qemu-system-x86_64: load of migration failed: Invalid argument
>> The above test migrated a 7.2 machine type from QEMU master to QEMU
>> 7.2.0,
>> with this cmdline:
>> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
>> In order to fix this, property x-pcie-err-unc-mask was introduced to
>> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
>> default, but is disabled if machine type <= 7.2.
>> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK
>> register")
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Leonardo Bras <leobras@redhat.com>
>
> It looks like 8.0-stable material to me, is it not?

It is .

I thought we have already requested that.  We lost the intent somewhere,
sorry.

Later, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Fiona Ebner]

Am 03.05.23 um 02:27 schrieb Leonardo Bras:
> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> set for machine types < 8.0 will cause migration to fail if the target
> QEMU version is < 8.0.0 :
> 
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load e1000e:parent_obj
> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> qemu-system-x86_64: load of migration failed: Invalid argument
> 
> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> with this cmdline:
> 
> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> 
> In order to fix this, property x-pcie-err-unc-mask was introduced to
> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> default, but is disabled if machine type <= 7.2.
> 
> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Thank you for the patch!

Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576

AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
when using machine type <= 7.2. That is because after this patch, when
using machine type <= 7.2, the wmask for the register is not set and
when 8.0 sends a nonzero value for the register, the error condition in
get_pci_config_device() will trigger again.

Is it necessary to also handle that? Maybe by special casing the error
condition in get_pci_config_device() to be prepared to accept such a
stream from 8.0?

If that is considered not worth it, consider this:

Tested-by: Fiona Ebner <f.ebner@proxmox.com>

Best Regards,
Fiona

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Fiona Ebner <f.ebner@proxmox.com> wrote:
> Am 03.05.23 um 02:27 schrieb Leonardo Bras:
>> Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
>> set for machine types < 8.0 will cause migration to fail if the target
>> QEMU version is < 8.0.0 :
>> 
>> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
>> qemu-system-x86_64: Failed to load PCIDevice:config
>> qemu-system-x86_64: Failed to load e1000e:parent_obj
>> qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
>> qemu-system-x86_64: load of migration failed: Invalid argument
>> 
>> The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
>> with this cmdline:
>> 
>> ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
>> 
>> In order to fix this, property x-pcie-err-unc-mask was introduced to
>> control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
>> default, but is disabled if machine type <= 7.2.
>> 
>> Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Leonardo Bras <leobras@redhat.com>
>
> Thank you for the patch!
>
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
>
> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
> when using machine type <= 7.2. That is because after this patch, when
> using machine type <= 7.2, the wmask for the register is not set and
> when 8.0 sends a nonzero value for the register, the error condition in
> get_pci_config_device() will trigger again.

I think that works correctly.
See https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02733.html

What we have (before this patch) (using abbrevs as in the doc before)

Current state:

(1) qemu-8.0 -M pc-8.0 -> qemu-8.0 -M pc-8.0 works

    not affected by the patch

(2) qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-8.0 works

    works well because 7.2 don't change that field

(3) qemu-8.0 -M pc-7.2 -> qemu-7.2 -M pc-7.2 fails

With the patch we fixed 3, so once it is in stable, 1 and 2 continue as
usual and for (3) we will have:

(3) qemu-8.0.1 -M pc-7.2 -> qemu-7.2 -M pc-7.2 works

If what you mean is that:

(3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works

Will fail, that is true, but I can think a "sane" way to fix this.


> Is it necessary to also handle that? Maybe by special casing the error
> condition in get_pci_config_device() to be prepared to accept such a
> stream from 8.0?

Well, we can do that, but it is to the pci maintainers to decide if that
is "sane".


> If that is considered not worth it, consider this:
>
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>
> Best Regards,
> Fiona

Later, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael Tokarev]

11.05.2023 11:40, Juan Quintela wrote:
> Fiona Ebner <f.ebner@proxmox.com> wrote:
...
>> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
>>
>> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
>> when using machine type <= 7.2. That is because after this patch, when
>> using machine type <= 7.2, the wmask for the register is not set and
>> when 8.0 sends a nonzero value for the register, the error condition in
>> get_pci_config_device() will trigger again.
> 
> I think that works correctly.
> See https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02733.html
> 
> What we have (before this patch) (using abbrevs as in the doc before)
> 
> Current state:
> 
> (1) qemu-8.0 -M pc-8.0 -> qemu-8.0 -M pc-8.0 works
> 
>      not affected by the patch
> 
> (2) qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-8.0 works
> 
>      works well because 7.2 don't change that field
> 
> (3) qemu-8.0 -M pc-7.2 -> qemu-7.2 -M pc-7.2 fails
> 
> With the patch we fixed 3, so once it is in stable, 1 and 2 continue as
> usual and for (3) we will have:
> 
> (3) qemu-8.0.1 -M pc-7.2 -> qemu-7.2 -M pc-7.2 works
> 
> If what you mean is that:
> 
> (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
> 
> Will fail, that is true, but I can think a "sane" way to fix this.

That's a great summary indeed.

>> Is it necessary to also handle that? Maybe by special casing the error
>> condition in get_pci_config_device() to be prepared to accept such a
>> stream from 8.0?
> 
> Well, we can do that, but it is to the pci maintainers to decide if that
> is "sane".

So, can we go from here somewhere? I'd love this fix to be in 8.0.1,
either with or without the (un)sane part of the (3) variant above which
might fail.  Or else we'll have the same situation in 8.0.1 as we now
have in 8.0.0 (the deadline is May-27).

We did broke x.y.0 => x.y.1 migration before already like this, such as
with 7.2.0=>7.2.1. I'm not saying it's a nice thing to do, just stating
a fact. Yes, it is better to avoid such breakage, but.. meh..

Thanks!

/mjt

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Michael Tokarev <mjt@tls.msk.ru> wrote:
> 11.05.2023 11:40, Juan Quintela wrote:
>> Fiona Ebner <f.ebner@proxmox.com> wrote:
> ...
>>> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
>>>
>>> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
>>> when using machine type <= 7.2. That is because after this patch, when
>>> using machine type <= 7.2, the wmask for the register is not set and
>>> when 8.0 sends a nonzero value for the register, the error condition in
>>> get_pci_config_device() will trigger again.
>> I think that works correctly.
>> See https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02733.html
>> What we have (before this patch) (using abbrevs as in the doc
>> before)
>> Current state:
>> (1) qemu-8.0 -M pc-8.0 -> qemu-8.0 -M pc-8.0 works
>>      not affected by the patch
>> (2) qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-8.0 works
>>      works well because 7.2 don't change that field
>> (3) qemu-8.0 -M pc-7.2 -> qemu-7.2 -M pc-7.2 fails
>> With the patch we fixed 3, so once it is in stable, 1 and 2 continue
>> as
>> usual and for (3) we will have:
>> (3) qemu-8.0.1 -M pc-7.2 -> qemu-7.2 -M pc-7.2 works
>> If what you mean is that:
>> (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
>> Will fail, that is true, but I can think a "sane" way to fix this.

Hi

> That's a great summary indeed.

Thanks.

>>> Is it necessary to also handle that? Maybe by special casing the error
>>> condition in get_pci_config_device() to be prepared to accept such a
>>> stream from 8.0?
>> Well, we can do that, but it is to the pci maintainers to decide if
>> that
>> is "sane".
>
> So, can we go from here somewhere? I'd love this fix to be in 8.0.1,
> either with or without the (un)sane part of the (3) variant above which
> might fail.  Or else we'll have the same situation in 8.0.1 as we now
> have in 8.0.0 (the deadline is May-27).
>
> We did broke x.y.0 => x.y.1 migration before already like this, such as
> with 7.2.0=>7.2.1. I'm not saying it's a nice thing to do, just stating
> a fact. Yes, it is better to avoid such breakage, but.. meh..

See patch for documentation:

https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03288.html

Basically, the best we can do is:
- get the patch posted.  Fixes everything except:
  (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works

And for that, we can document somewhere that we need to launch
qemu-8.0.1 as:

$ qemu-8.0.1 -M pc-7.2 -device blah,x-pci-err-unc-mask=on

And mark someone that this machine is tainted an can only be migrated to
qemu's >= qemu-8.0.1.  And that we should reboot it as the user
convenience. (reboot here means poweroff qemu and poweron it back
without x-pci-err-unc-mask=on).

Later, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Peter Xu]

On Thu, May 18, 2023 at 01:33:43PM +0200, Juan Quintela wrote:
> Michael Tokarev <mjt@tls.msk.ru> wrote:
> > 11.05.2023 11:40, Juan Quintela wrote:
> >> Fiona Ebner <f.ebner@proxmox.com> wrote:
> > ...
> >>> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
> >>>
> >>> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
> >>> when using machine type <= 7.2. That is because after this patch, when
> >>> using machine type <= 7.2, the wmask for the register is not set and
> >>> when 8.0 sends a nonzero value for the register, the error condition in
> >>> get_pci_config_device() will trigger again.
> >> I think that works correctly.
> >> See https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02733.html
> >> What we have (before this patch) (using abbrevs as in the doc
> >> before)
> >> Current state:
> >> (1) qemu-8.0 -M pc-8.0 -> qemu-8.0 -M pc-8.0 works
> >>      not affected by the patch
> >> (2) qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-8.0 works
> >>      works well because 7.2 don't change that field
> >> (3) qemu-8.0 -M pc-7.2 -> qemu-7.2 -M pc-7.2 fails
> >> With the patch we fixed 3, so once it is in stable, 1 and 2 continue
> >> as
> >> usual and for (3) we will have:
> >> (3) qemu-8.0.1 -M pc-7.2 -> qemu-7.2 -M pc-7.2 works
> >> If what you mean is that:
> >> (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
> >> Will fail, that is true, but I can think a "sane" way to fix this.
> 
> Hi
> 
> > That's a great summary indeed.
> 
> Thanks.
> 
> >>> Is it necessary to also handle that? Maybe by special casing the error
> >>> condition in get_pci_config_device() to be prepared to accept such a
> >>> stream from 8.0?
> >> Well, we can do that, but it is to the pci maintainers to decide if
> >> that
> >> is "sane".
> >
> > So, can we go from here somewhere? I'd love this fix to be in 8.0.1,
> > either with or without the (un)sane part of the (3) variant above which
> > might fail.  Or else we'll have the same situation in 8.0.1 as we now
> > have in 8.0.0 (the deadline is May-27).
> >
> > We did broke x.y.0 => x.y.1 migration before already like this, such as
> > with 7.2.0=>7.2.1. I'm not saying it's a nice thing to do, just stating
> > a fact. Yes, it is better to avoid such breakage, but.. meh..
> 
> See patch for documentation:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03288.html
> 
> Basically, the best we can do is:
> - get the patch posted.  Fixes everything except:
>   (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
> 
> And for that, we can document somewhere that we need to launch
> qemu-8.0.1 as:
> 
> $ qemu-8.0.1 -M pc-7.2 -device blah,x-pci-err-unc-mask=on

One thing we can also do to avoid it in the future is simply having someone
do this check around each softfreeze (and we'll also need maintainers be
careful on merging anything that's risky though after softfreeze) rather
than after release (what I did for this time, which is late), try to cover
as much devices as possible. I don't know whether there's a way to always
cover all devices.

I'll volunteer myself for that as long as I'll remember.  Juan, please also
have a check or remind me if I didn't. :)

I am not sure whether I mentioned it somewhere before, but maybe it'll work
if we can also have some way we check migrating each of the vmsd from
old-qemu to new-qemu (and also new->old) covering all devices.  It doesn't
need to be a real migration, just generate the per-device stream and try
loading on the other binary.

It might be an overkill to be part of CI to check each commit, but if
there's some way to check it then at least we can run it also after
softfreeze.  I also don't know whether it'll be easy to achieve it at all,
but I'll think more about it too and update if I found something useful.

> 
> And mark someone that this machine is tainted an can only be migrated to
> qemu's >= qemu-8.0.1.  And that we should reboot it as the user
> convenience. (reboot here means poweroff qemu and poweron it back
> without x-pci-err-unc-mask=on).
> 
> Later, Juan.
> 
> 
> 
> 

-- 
Peter Xu

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael S. Tsirkin]

On Thu, May 18, 2023 at 09:27:01AM -0400, Peter Xu wrote:
> On Thu, May 18, 2023 at 01:33:43PM +0200, Juan Quintela wrote:
> > Michael Tokarev <mjt@tls.msk.ru> wrote:
> > > 11.05.2023 11:40, Juan Quintela wrote:
> > >> Fiona Ebner <f.ebner@proxmox.com> wrote:
> > > ...
> > >>> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
> > >>>
> > >>> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
> > >>> when using machine type <= 7.2. That is because after this patch, when
> > >>> using machine type <= 7.2, the wmask for the register is not set and
> > >>> when 8.0 sends a nonzero value for the register, the error condition in
> > >>> get_pci_config_device() will trigger again.
> > >> I think that works correctly.
> > >> See https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02733.html
> > >> What we have (before this patch) (using abbrevs as in the doc
> > >> before)
> > >> Current state:
> > >> (1) qemu-8.0 -M pc-8.0 -> qemu-8.0 -M pc-8.0 works
> > >>      not affected by the patch
> > >> (2) qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-8.0 works
> > >>      works well because 7.2 don't change that field
> > >> (3) qemu-8.0 -M pc-7.2 -> qemu-7.2 -M pc-7.2 fails
> > >> With the patch we fixed 3, so once it is in stable, 1 and 2 continue
> > >> as
> > >> usual and for (3) we will have:
> > >> (3) qemu-8.0.1 -M pc-7.2 -> qemu-7.2 -M pc-7.2 works
> > >> If what you mean is that:
> > >> (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
> > >> Will fail, that is true, but I can think a "sane" way to fix this.
> > 
> > Hi
> > 
> > > That's a great summary indeed.
> > 
> > Thanks.
> > 
> > >>> Is it necessary to also handle that? Maybe by special casing the error
> > >>> condition in get_pci_config_device() to be prepared to accept such a
> > >>> stream from 8.0?
> > >> Well, we can do that, but it is to the pci maintainers to decide if
> > >> that
> > >> is "sane".
> > >
> > > So, can we go from here somewhere? I'd love this fix to be in 8.0.1,
> > > either with or without the (un)sane part of the (3) variant above which
> > > might fail.  Or else we'll have the same situation in 8.0.1 as we now
> > > have in 8.0.0 (the deadline is May-27).
> > >
> > > We did broke x.y.0 => x.y.1 migration before already like this, such as
> > > with 7.2.0=>7.2.1. I'm not saying it's a nice thing to do, just stating
> > > a fact. Yes, it is better to avoid such breakage, but.. meh..
> > 
> > See patch for documentation:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03288.html
> > 
> > Basically, the best we can do is:
> > - get the patch posted.  Fixes everything except:
> >   (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
> > 
> > And for that, we can document somewhere that we need to launch
> > qemu-8.0.1 as:
> > 
> > $ qemu-8.0.1 -M pc-7.2 -device blah,x-pci-err-unc-mask=on
> 
> One thing we can also do to avoid it in the future is simply having someone
> do this check around each softfreeze (and we'll also need maintainers be
> careful on merging anything that's risky though after softfreeze) rather
> than after release (what I did for this time, which is late), try to cover
> as much devices as possible. I don't know whether there's a way to always
> cover all devices.
> 
> I'll volunteer myself for that as long as I'll remember.  Juan, please also
> have a check or remind me if I didn't. :)
> 
> I am not sure whether I mentioned it somewhere before, but maybe it'll work
> if we can also have some way we check migrating each of the vmsd from
> old-qemu to new-qemu (and also new->old) covering all devices.  It doesn't
> need to be a real migration, just generate the per-device stream and try
> loading on the other binary.
> 
> It might be an overkill to be part of CI to check each commit, but if
> there's some way to check it then at least we can run it also after
> softfreeze.  I also don't know whether it'll be easy to achieve it at all,
> but I'll think more about it too and update if I found something useful.

I'm all for more testing but if it does not actually test that the
values loaded are actually used correctly then the testing is of course
lower quality. Better than nothing I guess ...



> > 
> > And mark someone that this machine is tainted an can only be migrated to
> > qemu's >= qemu-8.0.1.  And that we should reboot it as the user
> > convenience. (reboot here means poweroff qemu and poweron it back
> > without x-pci-err-unc-mask=on).
> > 
> > Later, Juan.
> > 
> > 
> > 
> > 
> 
> -- 
> Peter Xu

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:

[adding cleber]

> On Thu, May 18, 2023 at 09:27:01AM -0400, Peter Xu wrote:
>> One thing we can also do to avoid it in the future is simply having someone
>> do this check around each softfreeze (and we'll also need maintainers be
>> careful on merging anything that's risky though after softfreeze) rather
>> than after release (what I did for this time, which is late), try to cover
>> as much devices as possible. I don't know whether there's a way to always
>> cover all devices.
>> 
>> I'll volunteer myself for that as long as I'll remember.  Juan, please also
>> have a check or remind me if I didn't. :)
>> 
>> I am not sure whether I mentioned it somewhere before, but maybe it'll work
>> if we can also have some way we check migrating each of the vmsd from
>> old-qemu to new-qemu (and also new->old) covering all devices.  It doesn't
>> need to be a real migration, just generate the per-device stream and try
>> loading on the other binary.
>> 
>> It might be an overkill to be part of CI to check each commit, but if
>> there's some way to check it then at least we can run it also after
>> softfreeze.  I also don't know whether it'll be easy to achieve it at all,
>> but I'll think more about it too and update if I found something useful.
>
> I'm all for more testing but if it does not actually test that the
> values loaded are actually used correctly then the testing is of course
> lower quality. Better than nothing I guess ...

As said in my other email.  The problem that I have is that there is no
easy way to test two different qemu binaries. And what we would like is
testing at least:

qemu-8.0 -M pc-8.0 -> qemu-latest -M pc-8.0
qemu-latest -M pc-8.0 -> qemu-8.0 -M pc-8.0

Then we have the probem of:
- what architectures do we care?
  My bet is on x86_64, i386? (almost free with previous one), ppc64,
  aarch64, s390.
- what devices do we care?
  virtio-devices for sure.  Not sure with others.  Notice that if we
  want the test to run on CI, they need to be virtual devices.

Once the mechanism of testing and reporting is there, we can decide:
- what other devices we care
  no good ideas here: SATA, e1000e perhaps.
- what other versions we care
  perhaps it is not out of qestion to test the last two
  versions. i.e. to add:
  $ qemu-7.2 -M pc-7.2 -> qemu-latest -M pc-7.2
  $ qemu-latest -M pc-7.2 -> qemu-7.2 -M pc-7.2
  But we haven't been very good just allowing the latest version to add
  the latests two.

Cleber, any idea how difficult would be to add something like that?

For reference, we are trying to test, detect the failures described
here:

https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03285.html

See the three emails.

Thanks, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 18, 2023 at 01:33:43PM +0200, Juan Quintela wrote:
>> See patch for documentation:
>> 
>> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03288.html
>> 
>> Basically, the best we can do is:
>> - get the patch posted.  Fixes everything except:
>>   (3) qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2 works
>> 
>> And for that, we can document somewhere that we need to launch
>> qemu-8.0.1 as:
>> 
>> $ qemu-8.0.1 -M pc-7.2 -device blah,x-pci-err-unc-mask=on
>
> One thing we can also do to avoid it in the future is simply having someone
> do this check around each softfreeze (and we'll also need maintainers be
> careful on merging anything that's risky though after softfreeze) rather
> than after release (what I did for this time, which is late), try to cover
> as much devices as possible. I don't know whether there's a way to always
> cover all devices.
>
> I'll volunteer myself for that as long as I'll remember.  Juan, please also
> have a check or remind me if I didn't. :)
>
> I am not sure whether I mentioned it somewhere before, but maybe it'll work
> if we can also have some way we check migrating each of the vmsd from
> old-qemu to new-qemu (and also new->old) covering all devices.  It doesn't
> need to be a real migration, just generate the per-device stream and try
> loading on the other binary.
>
> It might be an overkill to be part of CI to check each commit, but if
> there's some way to check it then at least we can run it also after
> softfreeze.  I also don't know whether it'll be easy to achieve it at all,
> but I'll think more about it too and update if I found something useful.

Hi Peter

First, thanks for volunteering.

And next, I think this is done better as part of avocado.  Several
reasons:
- We need two different qemu's
- We want to run it perhaps daily.
- We want to report any problem.

I will start with something really simply.  Like getting the
migration-test tests cases that we have, and just run them in both
directions.  I.e. new -> old and old -> new.

That will give us a baseline:
- x86_64
- i386
- aarch64
- ppc
- s390

I think nothing else cares about versioned machine types right now.

Once the mechanism is working and the reporting is sent somewhere, we
can go from there and add machines with the devices that we care about.

But just the example that I showed would have detected the problem that
we are talking about.

After that I would make sure that we are checking all virtio devices,
with/without vhost.

And once we have done that, the device authors that care about their
devices will add test to the infrastructure.

Later, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael S. Tsirkin]

On Thu, May 11, 2023 at 10:27:35AM +0200, Fiona Ebner wrote:
> Am 03.05.23 um 02:27 schrieb Leonardo Bras:
> > Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> > set for machine types < 8.0 will cause migration to fail if the target
> > QEMU version is < 8.0.0 :
> > 
> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> > qemu-system-x86_64: Failed to load PCIDevice:config
> > qemu-system-x86_64: Failed to load e1000e:parent_obj
> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> > qemu-system-x86_64: load of migration failed: Invalid argument
> > 
> > The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> > with this cmdline:
> > 
> > ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> > 
> > In order to fix this, property x-pcie-err-unc-mask was introduced to
> > control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> > default, but is disabled if machine type <= 7.2.
> > 
> > Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> 
> Thank you for the patch!
> 
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
> 
> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
> when using machine type <= 7.2. That is because after this patch, when
> using machine type <= 7.2, the wmask for the register is not set and
> when 8.0 sends a nonzero value for the register, the error condition in
> get_pci_config_device() will trigger again.
> 
> Is it necessary to also handle that? Maybe by special casing the error
> condition in get_pci_config_device() to be prepared to accept such a
> stream from 8.0?
> 
> If that is considered not worth it, consider this:
> 
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> 
> Best Regards,
> Fiona

Yes any fix is like that. We keep encountering bugs like this
but there does not seem to be will to create infrastructure
for fixing it, which would involve describing
version of qemu being migrated to.

-- 
MST

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:

[Added libvirt people to the party, see the end of the message ]

> On Thu, May 11, 2023 at 10:27:35AM +0200, Fiona Ebner wrote:
>> Am 03.05.23 um 02:27 schrieb Leonardo Bras:
>> > Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
>> > set for machine types < 8.0 will cause migration to fail if the target
>> > QEMU version is < 8.0.0 :
>> > 
>> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
>> > qemu-system-x86_64: Failed to load PCIDevice:config
>> > qemu-system-x86_64: Failed to load e1000e:parent_obj
>> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
>> > qemu-system-x86_64: load of migration failed: Invalid argument
>> > 
>> > The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
>> > with this cmdline:
>> > 
>> > ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
>> > 
>> > In order to fix this, property x-pcie-err-unc-mask was introduced to
>> > control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
>> > default, but is disabled if machine type <= 7.2.
>> > 
>> > Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
>> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
>> 
>> Thank you for the patch!
>> 
>> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
>> 
>> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
>> when using machine type <= 7.2. That is because after this patch, when
>> using machine type <= 7.2, the wmask for the register is not set and
>> when 8.0 sends a nonzero value for the register, the error condition in
>> get_pci_config_device() will trigger again.
>> 
>> Is it necessary to also handle that? Maybe by special casing the error
>> condition in get_pci_config_device() to be prepared to accept such a
>> stream from 8.0?
>> 
>> If that is considered not worth it, consider this:
>> 
>> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>> 
>> Best Regards,
>> Fiona
>
> Yes any fix is like that. We keep encountering bugs like this
> but there does not seem to be will to create infrastructure
> for fixing it, which would involve describing
> version of qemu being migrated to.

You have it.

qemu-8.0 -M pc-7.2 -> is going to migrate to an older qemu, so be extra
careful.

We tried (and faield) that you could always migrate to the next version
without changing the machine type.

That was a myth that never really worked, but sometimes it did.

Now we say that we only support migration with same machine type,
period.  And then you know what you are going to do.

And what we are debating here is something different.

$ qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-7.2

We broke it.  We don't have a time machine.  We can fix that for 8.0.

And we have a fix.  But what the fix does is:

$ qemu-7.2 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2

works again.

But what Fiona was asking for is:

$ qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2

is broken right now.  But we can "fix" it.
But that implies that we change pci to allow both the correct value
(the old one, what qemu-7.2 sends), and the new value that qemu-8.0 -M
pc-7.2 sends.  Yes, it is a kludge.  That is why I said that it depended
on what PCI maintainers think.

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 103667c368..374d593ead 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -112,10 +112,13 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
uint16_t offset,
 
     pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
                  PCI_ERR_UNC_SUPPORTED);
-    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
-                 PCI_ERR_UNC_MASK_DEFAULT);
-    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
-                 PCI_ERR_UNC_SUPPORTED);
+
+    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
+        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
+                     PCI_ERR_UNC_MASK_DEFAULT);
+        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
+                     PCI_ERR_UNC_SUPPORTED);
+    }
 
     pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
                  PCI_ERR_UNC_SEVERITY_DEFAULT);

We do this at init time.
But we need to a migration incoming time, that for old machine types we
can find that this has been set up.  So they need to accept both ways.
And this is in the middle of the pci configuration space.

I have no clue how to do this and be sure that it is a valid pci
configuration afterwards.

With my migration knowledge, I know that the problem is in this function
(ok, I know th error message gave you a big clue):

static int get_pci_config_device(QEMUFile *f, void *pv, size_t size,
                                 const VMStateField *field)
{
    PCIDevice *s = container_of(pv, PCIDevice, config);
    uint8_t *config;
    int i;

    assert(size == pci_config_size(s));
    config = g_malloc(size);

    qemu_get_buffer(f, config, size);
    for (i = 0; i < size; ++i) {
        if ((config[i] ^ s->config[i]) &
            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {

-----> here --->>
            error_report("%s: Bad config data: i=0x%x read: %x device: %x "
                         "cmask: %x wmask: %x w1cmask:%x", __func__,
                         i, config[i], s->config[i],
                         s->cmask[i], s->wmask[i], s->w1cmask[i]);
            g_free(config);
            return -EINVAL;
        }
    }
    memcpy(s->config, config, size);

    pci_update_mappings(s);
    if (IS_PCI_BRIDGE(s)) {
        pci_bridge_update_mappings(PCI_BRIDGE(s));
    }

    memory_region_set_enabled(&s->bus_master_enable_region,
                              pci_get_word(s->config + PCI_COMMAND)
                              & PCI_COMMAND_MASTER);

    g_free(config);
    return 0;
}

But what is the kludge that I have to do to put in "here" a condition
that says:

if the problem is at dev->config + offset + PCI_ERR_UNCOR_MASK that is
not enabled in local (target of migration) device but it was enabled in remote
(source of migration) (the same for wmask)

+    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
+        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
+                     PCI_ERR_UNC_MASK_DEFAULT);
+        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
+                     PCI_ERR_UNC_SUPPORTED);
+    }

just enable it locally and don't give an error.  And we have to maintain
the kludge forever because every machine with a machine type older than
qemu-8.0 that has started in qemu-8.0 (i.e. pre qemu-8.0.1 that has the
fix) is going to have that enable when the machine definition says that
it should be disabled.

That would fix the:

qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2

It is worth it?  Dunno.  That is my question.

And knowing from what qemu it has migrated from would not help.  We
would need to add a new tweak and means:

This is a pc-7.2 machine that has been isntantiated in a qemu-8.0 and
has the pciaerr bug.  But wait, we have _that_.

And it is called

+    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },

from the patch.

We can teach libvirt about this glitch, and if he is migrating a pc-7.2
machine in qemu-8.0 machine, And they want to migrate to a new qemu
(call it qemu-8.1), it needs to be started:

qemu-8.1 -M pc-7.2 <whatever pci devices need to do>,x-pci-err-unc-mask="true"

Until the user reboots it and then that property can be reset to default
value.

So at the end, pci maintainers don't need to do anything (so they don't
have to kill me).

And now it is the libvirt people who is going to try to kill me.

Sniff.

It is worth it?  I don't know.

But as seen, migration has the infrastructure to do it. But it is a very
hard problem, and fixing it in general is very complicated.  We are very
bad at testing that we have broken migration for

qemu-X.Y -M pc-X.Y -> qemu-X.(Y+1) -M pc-X.Y

Sorry for the long mail, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael S. Tsirkin]

On Thu, May 11, 2023 at 01:43:47PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> [Added libvirt people to the party, see the end of the message ]
> 
> > On Thu, May 11, 2023 at 10:27:35AM +0200, Fiona Ebner wrote:
> >> Am 03.05.23 um 02:27 schrieb Leonardo Bras:
> >> > Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
> >> > set for machine types < 8.0 will cause migration to fail if the target
> >> > QEMU version is < 8.0.0 :
> >> > 
> >> > qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 device: 0 cmask: ff wmask: 0 w1cmask:0
> >> > qemu-system-x86_64: Failed to load PCIDevice:config
> >> > qemu-system-x86_64: Failed to load e1000e:parent_obj
> >> > qemu-system-x86_64: error while loading state for instance 0x0 of device '0000:00:02.0/e1000e'
> >> > qemu-system-x86_64: load of migration failed: Invalid argument
> >> > 
> >> > The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
> >> > with this cmdline:
> >> > 
> >> > ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> >> > 
> >> > In order to fix this, property x-pcie-err-unc-mask was introduced to
> >> > control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
> >> > default, but is disabled if machine type <= 7.2.
> >> > 
> >> > Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
> >> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> >> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> >> 
> >> Thank you for the patch!
> >> 
> >> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1576
> >> 
> >> AFAICT, this breaks (forward) migration from 8.0 to 8.0 + this patch
> >> when using machine type <= 7.2. That is because after this patch, when
> >> using machine type <= 7.2, the wmask for the register is not set and
> >> when 8.0 sends a nonzero value for the register, the error condition in
> >> get_pci_config_device() will trigger again.
> >> 
> >> Is it necessary to also handle that? Maybe by special casing the error
> >> condition in get_pci_config_device() to be prepared to accept such a
> >> stream from 8.0?
> >> 
> >> If that is considered not worth it, consider this:
> >> 
> >> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> >> 
> >> Best Regards,
> >> Fiona
> >
> > Yes any fix is like that. We keep encountering bugs like this
> > but there does not seem to be will to create infrastructure
> > for fixing it, which would involve describing
> > version of qemu being migrated to.
> 
> You have it.
> 
> qemu-8.0 -M pc-7.2 -> is going to migrate to an older qemu, so be extra
> careful.
> 
> We tried (and faield) that you could always migrate to the next version
> without changing the machine type.
> 
> That was a myth that never really worked, but sometimes it did.
> 
> Now we say that we only support migration with same machine type,
> period.  And then you know what you are going to do.
> 
> And what we are debating here is something different.
> 
> $ qemu-7.2 -M pc-7.2 -> qemu-8.0 -M pc-7.2
> 
> We broke it.  We don't have a time machine.  We can fix that for 8.0.
> 
> And we have a fix.  But what the fix does is:
> 
> $ qemu-7.2 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2
> 
> works again.
> 
> But what Fiona was asking for is:
> 
> $ qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2
> 
> is broken right now.  But we can "fix" it.
> But that implies that we change pci to allow both the correct value
> (the old one, what qemu-7.2 sends), and the new value that qemu-8.0 -M
> pc-7.2 sends.  Yes, it is a kludge.  That is why I said that it depended
> on what PCI maintainers think.
> 
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index 103667c368..374d593ead 100644
> --- a/hw/pci/pcie_aer.c
> +++ b/hw/pci/pcie_aer.c
> @@ -112,10 +112,13 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver,
> uint16_t offset,
>  
>      pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
>                   PCI_ERR_UNC_SUPPORTED);
> -    pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> -                 PCI_ERR_UNC_MASK_DEFAULT);
> -    pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> -                 PCI_ERR_UNC_SUPPORTED);
> +
> +    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
> +        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> +                     PCI_ERR_UNC_MASK_DEFAULT);
> +        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> +                     PCI_ERR_UNC_SUPPORTED);
> +    }
>  
>      pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
>                   PCI_ERR_UNC_SEVERITY_DEFAULT);
> 
> We do this at init time.
> But we need to a migration incoming time, that for old machine types we
> can find that this has been set up.  So they need to accept both ways.
> And this is in the middle of the pci configuration space.
> 
> I have no clue how to do this and be sure that it is a valid pci
> configuration afterwards.
> 
> With my migration knowledge, I know that the problem is in this function
> (ok, I know th error message gave you a big clue):
> 
> static int get_pci_config_device(QEMUFile *f, void *pv, size_t size,
>                                  const VMStateField *field)
> {
>     PCIDevice *s = container_of(pv, PCIDevice, config);
>     uint8_t *config;
>     int i;
> 
>     assert(size == pci_config_size(s));
>     config = g_malloc(size);
> 
>     qemu_get_buffer(f, config, size);
>     for (i = 0; i < size; ++i) {
>         if ((config[i] ^ s->config[i]) &
>             s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {
> 
> -----> here --->>
>             error_report("%s: Bad config data: i=0x%x read: %x device: %x "
>                          "cmask: %x wmask: %x w1cmask:%x", __func__,
>                          i, config[i], s->config[i],
>                          s->cmask[i], s->wmask[i], s->w1cmask[i]);
>             g_free(config);
>             return -EINVAL;
>         }
>     }
>     memcpy(s->config, config, size);
> 
>     pci_update_mappings(s);
>     if (IS_PCI_BRIDGE(s)) {
>         pci_bridge_update_mappings(PCI_BRIDGE(s));
>     }
> 
>     memory_region_set_enabled(&s->bus_master_enable_region,
>                               pci_get_word(s->config + PCI_COMMAND)
>                               & PCI_COMMAND_MASTER);
> 
>     g_free(config);
>     return 0;
> }
> 
> But what is the kludge that I have to do to put in "here" a condition
> that says:
> 
> if the problem is at dev->config + offset + PCI_ERR_UNCOR_MASK that is
> not enabled in local (target of migration) device but it was enabled in remote
> (source of migration) (the same for wmask)
> 
> +    if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
> +        pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> +                     PCI_ERR_UNC_MASK_DEFAULT);
> +        pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> +                     PCI_ERR_UNC_SUPPORTED);
> +    }
> 
> just enable it locally and don't give an error.  And we have to maintain
> the kludge forever because every machine with a machine type older than
> qemu-8.0 that has started in qemu-8.0 (i.e. pre qemu-8.0.1 that has the
> fix) is going to have that enable when the machine definition says that
> it should be disabled.
> 
> That would fix the:
> 
> qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2
> 
> It is worth it?  Dunno.  That is my question.
> 
> And knowing from what qemu it has migrated from would not help.  We
> would need to add a new tweak and means:
> 
> This is a pc-7.2 machine that has been isntantiated in a qemu-8.0 and
> has the pciaerr bug.  But wait, we have _that_.
> 
> And it is called
> 
> +    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> 
> from the patch.
> 
> We can teach libvirt about this glitch, and if he is migrating a pc-7.2
> machine in qemu-8.0 machine, And they want to migrate to a new qemu
> (call it qemu-8.1), it needs to be started:
> 
> qemu-8.1 -M pc-7.2 <whatever pci devices need to do>,x-pci-err-unc-mask="true"
> 
> Until the user reboots it and then that property can be reset to default
> value.
> 
> So at the end, pci maintainers don't need to do anything (so they don't
> have to kill me).
> 
> And now it is the libvirt people who is going to try to kill me.
> 
> Sniff.
> 
> It is worth it?  I don't know.
> 
> But as seen, migration has the infrastructure to do it. But it is a very
> hard problem, and fixing it in general is very complicated.  We are very
> bad at testing that we have broken migration for
> 
> qemu-X.Y -M pc-X.Y -> qemu-X.(Y+1) -M pc-X.Y
> 
> Sorry for the long mail, Juan.

Great summary thanks!
Yes the infrastructure is there but it's too hard to use.
qemu knows which versions need which properties but libvirt does not.


-- 
MST

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Jiri Denemark]

On Thu, May 11, 2023 at 13:43:47 +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> [Added libvirt people to the party, see the end of the message ]

Sorry, I'm not that much into parties :-)

> That would fix the:
> 
> qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2
> 
> It is worth it?  Dunno.  That is my question.
> 
> And knowing from what qemu it has migrated from would not help.  We
> would need to add a new tweak and means:
> 
> This is a pc-7.2 machine that has been isntantiated in a qemu-8.0 and
> has the pciaerr bug.  But wait, we have _that_.
> 
> And it is called
> 
> +    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> 
> from the patch.
> 
> We can teach libvirt about this glitch, and if he is migrating a pc-7.2
> machine in qemu-8.0 machine, And they want to migrate to a new qemu
> (call it qemu-8.1), it needs to be started:
> 
> qemu-8.1 -M pc-7.2 <whatever pci devices need to do>,x-pci-err-unc-mask="true"
> 
> Until the user reboots it and then that property can be reset to default
> value.

Hmm and what would happen if eventually this machine gets migrated back
to qemu-8.0? Or even when the machine is stopped, started again, and
then migrated to qemu-8.0?

Jirka

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Juan Quintela]

Jiri Denemark <jdenemar@redhat.com> wrote:
> On Thu, May 11, 2023 at 13:43:47 +0200, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> [Added libvirt people to the party, see the end of the message ]
>
> Sorry, I'm not that much into parties :-)
>
>> That would fix the:
>> 
>> qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2
>> 
>> It is worth it?  Dunno.  That is my question.
>> 
>> And knowing from what qemu it has migrated from would not help.  We
>> would need to add a new tweak and means:
>> 
>> This is a pc-7.2 machine that has been isntantiated in a qemu-8.0 and
>> has the pciaerr bug.  But wait, we have _that_.
>> 
>> And it is called
>> 
>> +    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
>> 
>> from the patch.
>> 
>> We can teach libvirt about this glitch, and if he is migrating a pc-7.2
>> machine in qemu-8.0 machine, And they want to migrate to a new qemu
>> (call it qemu-8.1), it needs to be started:
>> 
>> qemu-8.1 -M pc-7.2 <whatever pci devices need to do>,x-pci-err-unc-mask="true"
>> 
>> Until the user reboots it and then that property can be reset to default
>> value.
>
> Hmm and what would happen if eventually this machine gets migrated back
> to qemu-8.0?

It works.
migrating to qemu-7.2 is what is not going to work.
To migrate to qemu-8.0, you just need to drop the
"x-pci-err-unc-mask=true" bit.  And it would work.

So, to be clear, this machine can migrate to:

- qemu-8.0, you just need to drop the "x-pci-err-unc-mask=true" bit

- qemu-8.0.1 or newer, you just need to maintain the
  "x-pci-err-unc-mask=true" bit.

Let's just assume that qemu-7.2.1 don't get the
"x-pci-err-unc-mask=true" bit, so it will not be able to migrate there.


> Or even when the machine is stopped, started again, and
> then migrated to qemu-8.0?

If you do what I call a hard reset (i.e. poweroff + poweron so qemu
dies), you should drop the "x-pci-err-unc-mask=true" bit.  And then you
can migrate to qemu-7.2 and all qemu-8.0.1 and newer.

Basically what we need is a "mark" inside libvirt that means something
like:

- this is weird machine that looks like pc-7.2
- but has "x-pci-err-unc-mask=true"
- so it can only migrate to qemu-8.0 and newer.
- but if it even reboots in qemu-8.0.1 or newer, we want it back to
  become a "normal" pc-7.2 machine (i.e. drop the
  x-pci-err-unc-mask=true).

That would be the perfect world.  But as we are in an imperfect world,
something like:

- this machine started in qemu-8.0 -M pc-7.2, we know this is broken and
  it can't migrate outside of qemu-8.0 because it would fail to go to
  either qemu-7.2 or qemu-8.0.1.

I would argue that if you do the second option doing the "right" option
i.e. the first one is not much more complicated, but that is a question
that you should be better to answer.

And then we have the other Michael question.  How can we export that
information so libvirt can use it.

In this case we can comunicate libvirt:
- In qemu-8.0 we broke pc-7.2.
- The problem is fixed in qemu-8.0.1 using property
  "x-pci-err-unc-mask=false".
- You can migrate from qemu-8.0 in newer if you set that property as
  true.
- Guests started in qemu-8.0 -M pc-7.2 should reboot in qemu-8.0.1 or
  newer to become "normal pc-7.2".
- If we publish this on qemu, we can only publish it on qemu-8.0.1 and
  newer.
- Or we can publish it somewhere else and any libvirt can take this
  information.
- Or we can comunicate this to libvirt, and they incorporate it on their
  source anywhere that you see fit.

The point here is that when we use a property on a machine type, it can
be for two reasons:

- We detected at the right time that we changed the value of something,
  and we did the right thing on hw_compat_X_Y, so libvirt needs to do
  nothing.

- We *DID NOT* detect that we broke compatibility before release, and we
  need to make a property to identify that problem.  This is where we
  need to do this dance.

Notice that normally we detect lots of problems during development and
this *should* not happen.  But when it happens, we need to be able to do
something.

And also notice that normally we broke just some device, not a whole
machine type.  But as you can see we have broke it this time.  We are
trying to automate the detection of this kind of failures, but we are
still on design stage, so we need to plan how to handle this.

Any comments?

Later, Juan.

Re: [PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Michael S. Tsirkin]

On Fri, May 26, 2023 at 09:55:22AM +0200, Juan Quintela wrote:
> Jiri Denemark <jdenemar@redhat.com> wrote:
> > On Thu, May 11, 2023 at 13:43:47 +0200, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> [Added libvirt people to the party, see the end of the message ]
> >
> > Sorry, I'm not that much into parties :-)
> >
> >> That would fix the:
> >> 
> >> qemu-8.0 -M pc-7.2 -> qemu-8.0.1 -M pc-7.2
> >> 
> >> It is worth it?  Dunno.  That is my question.
> >> 
> >> And knowing from what qemu it has migrated from would not help.  We
> >> would need to add a new tweak and means:
> >> 
> >> This is a pc-7.2 machine that has been isntantiated in a qemu-8.0 and
> >> has the pciaerr bug.  But wait, we have _that_.
> >> 
> >> And it is called
> >> 
> >> +    { TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
> >> 
> >> from the patch.
> >> 
> >> We can teach libvirt about this glitch, and if he is migrating a pc-7.2
> >> machine in qemu-8.0 machine, And they want to migrate to a new qemu
> >> (call it qemu-8.1), it needs to be started:
> >> 
> >> qemu-8.1 -M pc-7.2 <whatever pci devices need to do>,x-pci-err-unc-mask="true"
> >> 
> >> Until the user reboots it and then that property can be reset to default
> >> value.
> >
> > Hmm and what would happen if eventually this machine gets migrated back
> > to qemu-8.0?
> 
> It works.
> migrating to qemu-7.2 is what is not going to work.
> To migrate to qemu-8.0, you just need to drop the
> "x-pci-err-unc-mask=true" bit.  And it would work.
> 
> So, to be clear, this machine can migrate to:
> 
> - qemu-8.0, you just need to drop the "x-pci-err-unc-mask=true" bit
> 
> - qemu-8.0.1 or newer, you just need to maintain the
>   "x-pci-err-unc-mask=true" bit.
> 
> Let's just assume that qemu-7.2.1 don't get the
> "x-pci-err-unc-mask=true" bit, so it will not be able to migrate there.
> 
> 
> > Or even when the machine is stopped, started again, and
> > then migrated to qemu-8.0?
> 
> If you do what I call a hard reset (i.e. poweroff + poweron so qemu
> dies), you should drop the "x-pci-err-unc-mask=true" bit.  And then you
> can migrate to qemu-7.2 and all qemu-8.0.1 and newer.
> 
> Basically what we need is a "mark" inside libvirt that means something
> like:
> 
> - this is weird machine that looks like pc-7.2
> - but has "x-pci-err-unc-mask=true"
> - so it can only migrate to qemu-8.0 and newer.
> - but if it even reboots in qemu-8.0.1 or newer, we want it back to
>   become a "normal" pc-7.2 machine (i.e. drop the
>   x-pci-err-unc-mask=true).
> 
> That would be the perfect world.  But as we are in an imperfect world,
> something like:
> 
> - this machine started in qemu-8.0 -M pc-7.2, we know this is broken and
>   it can't migrate outside of qemu-8.0 because it would fail to go to
>   either qemu-7.2 or qemu-8.0.1.
> 
> I would argue that if you do the second option doing the "right" option
> i.e. the first one is not much more complicated, but that is a question
> that you should be better to answer.
> 
> And then we have the other Michael question.  How can we export that
> information so libvirt can use it.
> 
> In this case we can comunicate libvirt:
> - In qemu-8.0 we broke pc-7.2.
> - The problem is fixed in qemu-8.0.1 using property
>   "x-pci-err-unc-mask=false".
> - You can migrate from qemu-8.0 in newer if you set that property as
>   true.
> - Guests started in qemu-8.0 -M pc-7.2 should reboot in qemu-8.0.1 or
>   newer to become "normal pc-7.2".
> - If we publish this on qemu, we can only publish it on qemu-8.0.1 and
>   newer.
> - Or we can publish it somewhere else and any libvirt can take this
>   information.
> - Or we can comunicate this to libvirt, and they incorporate it on their
>   source anywhere that you see fit.

And this is not an isolated instance. There are things like this in
almost each release.


My suggestion is a package with known bugs like this.
It would list these work arounds in some machine readable
format and would be essentially append only, making it
relatively safe even for very old RHEL distros to
pick up the latest version once in a while.

E.g. the fact we add bug workaround for 10.0 will not affect
7.2 so you do not need to fork with each release.




> The point here is that when we use a property on a machine type, it can
> be for two reasons:
> 
> - We detected at the right time that we changed the value of something,
>   and we did the right thing on hw_compat_X_Y, so libvirt needs to do
>   nothing.
> 
> - We *DID NOT* detect that we broke compatibility before release, and we
>   need to make a property to identify that problem.  This is where we
>   need to do this dance.
> 
> Notice that normally we detect lots of problems during development and
> this *should* not happen.  But when it happens, we need to be able to do
> something.
> 
> And also notice that normally we broke just some device, not a whole
> machine type.  But as you can see we have broke it this time.  We are
> trying to automate the detection of this kind of failures, but we are
> still on design stage, so we need to plan how to handle this.
> 
> Any comments?
> 
> Later, Juan.
> 
> 
> 
>