[Qemu-devel] [PATCH 0/4] e1000e: QOM property & configuration cleanups

[Qemu-devel] [PATCH 0/4] e1000e: QOM property & configuration cleanups

[Eduardo Habkost]

This changes some of the QOM property code in e1000e so it uses
regular property type macros, and the usual QOM property naming
style. This also removes unnecessary configuration validation on
migration/loadvm.

Some of the patches break command-line compatibility, but I am
assuming those properties are not being used in production by
anybody.

Eduardo Habkost (4):
  e1000e: Use regular DEFINE_PROP_<type> macros for properties
  e1000e: No need to validate configuration on migration
  e1000e: Rename "disable_vnet_hdr" property to "vnet"
  e1000e: Rename "subsys_ven" property to "subsys-vendor"

 hw/net/e1000e.c | 50 ++++++++------------------------------------------
 1 file changed, 8 insertions(+), 42 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> macros for properties

[Eduardo Habkost]

Instead of hacking custom PropertyInfo structs, use the regular
DEFINE_PROP_<type> macros for the e1000e properties.

This also fixes a bug in the disable_vnet_hdr property
definition, that was using qdev_prop_uint8 for a bool field.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/net/e1000e.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 4994e1c..df24e55 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
     }
 };
 
-static PropertyInfo e1000e_prop_disable_vnet,
-                    e1000e_prop_subsys_ven,
-                    e1000e_prop_subsys;
-
 static Property e1000e_properties[] = {
     DEFINE_NIC_PROPERTIES(E1000EState, conf),
-    DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
-                        e1000e_prop_disable_vnet, bool),
-    DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
-                        PCI_VENDOR_ID_INTEL,
-                        e1000e_prop_subsys_ven, uint16_t),
-    DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
-                        e1000e_prop_subsys, uint16_t),
+    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
+    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
+    DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
     dc->vmsd = &e1000e_vmstate;
     dc->props = e1000e_properties;
 
-    e1000e_prop_disable_vnet = qdev_prop_uint8;
-    e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
-                                           "perform SW offloads emulation "
-                                           "instead";
-
-    e1000e_prop_subsys_ven = qdev_prop_uint16;
-    e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
-
-    e1000e_prop_subsys = qdev_prop_uint16;
-    e1000e_prop_subsys.description = "PCI device Subsystem ID";
-
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/4] e1000e: No need to validate configuration on migration

[Eduardo Habkost]

The user (or management software) is responsible for keeping the
same configuration on both sides while migrating. Remove the
configuration validation code at e1000e_post_load, and the
unnecessary subsys_used/subsys_ven_used fields.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/net/e1000e.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index df24e55..a932620 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -66,9 +66,6 @@ typedef struct E1000EState {
     uint16_t subsys_ven;
     uint16_t subsys;
 
-    uint16_t subsys_ven_used;
-    uint16_t subsys_used;
-
     bool disable_vnet;
 
     E1000ECore core;
@@ -424,9 +421,6 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
     pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_ven);
     pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, s->subsys);
 
-    s->subsys_ven_used = s->subsys_ven;
-    s->subsys_used = s->subsys;
-
     /* Define IO/MMIO regions */
     memory_region_init_io(&s->mmio, OBJECT(s), &mmio_ops, s,
                           "e1000e-mmio", E1000E_MMIO_SIZE);
@@ -531,14 +525,6 @@ static int e1000e_post_load(void *opaque, int version_id)
 
     trace_e1000e_cb_post_load();
 
-    if ((s->subsys != s->subsys_used) ||
-        (s->subsys_ven != s->subsys_ven_used)) {
-        fprintf(stderr,
-            "ERROR: Cannot migrate while device properties "
-            "(subsys/subsys_ven) differ");
-        return -1;
-    }
-
     return e1000e_core_post_load(&s->core);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] e1000e: Rename "disable_vnet_hdr" property to "vnet"

[Eduardo Habkost]

The original commit that introduced e1000e (6f3fbe4e) mentioned a
property called "vnet". The actual property name added by the
patch is "disable_vnet_hdr". Rename the property so that:

1) we avoid confusing double-negatives like
   "disable_vnet_hdr=false";
2) we avoid underscores in property names.

This breaks command-line compatibility, but I am assuming the
property is not being used in production by anybody.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/net/e1000e.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index a932620..5a711a7 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -66,7 +66,7 @@ typedef struct E1000EState {
     uint16_t subsys_ven;
     uint16_t subsys;
 
-    bool disable_vnet;
+    bool has_vnet;
 
     E1000ECore core;
 
@@ -327,7 +327,7 @@ e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr)
     qemu_format_nic_info_str(qemu_get_queue(s->nic), macaddr);
 
     /* Setup virtio headers */
-    if (s->disable_vnet) {
+    if (!s->has_vnet) {
         s->core.has_vnet = false;
         trace_e1000e_cfg_support_virtio(false);
         return;
@@ -626,7 +626,7 @@ static const VMStateDescription e1000e_vmstate = {
 
 static Property e1000e_properties[] = {
     DEFINE_NIC_PROPERTIES(E1000EState, conf),
-    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
+    DEFINE_PROP_BOOL("vnet", E1000EState, has_vnet, true),
     DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
     DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] e1000e: Rename "subsys_ven" property to "subsys-vendor"

[Eduardo Habkost]

Follow the usual QOM property naming style, and make the property
name clearer.

This breaks command-line compatibility, but I am assuming the
property is not being used in production by anybody.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/net/e1000e.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 5a711a7..0b27da3 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -63,7 +63,7 @@ typedef struct E1000EState {
 
     uint32_t ioaddr;
 
-    uint16_t subsys_ven;
+    uint16_t subsys_vendor;
     uint16_t subsys;
 
     bool has_vnet;
@@ -418,7 +418,7 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
     pci_dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
     pci_dev->config[PCI_INTERRUPT_PIN] = 1;
 
-    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_ven);
+    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_vendor);
     pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, s->subsys);
 
     /* Define IO/MMIO regions */
@@ -524,7 +524,6 @@ static int e1000e_post_load(void *opaque, int version_id)
     E1000EState *s = opaque;
 
     trace_e1000e_cb_post_load();
-
     return e1000e_core_post_load(&s->core);
 }
 
@@ -596,7 +595,7 @@ static const VMStateDescription e1000e_vmstate = {
         VMSTATE_UINT32(core.delayed_causes, E1000EState),
 
         VMSTATE_UINT16(subsys, E1000EState),
-        VMSTATE_UINT16(subsys_ven, E1000EState),
+        VMSTATE_UINT16(subsys_vendor, E1000EState),
 
         VMSTATE_E1000E_INTR_DELAY_TIMER(core.rdtr, E1000EState),
         VMSTATE_E1000E_INTR_DELAY_TIMER(core.radv, E1000EState),
@@ -627,7 +626,7 @@ static const VMStateDescription e1000e_vmstate = {
 static Property e1000e_properties[] = {
     DEFINE_NIC_PROPERTIES(E1000EState, conf),
     DEFINE_PROP_BOOL("vnet", E1000EState, has_vnet, true),
-    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
+    DEFINE_PROP_UINT16("subsys-vendor", E1000EState, subsys_vendor, PCI_VENDOR_ID_INTEL),
     DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> macros for properties

[Jason Wang]

On 2016年10月27日 03:21, Eduardo Habkost wrote:
> Instead of hacking custom PropertyInfo structs, use the regular
> DEFINE_PROP_<type> macros for the e1000e properties.
>
> This also fixes a bug in the disable_vnet_hdr property
> definition, that was using qdev_prop_uint8 for a bool field.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/net/e1000e.c | 25 +++----------------------
>   1 file changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 4994e1c..df24e55 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
>       }
>   };
>   
> -static PropertyInfo e1000e_prop_disable_vnet,
> -                    e1000e_prop_subsys_ven,
> -                    e1000e_prop_subsys;
> -
>   static Property e1000e_properties[] = {
>       DEFINE_NIC_PROPERTIES(E1000EState, conf),
> -    DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
> -                        e1000e_prop_disable_vnet, bool),
> -    DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> -                        PCI_VENDOR_ID_INTEL,
> -                        e1000e_prop_subsys_ven, uint16_t),
> -    DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> -                        e1000e_prop_subsys, uint16_t),
> +    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
> +    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
> +    DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
>       dc->vmsd = &e1000e_vmstate;
>       dc->props = e1000e_properties;
>   
> -    e1000e_prop_disable_vnet = qdev_prop_uint8;
> -    e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
> -                                           "perform SW offloads emulation "
> -                                           "instead";
> -
> -    e1000e_prop_subsys_ven = qdev_prop_uint16;
> -    e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
> -
> -    e1000e_prop_subsys = qdev_prop_uint16;
> -    e1000e_prop_subsys.description = "PCI device Subsystem ID";
> -
>       set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>   }
>   

Is there a way to keep the description here? At least for "vnet" I believe.

Thanks

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> macros for properties

[Dmitry Fleytman]

> On 27 Oct 2016, at 06:12 AM, Jason Wang <jasowang@redhat.com> wrote:
> 
> 
> 
> On 2016年10月27日 03:21, Eduardo Habkost wrote:
>> Instead of hacking custom PropertyInfo structs, use the regular
>> DEFINE_PROP_<type> macros for the e1000e properties.
>> 
>> This also fixes a bug in the disable_vnet_hdr property
>> definition, that was using qdev_prop_uint8 for a bool field.
>> 
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  hw/net/e1000e.c | 25 +++----------------------
>>  1 file changed, 3 insertions(+), 22 deletions(-)
>> 
>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>> index 4994e1c..df24e55 100644
>> --- a/hw/net/e1000e.c
>> +++ b/hw/net/e1000e.c
>> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
>>      }
>>  };
>>  -static PropertyInfo e1000e_prop_disable_vnet,
>> -                    e1000e_prop_subsys_ven,
>> -                    e1000e_prop_subsys;
>> -
>>  static Property e1000e_properties[] = {
>>      DEFINE_NIC_PROPERTIES(E1000EState, conf),
>> -    DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
>> -                        e1000e_prop_disable_vnet, bool),
>> -    DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
>> -                        PCI_VENDOR_ID_INTEL,
>> -                        e1000e_prop_subsys_ven, uint16_t),
>> -    DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
>> -                        e1000e_prop_subsys, uint16_t),
>> +    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
>> +    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
>> +    DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
>>      dc->vmsd = &e1000e_vmstate;
>>      dc->props = e1000e_properties;
>>  -    e1000e_prop_disable_vnet = qdev_prop_uint8;
>> -    e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
>> -                                           "perform SW offloads emulation "
>> -                                           "instead";
>> -
>> -    e1000e_prop_subsys_ven = qdev_prop_uint16;
>> -    e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
>> -
>> -    e1000e_prop_subsys = qdev_prop_uint16;
>> -    e1000e_prop_subsys.description = "PCI device Subsystem ID";
>> -
>>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>>  }
>>  
> 
> Is there a way to keep the description here? At least for "vnet" I believe.

Actually, ability to add description was the only reason for having custom PropertyInfo structs.
Does DEFINE_PROP_… macros set worth an improvement?

> 
> Thanks

Re: [Qemu-devel] [PATCH 2/4] e1000e: No need to validate configuration on migration

[Dmitry Fleytman]

> On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> The user (or management software) is responsible for keeping the
> same configuration on both sides while migrating. Remove the
> configuration validation code at e1000e_post_load, and the
> unnecessary subsys_used/subsys_ven_used fields.

Migration with different subsys/subsys_ven may have a
really cumbersome consequences that are very hard to debug,
so I believe such a verification may save a lot of time,
however it’s a matter of convention.

Jason, is there specific convention for cases like this?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/net/e1000e.c | 14 --------------
> 1 file changed, 14 deletions(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index df24e55..a932620 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -66,9 +66,6 @@ typedef struct E1000EState {
>     uint16_t subsys_ven;
>     uint16_t subsys;
> 
> -    uint16_t subsys_ven_used;
> -    uint16_t subsys_used;
> -
>     bool disable_vnet;
> 
>     E1000ECore core;
> @@ -424,9 +421,6 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
>     pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_ven);
>     pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, s->subsys);
> 
> -    s->subsys_ven_used = s->subsys_ven;
> -    s->subsys_used = s->subsys;
> -
>     /* Define IO/MMIO regions */
>     memory_region_init_io(&s->mmio, OBJECT(s), &mmio_ops, s,
>                           "e1000e-mmio", E1000E_MMIO_SIZE);
> @@ -531,14 +525,6 @@ static int e1000e_post_load(void *opaque, int version_id)
> 
>     trace_e1000e_cb_post_load();
> 
> -    if ((s->subsys != s->subsys_used) ||
> -        (s->subsys_ven != s->subsys_ven_used)) {
> -        fprintf(stderr,
> -            "ERROR: Cannot migrate while device properties "
> -            "(subsys/subsys_ven) differ");
> -        return -1;
> -    }
> -
>     return e1000e_core_post_load(&s->core);
> }
> 
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 4/4] e1000e: Rename "subsys_ven" property to "subsys-vendor"

[Dmitry Fleytman]

> On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> Follow the usual QOM property naming style, and make the property
> name clearer.
> 
> This breaks command-line compatibility, but I am assuming the
> property is not being used in production by anybody.

I’m basically Ok with this patch in case breakage
of command line compatibility is not an issue.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/net/e1000e.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 5a711a7..0b27da3 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -63,7 +63,7 @@ typedef struct E1000EState {
> 
>     uint32_t ioaddr;
> 
> -    uint16_t subsys_ven;
> +    uint16_t subsys_vendor;
>     uint16_t subsys;
> 
>     bool has_vnet;
> @@ -418,7 +418,7 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
>     pci_dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
>     pci_dev->config[PCI_INTERRUPT_PIN] = 1;
> 
> -    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_ven);
> +    pci_set_word(pci_dev->config + PCI_SUBSYSTEM_VENDOR_ID, s->subsys_vendor);
>     pci_set_word(pci_dev->config + PCI_SUBSYSTEM_ID, s->subsys);
> 
>     /* Define IO/MMIO regions */
> @@ -524,7 +524,6 @@ static int e1000e_post_load(void *opaque, int version_id)
>     E1000EState *s = opaque;
> 
>     trace_e1000e_cb_post_load();
> -

This hunk should not be in this patch.

>     return e1000e_core_post_load(&s->core);
> }
> 
> @@ -596,7 +595,7 @@ static const VMStateDescription e1000e_vmstate = {
>         VMSTATE_UINT32(core.delayed_causes, E1000EState),
> 
>         VMSTATE_UINT16(subsys, E1000EState),
> -        VMSTATE_UINT16(subsys_ven, E1000EState),
> +        VMSTATE_UINT16(subsys_vendor, E1000EState),
> 
>         VMSTATE_E1000E_INTR_DELAY_TIMER(core.rdtr, E1000EState),
>         VMSTATE_E1000E_INTR_DELAY_TIMER(core.radv, E1000EState),
> @@ -627,7 +626,7 @@ static const VMStateDescription e1000e_vmstate = {
> static Property e1000e_properties[] = {
>     DEFINE_NIC_PROPERTIES(E1000EState, conf),
>     DEFINE_PROP_BOOL("vnet", E1000EState, has_vnet, true),
> -    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
> +    DEFINE_PROP_UINT16("subsys-vendor", E1000EState, subsys_vendor, PCI_VENDOR_ID_INTEL),
>     DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
>     DEFINE_PROP_END_OF_LIST(),
> };
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 3/4] e1000e: Rename "disable_vnet_hdr" property to "vnet"

[Dmitry Fleytman]

> On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> The original commit that introduced e1000e (6f3fbe4e) mentioned a
> property called "vnet". The actual property name added by the
> patch is "disable_vnet_hdr". Rename the property so that:
> 
> 1) we avoid confusing double-negatives like
>   "disable_vnet_hdr=false";
> 2) we avoid underscores in property names.
> 
> This breaks command-line compatibility, but I am assuming the
> property is not being used in production by anybody.

Reviewed-by: Dmitry Fleytman <dmitry@daynix.com>

Again, this patch is Ok in case command line compatibility is not an issue.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/net/e1000e.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index a932620..5a711a7 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -66,7 +66,7 @@ typedef struct E1000EState {
>     uint16_t subsys_ven;
>     uint16_t subsys;
> 
> -    bool disable_vnet;
> +    bool has_vnet;
> 
>     E1000ECore core;
> 
> @@ -327,7 +327,7 @@ e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, uint8_t *macaddr)
>     qemu_format_nic_info_str(qemu_get_queue(s->nic), macaddr);
> 
>     /* Setup virtio headers */
> -    if (s->disable_vnet) {
> +    if (!s->has_vnet) {
>         s->core.has_vnet = false;
>         trace_e1000e_cfg_support_virtio(false);
>         return;
> @@ -626,7 +626,7 @@ static const VMStateDescription e1000e_vmstate = {
> 
> static Property e1000e_properties[] = {
>     DEFINE_NIC_PROPERTIES(E1000EState, conf),
> -    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
> +    DEFINE_PROP_BOOL("vnet", E1000EState, has_vnet, true),
>     DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
>     DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
>     DEFINE_PROP_END_OF_LIST(),
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> macros for properties

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 09:38:28AM +0300, Dmitry Fleytman wrote:
> 
> > On 27 Oct 2016, at 06:12 AM, Jason Wang <jasowang@redhat.com> wrote:
> > 
> > 
> > 
> > On 2016年10月27日 03:21, Eduardo Habkost wrote:
> >> Instead of hacking custom PropertyInfo structs, use the regular
> >> DEFINE_PROP_<type> macros for the e1000e properties.
> >> 
> >> This also fixes a bug in the disable_vnet_hdr property
> >> definition, that was using qdev_prop_uint8 for a bool field.
> >> 
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >>  hw/net/e1000e.c | 25 +++----------------------
> >>  1 file changed, 3 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> >> index 4994e1c..df24e55 100644
> >> --- a/hw/net/e1000e.c
> >> +++ b/hw/net/e1000e.c
> >> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
> >>      }
> >>  };
> >>  -static PropertyInfo e1000e_prop_disable_vnet,
> >> -                    e1000e_prop_subsys_ven,
> >> -                    e1000e_prop_subsys;
> >> -
> >>  static Property e1000e_properties[] = {
> >>      DEFINE_NIC_PROPERTIES(E1000EState, conf),
> >> -    DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
> >> -                        e1000e_prop_disable_vnet, bool),
> >> -    DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> >> -                        PCI_VENDOR_ID_INTEL,
> >> -                        e1000e_prop_subsys_ven, uint16_t),
> >> -    DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> >> -                        e1000e_prop_subsys, uint16_t),
> >> +    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
> >> +    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
> >> +    DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
> >>      dc->vmsd = &e1000e_vmstate;
> >>      dc->props = e1000e_properties;
> >>  -    e1000e_prop_disable_vnet = qdev_prop_uint8;
> >> -    e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
> >> -                                           "perform SW offloads emulation "
> >> -                                           "instead";
> >> -
> >> -    e1000e_prop_subsys_ven = qdev_prop_uint16;
> >> -    e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
> >> -
> >> -    e1000e_prop_subsys = qdev_prop_uint16;
> >> -    e1000e_prop_subsys.description = "PCI device Subsystem ID";
> >> -
> >>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> >>  }
> >>  
> > 
> > Is there a way to keep the description here? At least for "vnet" I believe.
> 
> Actually, ability to add description was the only reason for having custom PropertyInfo structs.
> Does DEFINE_PROP_… macros set worth an improvement?

I think it would be reasonable to add a descrition field to
struct Property.

Alternatively, you can call object_property_set_description() on
instance_init.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/4] e1000e: Rename "subsys_ven" property to "subsys-vendor"

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 09:45:27AM +0300, Dmitry Fleytman wrote:
[...]
> > @@ -524,7 +524,6 @@ static int e1000e_post_load(void *opaque, int version_id)
> >     E1000EState *s = opaque;
> > 
> >     trace_e1000e_cb_post_load();
> > -
> 
> This hunk should not be in this patch.

Sorry, this is a leftover from conflict resolution when I was
reordering the patches. This can be removed when committing, or I
can send a new version. Jason?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] e1000e: No need to validate configuration on migration

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 09:42:53AM +0300, Dmitry Fleytman wrote:
> 
> > On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > The user (or management software) is responsible for keeping the
> > same configuration on both sides while migrating. Remove the
> > configuration validation code at e1000e_post_load, and the
> > unnecessary subsys_used/subsys_ven_used fields.
> 
> Migration with different subsys/subsys_ven may have a
> really cumbersome consequences that are very hard to debug,
> so I believe such a verification may save a lot of time,
> however it’s a matter of convention.

The same issues apply to almost every other property on every
other device. But the convention is to not expect devices to
validate configuration changes on migration.

(It would be nice to have a more generic mechanism that detects
configuration mismatch on migration, though. Having each device
manually checking each of its properties wouldn't work).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] e1000e: No need to validate configuration on migration

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Thu, Oct 27, 2016 at 09:42:53AM +0300, Dmitry Fleytman wrote:
> > 
> > > On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > The user (or management software) is responsible for keeping the
> > > same configuration on both sides while migrating. Remove the
> > > configuration validation code at e1000e_post_load, and the
> > > unnecessary subsys_used/subsys_ven_used fields.
> > 
> > Migration with different subsys/subsys_ven may have a
> > really cumbersome consequences that are very hard to debug,
> > so I believe such a verification may save a lot of time,
> > however it’s a matter of convention.
> 
> The same issues apply to almost every other property on every
> other device. But the convention is to not expect devices to
> validate configuration changes on migration.
> 
> (It would be nice to have a more generic mechanism that detects
> configuration mismatch on migration, though. Having each device
> manually checking each of its properties wouldn't work).

Without knowing the e1000e code, isn't the easy way to do this
to change the:

        VMSTATE_UINT16(subsys, E1000EState),
        VMSTATE_UINT16(subsys_ven, E1000EState),
to

        VMSTATE_UINT16_EQUAL(subsys, E1000EState),
        VMSTATE_UINT16_EQUAL(subsys_ven, E1000EState),

and the migration code will flag if they don't match for you.

Dave

> 
> -- 
> Eduardo
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/4] e1000e: No need to validate configuration on migration

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 01:22:29PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Thu, Oct 27, 2016 at 09:42:53AM +0300, Dmitry Fleytman wrote:
> > > 
> > > > On 26 Oct 2016, at 22:21 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > The user (or management software) is responsible for keeping the
> > > > same configuration on both sides while migrating. Remove the
> > > > configuration validation code at e1000e_post_load, and the
> > > > unnecessary subsys_used/subsys_ven_used fields.
> > > 
> > > Migration with different subsys/subsys_ven may have a
> > > really cumbersome consequences that are very hard to debug,
> > > so I believe such a verification may save a lot of time,
> > > however it’s a matter of convention.
> > 
> > The same issues apply to almost every other property on every
> > other device. But the convention is to not expect devices to
> > validate configuration changes on migration.
> > 
> > (It would be nice to have a more generic mechanism that detects
> > configuration mismatch on migration, though. Having each device
> > manually checking each of its properties wouldn't work).
> 
> Without knowing the e1000e code, isn't the easy way to do this
> to change the:
> 
>         VMSTATE_UINT16(subsys, E1000EState),
>         VMSTATE_UINT16(subsys_ven, E1000EState),
> to
> 
>         VMSTATE_UINT16_EQUAL(subsys, E1000EState),
>         VMSTATE_UINT16_EQUAL(subsys_ven, E1000EState),
> 
> and the migration code will flag if they don't match for you.

That's very useful. I didn't know about it. I will submit a new
version of the series that uses VMSTATE_UINT16_EQUAL.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> macros for properties

[Igor Mammedov]

On Thu, 27 Oct 2016 09:25:27 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 27, 2016 at 09:38:28AM +0300, Dmitry Fleytman wrote:
> >   
> > > On 27 Oct 2016, at 06:12 AM, Jason Wang <jasowang@redhat.com> wrote:
> > > 
> > > 
> > > 
> > > On 2016年10月27日 03:21, Eduardo Habkost wrote:  
> > >> Instead of hacking custom PropertyInfo structs, use the regular
> > >> DEFINE_PROP_<type> macros for the e1000e properties.
> > >> 
> > >> This also fixes a bug in the disable_vnet_hdr property
> > >> definition, that was using qdev_prop_uint8 for a bool field.
> > >> 
> > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > >> ---
> > >>  hw/net/e1000e.c | 25 +++----------------------
> > >>  1 file changed, 3 insertions(+), 22 deletions(-)
> > >> 
> > >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > >> index 4994e1c..df24e55 100644
> > >> --- a/hw/net/e1000e.c
> > >> +++ b/hw/net/e1000e.c
> > >> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
> > >>      }
> > >>  };
> > >>  -static PropertyInfo e1000e_prop_disable_vnet,
> > >> -                    e1000e_prop_subsys_ven,
> > >> -                    e1000e_prop_subsys;
> > >> -
> > >>  static Property e1000e_properties[] = {
> > >>      DEFINE_NIC_PROPERTIES(E1000EState, conf),
> > >> -    DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
> > >> -                        e1000e_prop_disable_vnet, bool),
> > >> -    DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> > >> -                        PCI_VENDOR_ID_INTEL,
> > >> -                        e1000e_prop_subsys_ven, uint16_t),
> > >> -    DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> > >> -                        e1000e_prop_subsys, uint16_t),
> > >> +    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
> > >> +    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
> > >> +    DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
> > >>      DEFINE_PROP_END_OF_LIST(),
> > >>  };
> > >>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
> > >>      dc->vmsd = &e1000e_vmstate;
> > >>      dc->props = e1000e_properties;
> > >>  -    e1000e_prop_disable_vnet = qdev_prop_uint8;
> > >> -    e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
> > >> -                                           "perform SW offloads emulation "
> > >> -                                           "instead";
> > >> -
> > >> -    e1000e_prop_subsys_ven = qdev_prop_uint16;
> > >> -    e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
> > >> -
> > >> -    e1000e_prop_subsys = qdev_prop_uint16;
> > >> -    e1000e_prop_subsys.description = "PCI device Subsystem ID";
> > >> -
> > >>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> > >>  }
> > >>    
> > > 
> > > Is there a way to keep the description here? At least for "vnet" I believe.  
> > 
> > Actually, ability to add description was the only reason for having custom PropertyInfo structs.
> > Does DEFINE_PROP_… macros set worth an improvement?  
> 
> I think it would be reasonable to add a descrition field to
> struct Property.
> 
> Alternatively, you can call object_property_set_description() on
> instance_init.
that would look sort of convoluted in case of class properties.
description should be set at the same time the property is added.

> 

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> macros for properties

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 05:13:34PM +0200, Igor Mammedov wrote:
> On Thu, 27 Oct 2016 09:25:27 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 27, 2016 at 09:38:28AM +0300, Dmitry Fleytman wrote:
> > >   
> > > > On 27 Oct 2016, at 06:12 AM, Jason Wang <jasowang@redhat.com> wrote:
> > > > 
> > > > 
> > > > 
> > > > On 2016年10月27日 03:21, Eduardo Habkost wrote:  
> > > >> Instead of hacking custom PropertyInfo structs, use the regular
> > > >> DEFINE_PROP_<type> macros for the e1000e properties.
> > > >> 
> > > >> This also fixes a bug in the disable_vnet_hdr property
> > > >> definition, that was using qdev_prop_uint8 for a bool field.
> > > >> 
> > > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > >> ---
> > > >>  hw/net/e1000e.c | 25 +++----------------------
> > > >>  1 file changed, 3 insertions(+), 22 deletions(-)
> > > >> 
> > > >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > > >> index 4994e1c..df24e55 100644
> > > >> --- a/hw/net/e1000e.c
> > > >> +++ b/hw/net/e1000e.c
> > > >> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
> > > >>      }
> > > >>  };
> > > >>  -static PropertyInfo e1000e_prop_disable_vnet,
> > > >> -                    e1000e_prop_subsys_ven,
> > > >> -                    e1000e_prop_subsys;
> > > >> -
> > > >>  static Property e1000e_properties[] = {
> > > >>      DEFINE_NIC_PROPERTIES(E1000EState, conf),
> > > >> -    DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
> > > >> -                        e1000e_prop_disable_vnet, bool),
> > > >> -    DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> > > >> -                        PCI_VENDOR_ID_INTEL,
> > > >> -                        e1000e_prop_subsys_ven, uint16_t),
> > > >> -    DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> > > >> -                        e1000e_prop_subsys, uint16_t),
> > > >> +    DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
> > > >> +    DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, PCI_VENDOR_ID_INTEL),
> > > >> +    DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
> > > >>      DEFINE_PROP_END_OF_LIST(),
> > > >>  };
> > > >>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)
> > > >>      dc->vmsd = &e1000e_vmstate;
> > > >>      dc->props = e1000e_properties;
> > > >>  -    e1000e_prop_disable_vnet = qdev_prop_uint8;
> > > >> -    e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
> > > >> -                                           "perform SW offloads emulation "
> > > >> -                                           "instead";
> > > >> -
> > > >> -    e1000e_prop_subsys_ven = qdev_prop_uint16;
> > > >> -    e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
> > > >> -
> > > >> -    e1000e_prop_subsys = qdev_prop_uint16;
> > > >> -    e1000e_prop_subsys.description = "PCI device Subsystem ID";
> > > >> -
> > > >>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> > > >>  }
> > > >>    
> > > > 
> > > > Is there a way to keep the description here? At least for "vnet" I believe.  
> > > 
> > > Actually, ability to add description was the only reason for having custom PropertyInfo structs.
> > > Does DEFINE_PROP_… macros set worth an improvement?  
> > 
> > I think it would be reasonable to add a descrition field to
> > struct Property.
> > 
> > Alternatively, you can call object_property_set_description() on
> > instance_init.
> that would look sort of convoluted in case of class properties.
> description should be set at the same time the property is added.

In the case of class properties, we have
object_class_property_set_description().

It's easier to redo this series on top of the
qdev-class-properties series, to avoid conflicts. I will do that.

-- 
Eduardo