Patchwork [qom-next,for-next,v2,2/6] pci: Simplify VMSTATE_PCIE_DEVICE() macro

login
register
mail settings
Submitter Andreas Färber
Date July 29, 2013, 12:26 a.m.
Message ID <1375057621-19961-3-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/262632/
State New
Headers show

Comments

Andreas Färber - July 29, 2013, 12:26 a.m.
Drop the arguments to avoid QOM refactorings causing more churn.

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/usb/hcd-xhci.c                  | 2 +-
 include/hw/pci/pcie.h              | 6 +++---
 5 files changed, 7 insertions(+), 7 deletions(-)
Michael S. Tsirkin - Sept. 2, 2013, 11:36 a.m.
On Mon, Jul 29, 2013 at 02:26:57AM +0200, Andreas Färber wrote:
> Drop the arguments to avoid QOM refactorings causing more churn.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

We discussed and discarded something very similar.

This only works correctly if you put it in a struct which has the pci
device at offset 0, and there's no either runtime or compile-time check
to verify it's the correct type.

So please find some other way to do this.
Or go back to give fields sane names instead
of "parent_obj".


> ---
>  hw/pci-bridge/ioh3420.c            | 2 +-
>  hw/pci-bridge/xio3130_downstream.c | 2 +-
>  hw/pci-bridge/xio3130_upstream.c   | 2 +-
>  hw/usb/hcd-xhci.c                  | 2 +-
>  include/hw/pci/pcie.h              | 6 +++---
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index e07c7e8..0657354 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -187,7 +187,7 @@ static const VMStateDescription vmstate_ioh3420 = {
>      .minimum_version_id_old = 1,
>      .post_load = pcie_cap_slot_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> +        VMSTATE_PCIE_DEVICE(),
>          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..9022949 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -154,7 +154,7 @@ static const VMStateDescription vmstate_xio3130_downstream = {
>      .minimum_version_id_old = 1,
>      .post_load = pcie_cap_slot_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> +        VMSTATE_PCIE_DEVICE(),
>          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..046b790 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -133,7 +133,7 @@ static const VMStateDescription vmstate_xio3130_upstream = {
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort),
> +        VMSTATE_PCIE_DEVICE(),
>          VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort, 0,
>                         vmstate_pcie_aer_log, PCIEAERLog),
>          VMSTATE_END_OF_LIST()
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 9ba3e3e..a22861f 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3544,7 +3544,7 @@ static const VMStateDescription vmstate_xhci = {
>      .version_id = 1,
>      .post_load = usb_xhci_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
> +        VMSTATE_PCIE_DEVICE(),
>          VMSTATE_MSIX(parent_obj, XHCIState),
>  
>          VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1,
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index c010007..fc4ebd3 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -132,12 +132,12 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
>  
>  extern const VMStateDescription vmstate_pcie_device;
>  
> -#define VMSTATE_PCIE_DEVICE(_field, _state) {                        \
> -    .name       = (stringify(_field)),                               \
> +#define VMSTATE_PCIE_DEVICE() {                                      \
> +    .name       = "parent_obj",                                      \
>      .size       = sizeof(PCIDevice),                                 \
>      .vmsd       = &vmstate_pcie_device,                              \
>      .flags      = VMS_STRUCT,                                        \
> -    .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
> +    .offset     = 0,                                                 \
>  }
>  
>  #endif /* QEMU_PCIE_H */
> -- 
> 1.8.1.4
Andreas Färber - Sept. 2, 2013, 11:38 a.m.
Am 02.09.2013 13:36, schrieb Michael S. Tsirkin:
> On Mon, Jul 29, 2013 at 02:26:57AM +0200, Andreas Färber wrote:
>> Drop the arguments to avoid QOM refactorings causing more churn.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> We discussed and discarded something very similar.

Anthony requested that I don't mix different types for the offsets in
VMSTATE_*() macros.

> 
> This only works correctly if you put it in a struct which has the pci
> device at offset 0, and there's no either runtime or compile-time check
> to verify it's the correct type.

As Anthony already said, QOM depends on the offset being 0, casts will
fail otherwise. So there are runtime checks.

> 
> So please find some other way to do this.

I did post an RFC, but that is significantly more invasive, since it
affects not only PCI and needs a lot of review before we can move
forward with it.

Andreas

> Or go back to give fields sane names instead
> of "parent_obj".
> 
> 
>> ---
>>  hw/pci-bridge/ioh3420.c            | 2 +-
>>  hw/pci-bridge/xio3130_downstream.c | 2 +-
>>  hw/pci-bridge/xio3130_upstream.c   | 2 +-
>>  hw/usb/hcd-xhci.c                  | 2 +-
>>  include/hw/pci/pcie.h              | 6 +++---
>>  5 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
>> index e07c7e8..0657354 100644
>> --- a/hw/pci-bridge/ioh3420.c
>> +++ b/hw/pci-bridge/ioh3420.c
>> @@ -187,7 +187,7 @@ static const VMStateDescription vmstate_ioh3420 = {
>>      .minimum_version_id_old = 1,
>>      .post_load = pcie_cap_slot_post_load,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
>> +        VMSTATE_PCIE_DEVICE(),
>>          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..9022949 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -154,7 +154,7 @@ static const VMStateDescription vmstate_xio3130_downstream = {
>>      .minimum_version_id_old = 1,
>>      .post_load = pcie_cap_slot_post_load,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
>> +        VMSTATE_PCIE_DEVICE(),
>>          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..046b790 100644
>> --- a/hw/pci-bridge/xio3130_upstream.c
>> +++ b/hw/pci-bridge/xio3130_upstream.c
>> @@ -133,7 +133,7 @@ static const VMStateDescription vmstate_xio3130_upstream = {
>>      .minimum_version_id = 1,
>>      .minimum_version_id_old = 1,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort),
>> +        VMSTATE_PCIE_DEVICE(),
>>          VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort, 0,
>>                         vmstate_pcie_aer_log, PCIEAERLog),
>>          VMSTATE_END_OF_LIST()
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 9ba3e3e..a22861f 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -3544,7 +3544,7 @@ static const VMStateDescription vmstate_xhci = {
>>      .version_id = 1,
>>      .post_load = usb_xhci_post_load,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
>> +        VMSTATE_PCIE_DEVICE(),
>>          VMSTATE_MSIX(parent_obj, XHCIState),
>>  
>>          VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1,
>> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
>> index c010007..fc4ebd3 100644
>> --- a/include/hw/pci/pcie.h
>> +++ b/include/hw/pci/pcie.h
>> @@ -132,12 +132,12 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
>>  
>>  extern const VMStateDescription vmstate_pcie_device;
>>  
>> -#define VMSTATE_PCIE_DEVICE(_field, _state) {                        \
>> -    .name       = (stringify(_field)),                               \
>> +#define VMSTATE_PCIE_DEVICE() {                                      \
>> +    .name       = "parent_obj",                                      \
>>      .size       = sizeof(PCIDevice),                                 \
>>      .vmsd       = &vmstate_pcie_device,                              \
>>      .flags      = VMS_STRUCT,                                        \
>> -    .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
>> +    .offset     = 0,                                                 \
>>  }
>>  
>>  #endif /* QEMU_PCIE_H */
>> -- 
>> 1.8.1.4
Michael S. Tsirkin - Sept. 2, 2013, 11:45 a.m.
On Mon, Sep 02, 2013 at 01:38:25PM +0200, Andreas Färber wrote:
> Am 02.09.2013 13:36, schrieb Michael S. Tsirkin:
> > On Mon, Jul 29, 2013 at 02:26:57AM +0200, Andreas Färber wrote:
> >> Drop the arguments to avoid QOM refactorings causing more churn.
> >>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> > 
> > We discussed and discarded something very similar.
> 
> Anthony requested that I don't mix different types for the offsets in
> VMSTATE_*() macros.

Exactly. The solution is *not* not to put any types there.

> > 
> > This only works correctly if you put it in a struct which has the pci
> > device at offset 0, and there's no either runtime or compile-time check
> > to verify it's the correct type.
> 
> As Anthony already said, QOM depends on the offset being 0, casts will
> fail otherwise. So there are runtime checks.

I don't see any runtime casts in vmstate.
That's exactly the issue. This is a bad API.

vmstate isn't limited to QOM objects, if you make
a macro without arguments it should either make
sense in any vmstate, make compilation fail
if misused, or at least quickly make migration fail
if misused.

Not corrupt random memory.

> > 
> > So please find some other way to do this.
> 
> I did post an RFC, but that is significantly more invasive, since it
> affects not only PCI and needs a lot of review before we can move
> forward with it.
> 
> Andreas

That one looks ok.

> > Or go back to give fields sane names instead
> > of "parent_obj".
> > 
> > 
> >> ---
> >>  hw/pci-bridge/ioh3420.c            | 2 +-
> >>  hw/pci-bridge/xio3130_downstream.c | 2 +-
> >>  hw/pci-bridge/xio3130_upstream.c   | 2 +-
> >>  hw/usb/hcd-xhci.c                  | 2 +-
> >>  include/hw/pci/pcie.h              | 6 +++---
> >>  5 files changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> >> index e07c7e8..0657354 100644
> >> --- a/hw/pci-bridge/ioh3420.c
> >> +++ b/hw/pci-bridge/ioh3420.c
> >> @@ -187,7 +187,7 @@ static const VMStateDescription vmstate_ioh3420 = {
> >>      .minimum_version_id_old = 1,
> >>      .post_load = pcie_cap_slot_post_load,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> >> +        VMSTATE_PCIE_DEVICE(),
> >>          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..9022949 100644
> >> --- a/hw/pci-bridge/xio3130_downstream.c
> >> +++ b/hw/pci-bridge/xio3130_downstream.c
> >> @@ -154,7 +154,7 @@ static const VMStateDescription vmstate_xio3130_downstream = {
> >>      .minimum_version_id_old = 1,
> >>      .post_load = pcie_cap_slot_post_load,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> >> +        VMSTATE_PCIE_DEVICE(),
> >>          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..046b790 100644
> >> --- a/hw/pci-bridge/xio3130_upstream.c
> >> +++ b/hw/pci-bridge/xio3130_upstream.c
> >> @@ -133,7 +133,7 @@ static const VMStateDescription vmstate_xio3130_upstream = {
> >>      .minimum_version_id = 1,
> >>      .minimum_version_id_old = 1,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort),
> >> +        VMSTATE_PCIE_DEVICE(),
> >>          VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort, 0,
> >>                         vmstate_pcie_aer_log, PCIEAERLog),
> >>          VMSTATE_END_OF_LIST()
> >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> >> index 9ba3e3e..a22861f 100644
> >> --- a/hw/usb/hcd-xhci.c
> >> +++ b/hw/usb/hcd-xhci.c
> >> @@ -3544,7 +3544,7 @@ static const VMStateDescription vmstate_xhci = {
> >>      .version_id = 1,
> >>      .post_load = usb_xhci_post_load,
> >>      .fields = (VMStateField[]) {
> >> -        VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
> >> +        VMSTATE_PCIE_DEVICE(),
> >>          VMSTATE_MSIX(parent_obj, XHCIState),
> >>  
> >>          VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1,
> >> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> >> index c010007..fc4ebd3 100644
> >> --- a/include/hw/pci/pcie.h
> >> +++ b/include/hw/pci/pcie.h
> >> @@ -132,12 +132,12 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> >>  
> >>  extern const VMStateDescription vmstate_pcie_device;
> >>  
> >> -#define VMSTATE_PCIE_DEVICE(_field, _state) {                        \
> >> -    .name       = (stringify(_field)),                               \
> >> +#define VMSTATE_PCIE_DEVICE() {                                      \
> >> +    .name       = "parent_obj",                                      \
> >>      .size       = sizeof(PCIDevice),                                 \
> >>      .vmsd       = &vmstate_pcie_device,                              \
> >>      .flags      = VMS_STRUCT,                                        \
> >> -    .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
> >> +    .offset     = 0,                                                 \
> >>  }
> >>  
> >>  #endif /* QEMU_PCIE_H */
> >> -- 
> >> 1.8.1.4
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index e07c7e8..0657354 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -187,7 +187,7 @@  static const VMStateDescription vmstate_ioh3420 = {
     .minimum_version_id_old = 1,
     .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
+        VMSTATE_PCIE_DEVICE(),
         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..9022949 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -154,7 +154,7 @@  static const VMStateDescription vmstate_xio3130_downstream = {
     .minimum_version_id_old = 1,
     .post_load = pcie_cap_slot_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
+        VMSTATE_PCIE_DEVICE(),
         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..046b790 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -133,7 +133,7 @@  static const VMStateDescription vmstate_xio3130_upstream = {
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort),
+        VMSTATE_PCIE_DEVICE(),
         VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort, 0,
                        vmstate_pcie_aer_log, PCIEAERLog),
         VMSTATE_END_OF_LIST()
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 9ba3e3e..a22861f 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3544,7 +3544,7 @@  static const VMStateDescription vmstate_xhci = {
     .version_id = 1,
     .post_load = usb_xhci_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
+        VMSTATE_PCIE_DEVICE(),
         VMSTATE_MSIX(parent_obj, XHCIState),
 
         VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1,
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index c010007..fc4ebd3 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -132,12 +132,12 @@  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
 
 extern const VMStateDescription vmstate_pcie_device;
 
-#define VMSTATE_PCIE_DEVICE(_field, _state) {                        \
-    .name       = (stringify(_field)),                               \
+#define VMSTATE_PCIE_DEVICE() {                                      \
+    .name       = "parent_obj",                                      \
     .size       = sizeof(PCIDevice),                                 \
     .vmsd       = &vmstate_pcie_device,                              \
     .flags      = VMS_STRUCT,                                        \
-    .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
+    .offset     = 0,                                                 \
 }
 
 #endif /* QEMU_PCIE_H */