[Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[armbru]

From: Markus Armbruster <armbru@redhat.com>

Pointer properties can be set only by code, not by device_add.  A
device with a pointer property can't work with device_add only unless
the property may remain null.  cannot_instantiate_with_device_add_yet
needs to be set then.  PATCH 1/2 sets it when needed and else
documents why not.  PATCH 2/2 documents this for future users of
pointer properties.

This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
series.

Markus Armbruster (2):
  hw: cannot_instantiate_with_device_add_yet due to pointer props
  qdev: Document that pointer properties kill device_add

 hw/audio/marvell_88w8618.c   |  2 ++
 hw/dma/sparc32_dma.c         |  2 ++
 hw/gpio/omap_gpio.c          |  4 ++++
 hw/i2c/omap_i2c.c            |  2 ++
 hw/i2c/smbus_eeprom.c        |  2 ++
 hw/intc/etraxfs_pic.c        |  4 ++++
 hw/intc/grlib_irqmp.c        |  2 ++
 hw/intc/omap_intc.c          |  4 ++++
 hw/net/etraxfs_eth.c         |  2 ++
 hw/net/lance.c               |  2 ++
 include/hw/qdev-properties.h | 17 +++++++++++++++++
 11 files changed, 43 insertions(+)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[armbru]

From: Markus Armbruster <armbru@redhat.com>

Pointer properties can be set only by code, not by device_add.  A
device with a pointer property can work with device_add only when the
property may remain null.

This is the case for property "interrupt_vector" of device
"etraxfs,pic".  Add a comment there.

Set cannot_instantiate_with_device_add_yet for the other devices with
pointer properties, with a comment explaining why.

Juha Riihimäki and Peter Maydell deserve my thanks for making "pointer
property must not remain null" blatantly obvious in the OMAP devices.

Only device "smbus-eeprom" is actually changed.  The others are all
sysbus devices, which get cannot_instantiate_with_device_add_yet set
in their abstract base's class init function.  Setting it again in
their class init function is technically redundant, but serves as
insurance for when sysbus devices become available with device_add,
and as documentation.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/audio/marvell_88w8618.c | 2 ++
 hw/dma/sparc32_dma.c       | 2 ++
 hw/gpio/omap_gpio.c        | 4 ++++
 hw/i2c/omap_i2c.c          | 2 ++
 hw/i2c/smbus_eeprom.c      | 2 ++
 hw/intc/etraxfs_pic.c      | 4 ++++
 hw/intc/grlib_irqmp.c      | 2 ++
 hw/intc/omap_intc.c        | 4 ++++
 hw/net/etraxfs_eth.c       | 2 ++
 hw/net/lance.c             | 2 ++
 10 files changed, 26 insertions(+)

diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
index 97194ce..cdce238 100644
--- a/hw/audio/marvell_88w8618.c
+++ b/hw/audio/marvell_88w8618.c
@@ -288,6 +288,8 @@ static void mv88w8618_audio_class_init(ObjectClass *klass, void *data)
     dc->reset = mv88w8618_audio_reset;
     dc->vmsd = &mv88w8618_audio_vmsd;
     dc->props = mv88w8618_audio_properties;
+    /* Reason: pointer property "wm8750" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo mv88w8618_audio_info = {
diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 2a92ffb..eac338f 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -304,6 +304,8 @@ static void sparc32_dma_class_init(ObjectClass *klass, void *data)
     dc->reset = dma_reset;
     dc->vmsd = &vmstate_dma;
     dc->props = sparc32_dma_properties;
+    /* Reason: pointer property "iommu_opaque" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sparc32_dma_info = {
diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
index b8f572b..938782a 100644
--- a/hw/gpio/omap_gpio.c
+++ b/hw/gpio/omap_gpio.c
@@ -759,6 +759,8 @@ static void omap_gpio_class_init(ObjectClass *klass, void *data)
     k->init = omap_gpio_init;
     dc->reset = omap_gpif_reset;
     dc->props = omap_gpio_properties;
+    /* Reason: pointer property "clk" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo omap_gpio_info = {
@@ -788,6 +790,8 @@ static void omap2_gpio_class_init(ObjectClass *klass, void *data)
     k->init = omap2_gpio_init;
     dc->reset = omap2_gpif_reset;
     dc->props = omap2_gpio_properties;
+    /* Reason: pointer properties "iclk", "fclk0", ..., "fclk5" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo omap2_gpio_info = {
diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
index f528b2b..2d8e2b7 100644
--- a/hw/i2c/omap_i2c.c
+++ b/hw/i2c/omap_i2c.c
@@ -475,6 +475,8 @@ static void omap_i2c_class_init(ObjectClass *klass, void *data)
     k->init = omap_i2c_init;
     dc->props = omap_i2c_properties;
     dc->reset = omap_i2c_reset;
+    /* Reason: pointer properties "iclk", "fclk" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo omap_i2c_info = {
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 0154283..0218f8a 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -121,6 +121,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     sc->write_data = eeprom_write_data;
     sc->read_data = eeprom_read_data;
     dc->props = smbus_eeprom_properties;
+    /* Reason: pointer property "data" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo smbus_eeprom_info = {
diff --git a/hw/intc/etraxfs_pic.c b/hw/intc/etraxfs_pic.c
index e02da53..636262b 100644
--- a/hw/intc/etraxfs_pic.c
+++ b/hw/intc/etraxfs_pic.c
@@ -170,6 +170,10 @@ static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
 
     k->init = etraxfs_pic_init;
     dc->props = etraxfs_pic_properties;
+    /*
+     * Note: pointer property "interrupt_vector" may remain null, thus
+     * no need for dc->cannot_instantiate_with_device_add_yet = true;
+     */
 }
 
 static const TypeInfo etraxfs_pic_info = {
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 42e00bc..d1813f7 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -355,6 +355,8 @@ static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
     k->init = grlib_irqmp_init;
     dc->reset = grlib_irqmp_reset;
     dc->props = grlib_irqmp_properties;
+    /* Reason: pointer properties "set_pil_in", "set_pil_in_opaque" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo grlib_irqmp_info = {
diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
index 7dd63da..ad3931c 100644
--- a/hw/intc/omap_intc.c
+++ b/hw/intc/omap_intc.c
@@ -392,6 +392,8 @@ static void omap_intc_class_init(ObjectClass *klass, void *data)
     k->init = omap_intc_init;
     dc->reset = omap_inth_reset;
     dc->props = omap_intc_properties;
+    /* Reason: pointer property "clk" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo omap_intc_info = {
@@ -637,6 +639,8 @@ static void omap2_intc_class_init(ObjectClass *klass, void *data)
     k->init = omap2_intc_init;
     dc->reset = omap_inth_reset;
     dc->props = omap2_intc_properties;
+    /* Reason: pointer property "iclk", "fclk" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo omap2_intc_info = {
diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
index 78ebbbc..6a3c86d 100644
--- a/hw/net/etraxfs_eth.c
+++ b/hw/net/etraxfs_eth.c
@@ -646,6 +646,8 @@ static void etraxfs_eth_class_init(ObjectClass *klass, void *data)
 
     k->init = fs_eth_init;
     dc->props = etraxfs_eth_properties;
+    /* Reason: pointer properties "dma_out", "dma_in" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo etraxfs_eth_info = {
diff --git a/hw/net/lance.c b/hw/net/lance.c
index e339f02..fe18564 100644
--- a/hw/net/lance.c
+++ b/hw/net/lance.c
@@ -161,6 +161,8 @@ static void lance_class_init(ObjectClass *klass, void *data)
     dc->reset = lance_reset;
     dc->vmsd = &vmstate_lance;
     dc->props = lance_properties;
+    /* Reason: pointer property "dma" */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo lance_info = {
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] qdev: Document that pointer properties kill device_add

[armbru]

From: Markus Armbruster <armbru@redhat.com>

Ask users of DEFINE_PROP_PTR() to set
cannot_instantiate_with_device_add_yet, or explain why it's not
needed.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/qdev-properties.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 692f82e..77c6f7c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -122,8 +122,25 @@ extern PropertyInfo qdev_prop_arraylen;
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
 
+/*
+ * Please avoid pointer properties.  If you must use them, you must
+ * cover them in their device's class init function as follows:
+ *
+ * - If the property must be set, the device cannot be used with
+ *   device_add, so add code like this:
+ *   |* Reason: pointer property "NAME-OF-YOUR-PROP" *|
+ *   DeviceClass *dc = DEVICE_CLASS(class);
+ *   dc->cannot_instantiate_with_device_add_yet = true;
+ *
+ * - If the property may safely remain null, document it like this:
+ *   |*
+ *    * Note: pointer property "interrupt_vector" may remain null, thus
+ *    * no need for dc->cannot_instantiate_with_device_add_yet = true;
+ *    *|
+ */
 #define DEFINE_PROP_PTR(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)
+
 #define DEFINE_PROP_CHR(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_chr, CharDriverState*)
 #define DEFINE_PROP_STRING(_n, _s, _f)             \
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Edgar E. Iglesias]

On Fri, Nov 29, 2013 at 10:43:44AM +0100, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Pointer properties can be set only by code, not by device_add.  A
> device with a pointer property can work with device_add only when the
> property may remain null.
> 
> This is the case for property "interrupt_vector" of device
> "etraxfs,pic".  Add a comment there.
> 
> Set cannot_instantiate_with_device_add_yet for the other devices with
> pointer properties, with a comment explaining why.
> 
> Juha Riihimäki and Peter Maydell deserve my thanks for making "pointer
> property must not remain null" blatantly obvious in the OMAP devices.
> 
> Only device "smbus-eeprom" is actually changed.  The others are all
> sysbus devices, which get cannot_instantiate_with_device_add_yet set
> in their abstract base's class init function.  Setting it again in
> their class init function is technically redundant, but serves as
> insurance for when sysbus devices become available with device_add,
> and as documentation.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Hi,

The ETRAX parts look good, Thanks.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>


> ---
>  hw/audio/marvell_88w8618.c | 2 ++
>  hw/dma/sparc32_dma.c       | 2 ++
>  hw/gpio/omap_gpio.c        | 4 ++++
>  hw/i2c/omap_i2c.c          | 2 ++
>  hw/i2c/smbus_eeprom.c      | 2 ++
>  hw/intc/etraxfs_pic.c      | 4 ++++
>  hw/intc/grlib_irqmp.c      | 2 ++
>  hw/intc/omap_intc.c        | 4 ++++
>  hw/net/etraxfs_eth.c       | 2 ++
>  hw/net/lance.c             | 2 ++
>  10 files changed, 26 insertions(+)
> 
> diff --git a/hw/audio/marvell_88w8618.c b/hw/audio/marvell_88w8618.c
> index 97194ce..cdce238 100644
> --- a/hw/audio/marvell_88w8618.c
> +++ b/hw/audio/marvell_88w8618.c
> @@ -288,6 +288,8 @@ static void mv88w8618_audio_class_init(ObjectClass *klass, void *data)
>      dc->reset = mv88w8618_audio_reset;
>      dc->vmsd = &mv88w8618_audio_vmsd;
>      dc->props = mv88w8618_audio_properties;
> +    /* Reason: pointer property "wm8750" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo mv88w8618_audio_info = {
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index 2a92ffb..eac338f 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -304,6 +304,8 @@ static void sparc32_dma_class_init(ObjectClass *klass, void *data)
>      dc->reset = dma_reset;
>      dc->vmsd = &vmstate_dma;
>      dc->props = sparc32_dma_properties;
> +    /* Reason: pointer property "iommu_opaque" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo sparc32_dma_info = {
> diff --git a/hw/gpio/omap_gpio.c b/hw/gpio/omap_gpio.c
> index b8f572b..938782a 100644
> --- a/hw/gpio/omap_gpio.c
> +++ b/hw/gpio/omap_gpio.c
> @@ -759,6 +759,8 @@ static void omap_gpio_class_init(ObjectClass *klass, void *data)
>      k->init = omap_gpio_init;
>      dc->reset = omap_gpif_reset;
>      dc->props = omap_gpio_properties;
> +    /* Reason: pointer property "clk" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo omap_gpio_info = {
> @@ -788,6 +790,8 @@ static void omap2_gpio_class_init(ObjectClass *klass, void *data)
>      k->init = omap2_gpio_init;
>      dc->reset = omap2_gpif_reset;
>      dc->props = omap2_gpio_properties;
> +    /* Reason: pointer properties "iclk", "fclk0", ..., "fclk5" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo omap2_gpio_info = {
> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
> index f528b2b..2d8e2b7 100644
> --- a/hw/i2c/omap_i2c.c
> +++ b/hw/i2c/omap_i2c.c
> @@ -475,6 +475,8 @@ static void omap_i2c_class_init(ObjectClass *klass, void *data)
>      k->init = omap_i2c_init;
>      dc->props = omap_i2c_properties;
>      dc->reset = omap_i2c_reset;
> +    /* Reason: pointer properties "iclk", "fclk" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo omap_i2c_info = {
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 0154283..0218f8a 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -121,6 +121,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      sc->write_data = eeprom_write_data;
>      sc->read_data = eeprom_read_data;
>      dc->props = smbus_eeprom_properties;
> +    /* Reason: pointer property "data" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo smbus_eeprom_info = {
> diff --git a/hw/intc/etraxfs_pic.c b/hw/intc/etraxfs_pic.c
> index e02da53..636262b 100644
> --- a/hw/intc/etraxfs_pic.c
> +++ b/hw/intc/etraxfs_pic.c
> @@ -170,6 +170,10 @@ static void etraxfs_pic_class_init(ObjectClass *klass, void *data)
>  
>      k->init = etraxfs_pic_init;
>      dc->props = etraxfs_pic_properties;
> +    /*
> +     * Note: pointer property "interrupt_vector" may remain null, thus
> +     * no need for dc->cannot_instantiate_with_device_add_yet = true;
> +     */
>  }
>  
>  static const TypeInfo etraxfs_pic_info = {
> diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
> index 42e00bc..d1813f7 100644
> --- a/hw/intc/grlib_irqmp.c
> +++ b/hw/intc/grlib_irqmp.c
> @@ -355,6 +355,8 @@ static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
>      k->init = grlib_irqmp_init;
>      dc->reset = grlib_irqmp_reset;
>      dc->props = grlib_irqmp_properties;
> +    /* Reason: pointer properties "set_pil_in", "set_pil_in_opaque" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo grlib_irqmp_info = {
> diff --git a/hw/intc/omap_intc.c b/hw/intc/omap_intc.c
> index 7dd63da..ad3931c 100644
> --- a/hw/intc/omap_intc.c
> +++ b/hw/intc/omap_intc.c
> @@ -392,6 +392,8 @@ static void omap_intc_class_init(ObjectClass *klass, void *data)
>      k->init = omap_intc_init;
>      dc->reset = omap_inth_reset;
>      dc->props = omap_intc_properties;
> +    /* Reason: pointer property "clk" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo omap_intc_info = {
> @@ -637,6 +639,8 @@ static void omap2_intc_class_init(ObjectClass *klass, void *data)
>      k->init = omap2_intc_init;
>      dc->reset = omap_inth_reset;
>      dc->props = omap2_intc_properties;
> +    /* Reason: pointer property "iclk", "fclk" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo omap2_intc_info = {
> diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
> index 78ebbbc..6a3c86d 100644
> --- a/hw/net/etraxfs_eth.c
> +++ b/hw/net/etraxfs_eth.c
> @@ -646,6 +646,8 @@ static void etraxfs_eth_class_init(ObjectClass *klass, void *data)
>  
>      k->init = fs_eth_init;
>      dc->props = etraxfs_eth_properties;
> +    /* Reason: pointer properties "dma_out", "dma_in" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo etraxfs_eth_info = {
> diff --git a/hw/net/lance.c b/hw/net/lance.c
> index e339f02..fe18564 100644
> --- a/hw/net/lance.c
> +++ b/hw/net/lance.c
> @@ -161,6 +161,8 @@ static void lance_class_init(ObjectClass *klass, void *data)
>      dc->reset = lance_reset;
>      dc->vmsd = &vmstate_lance;
>      dc->props = lance_properties;
> +    /* Reason: pointer property "dma" */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo lance_info = {
> -- 
> 1.8.1.4
> 

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Marcel Apfelbaum]

On Fri, 2013-11-29 at 10:43 +0100, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Pointer properties can be set only by code, not by device_add.  A
> device with a pointer property can't work with device_add only unless
> the property may remain null.  cannot_instantiate_with_device_add_yet
> needs to be set then.  PATCH 1/2 sets it when needed and else
> documents why not.  PATCH 2/2 documents this for future users of
> pointer properties.
> 
> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
> series.

Even that I am not familiar with this code, I've checked all the changes
and I agree with them.

Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

Anyway, I do have a question:
Why not asserting on qdev_device_add if we have a pointer property?
Instead of checking only cannot_instantiate_with_device_add_yet,
we can go over properties and if we have a pointer property, assert or
return...

Thanks,
Marcel

> 
> Markus Armbruster (2):
>   hw: cannot_instantiate_with_device_add_yet due to pointer props
>   qdev: Document that pointer properties kill device_add
> 
>  hw/audio/marvell_88w8618.c   |  2 ++
>  hw/dma/sparc32_dma.c         |  2 ++
>  hw/gpio/omap_gpio.c          |  4 ++++
>  hw/i2c/omap_i2c.c            |  2 ++
>  hw/i2c/smbus_eeprom.c        |  2 ++
>  hw/intc/etraxfs_pic.c        |  4 ++++
>  hw/intc/grlib_irqmp.c        |  2 ++
>  hw/intc/omap_intc.c          |  4 ++++
>  hw/net/etraxfs_eth.c         |  2 ++
>  hw/net/lance.c               |  2 ++
>  include/hw/qdev-properties.h | 17 +++++++++++++++++
>  11 files changed, 43 insertions(+)
> 

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Andreas Färber]

Am 01.12.2013 14:13, schrieb Marcel Apfelbaum:
> On Fri, 2013-11-29 at 10:43 +0100, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> Pointer properties can be set only by code, not by device_add.  A
>> device with a pointer property can't work with device_add only unless
>> the property may remain null.  cannot_instantiate_with_device_add_yet
>> needs to be set then.  PATCH 1/2 sets it when needed and else
>> documents why not.  PATCH 2/2 documents this for future users of
>> pointer properties.
>>
>> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
>> series.
> 
> Even that I am not familiar with this code, I've checked all the changes
> and I agree with them.
> 
> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> Anyway, I do have a question:
> Why not asserting on qdev_device_add if we have a pointer property?

When we do device_add / device-add, the guest is usually running and we
shouldn't kill a running guest just because the user is trying something
stupid that we can easily prevent. ;)

The alternative BTW is dropping all those pointer properties and
replacing them with link<> properties. Paolo tried that for the OMAP
timers once but I fear that series was never picked up...?

> Instead of checking only cannot_instantiate_with_device_add_yet,
> we can go over properties and if we have a pointer property, assert or
> return...

Raising an error for certain property types may be an option. Although
theoretically the existence of an incompatible property would not
necessarily indicate incompatibility to instantiate the device, in
practice I believe we don't have such excess properties.

Regards,
Andreas

> 
> Thanks,
> Marcel
> 
>>
>> Markus Armbruster (2):
>>   hw: cannot_instantiate_with_device_add_yet due to pointer props
>>   qdev: Document that pointer properties kill device_add
>>
>>  hw/audio/marvell_88w8618.c   |  2 ++
>>  hw/dma/sparc32_dma.c         |  2 ++
>>  hw/gpio/omap_gpio.c          |  4 ++++
>>  hw/i2c/omap_i2c.c            |  2 ++
>>  hw/i2c/smbus_eeprom.c        |  2 ++
>>  hw/intc/etraxfs_pic.c        |  4 ++++
>>  hw/intc/grlib_irqmp.c        |  2 ++
>>  hw/intc/omap_intc.c          |  4 ++++
>>  hw/net/etraxfs_eth.c         |  2 ++
>>  hw/net/lance.c               |  2 ++
>>  include/hw/qdev-properties.h | 17 +++++++++++++++++
>>  11 files changed, 43 insertions(+)
>>
> 
> 
> 


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

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 01.12.2013 14:13, schrieb Marcel Apfelbaum:
>> On Fri, 2013-11-29 at 10:43 +0100, armbru@redhat.com wrote:
>>> From: Markus Armbruster <armbru@redhat.com>
>>>
>>> Pointer properties can be set only by code, not by device_add.  A
>>> device with a pointer property can't work with device_add only unless
>>> the property may remain null.  cannot_instantiate_with_device_add_yet
>>> needs to be set then.  PATCH 1/2 sets it when needed and else
>>> documents why not.  PATCH 2/2 documents this for future users of
>>> pointer properties.
>>>
>>> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
>>> series.
>> 
>> Even that I am not familiar with this code, I've checked all the changes
>> and I agree with them.
>> 
>> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
>> 
>> Anyway, I do have a question:
>> Why not asserting on qdev_device_add if we have a pointer property?

This is a really good thought.  In fact, it occurred to me, too.
However, see "unless the property may remain null" above: there are uses
of pointer properties that do *not* make the device unusable with
device_add.  We even have an example: etraxfs,pic; see PATCH 1/1.  It's
a sysbus device, so it's unavailable anyway.  But there certainly could
be a device with an optional property that does not and should not have
cannot_instantiate_with_device_add_yet set.

> When we do device_add / device-add, the guest is usually running and we
> shouldn't kill a running guest just because the user is trying something
> stupid that we can easily prevent. ;)

You have a point on assert(bad_input), but this would be
assert(programming_error), where the error is "device doesn't have
cannot_instantiate_with_device_add_yet set".  I'm advocating to be
ruthless with programming error asserts.

> The alternative BTW is dropping all those pointer properties and
> replacing them with link<> properties. Paolo tried that for the OMAP
> timers once but I fear that series was never picked up...?

/* FIXME: Remove opaque pointer properties.  */

/* Not a proper property, just for dirty hacks.  TODO Remove it!  */

:)

>> Instead of checking only cannot_instantiate_with_device_add_yet,
>> we can go over properties and if we have a pointer property, assert or
>> return...
>
> Raising an error for certain property types may be an option. Although
> theoretically the existence of an incompatible property would not
> necessarily indicate incompatibility to instantiate the device, in
> practice I believe we don't have such excess properties.

We don't have them now.  I hope we won't permit any new pointer
properties.  If you guys want pointer property imply its owner's
cannot_instantiate_with_device_add_yet, even though it's not generally
necessary, I'm fine with that.

[...]

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Marcel Apfelbaum]

On Sun, 2013-12-01 at 16:14 +0100, Andreas Färber wrote:
> Am 01.12.2013 14:13, schrieb Marcel Apfelbaum:
> > On Fri, 2013-11-29 at 10:43 +0100, armbru@redhat.com wrote:
> >> From: Markus Armbruster <armbru@redhat.com>
> >>
> >> Pointer properties can be set only by code, not by device_add.  A
> >> device with a pointer property can't work with device_add only unless
> >> the property may remain null.  cannot_instantiate_with_device_add_yet
> >> needs to be set then.  PATCH 1/2 sets it when needed and else
> >> documents why not.  PATCH 2/2 documents this for future users of
> >> pointer properties.
> >>
> >> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
> >> series.
> > 
> > Even that I am not familiar with this code, I've checked all the changes
> > and I agree with them.
> > 
> > Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > 
> > Anyway, I do have a question:
> > Why not asserting on qdev_device_add if we have a pointer property?
> 
> When we do device_add / device-add, the guest is usually running and we
> shouldn't kill a running guest just because the user is trying something
> stupid that we can easily prevent. ;)
> 
> The alternative BTW is dropping all those pointer properties and
> replacing them with link<> properties. Paolo tried that for the OMAP
> timers once but I fear that series was never picked up...?
I heard about these link<> properties, can someone point me to their implementation?

> 
> > Instead of checking only cannot_instantiate_with_device_add_yet,
> > we can go over properties and if we have a pointer property, assert or
> > return...
> 
> Raising an error for certain property types may be an option. Although
> theoretically the existence of an incompatible property would not
> necessarily indicate incompatibility to instantiate the device, in
> practice I believe we don't have such excess properties.
If a pointer property does not necessary mean: "Can't be used with device_add",
I am not so sure anymore...

Thanks Andreas!
Marcel

> 
> Regards,
> Andreas
> 
> > 
> > Thanks,
> > Marcel
> > 
> >>
> >> Markus Armbruster (2):
> >>   hw: cannot_instantiate_with_device_add_yet due to pointer props
> >>   qdev: Document that pointer properties kill device_add
> >>
> >>  hw/audio/marvell_88w8618.c   |  2 ++
> >>  hw/dma/sparc32_dma.c         |  2 ++
> >>  hw/gpio/omap_gpio.c          |  4 ++++
> >>  hw/i2c/omap_i2c.c            |  2 ++
> >>  hw/i2c/smbus_eeprom.c        |  2 ++
> >>  hw/intc/etraxfs_pic.c        |  4 ++++
> >>  hw/intc/grlib_irqmp.c        |  2 ++
> >>  hw/intc/omap_intc.c          |  4 ++++
> >>  hw/net/etraxfs_eth.c         |  2 ++
> >>  hw/net/lance.c               |  2 ++
> >>  include/hw/qdev-properties.h | 17 +++++++++++++++++
> >>  11 files changed, 43 insertions(+)
> >>
> > 
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Marcel Apfelbaum]

On Mon, 2013-12-02 at 08:30 +0100, Markus Armbruster wrote:
> Andreas Färber <afaerber@suse.de> writes:
> 
> > Am 01.12.2013 14:13, schrieb Marcel Apfelbaum:
> >> On Fri, 2013-11-29 at 10:43 +0100, armbru@redhat.com wrote:
> >>> From: Markus Armbruster <armbru@redhat.com>
> >>>
> >>> Pointer properties can be set only by code, not by device_add.  A
> >>> device with a pointer property can't work with device_add only unless
> >>> the property may remain null.  cannot_instantiate_with_device_add_yet
> >>> needs to be set then.  PATCH 1/2 sets it when needed and else
> >>> documents why not.  PATCH 2/2 documents this for future users of
> >>> pointer properties.
> >>>
> >>> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
> >>> series.
> >> 
> >> Even that I am not familiar with this code, I've checked all the changes
> >> and I agree with them.
> >> 
> >> Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >> 
> >> Anyway, I do have a question:
> >> Why not asserting on qdev_device_add if we have a pointer property?
> 
> This is a really good thought.  In fact, it occurred to me, too.
> However, see "unless the property may remain null" above: there are uses
> of pointer properties that do *not* make the device unusable with
> device_add.  We even have an example: etraxfs,pic; see PATCH 1/1.  It's
> a sysbus device, so it's unavailable anyway.  But there certainly could
> be a device with an optional property that does not and should not have
> cannot_instantiate_with_device_add_yet set.
> 
> > When we do device_add / device-add, the guest is usually running and we
> > shouldn't kill a running guest just because the user is trying something
> > stupid that we can easily prevent. ;)
> 
> You have a point on assert(bad_input), but this would be
> assert(programming_error), where the error is "device doesn't have
> cannot_instantiate_with_device_add_yet set".  I'm advocating to be
> ruthless with programming error asserts.
> 
> > The alternative BTW is dropping all those pointer properties and
> > replacing them with link<> properties. Paolo tried that for the OMAP
> > timers once but I fear that series was never picked up...?
> 
> /* FIXME: Remove opaque pointer properties.  */
> 
> /* Not a proper property, just for dirty hacks.  TODO Remove it!  */
> 
> :)
> 
> >> Instead of checking only cannot_instantiate_with_device_add_yet,
> >> we can go over properties and if we have a pointer property, assert or
> >> return...
> >
> > Raising an error for certain property types may be an option. Although
> > theoretically the existence of an incompatible property would not
> > necessarily indicate incompatibility to instantiate the device, in
> > practice I believe we don't have such excess properties.
> 
> We don't have them now.  I hope we won't permit any new pointer
> properties.  If you guys want pointer property imply its owner's
> cannot_instantiate_with_device_add_yet, even though it's not generally
> necessary, I'm fine with that.
It was merely a design (and understanding) question, if we prefer to enforce
such things and not rely on future work to comply with rules defined in comments.
Though I am curios what others think about this specific scenario?

Worst case scenario: the coder forgets about it, the reviewers don't catch
this, the initialization code does not ensure the property is set and
the device is added with an "unhealthy" state. But I suppose such a scenario
would be caught early in the development cycle and is not a real issue. 

Markus, thanks for the explanations,
Marcel

> 
> [...]

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Andreas Färber]

Am 02.12.2013 09:52, schrieb Marcel Apfelbaum:
> On Sun, 2013-12-01 at 16:14 +0100, Andreas Färber wrote:
>> The alternative BTW is dropping all those pointer properties and
>> replacing them with link<> properties. Paolo tried that for the OMAP
>> timers once but I fear that series was never picked up...?
> I heard about these link<> properties, can someone point me to their implementation?

Start from object_property_add_link().

link<foo> properties represent a cross-reference to an object of type
foo, which is representated as a pointer in C and as a textual canonical
path representation in QMP. By contrast we model child<foo> properties
as value fields (cf. SoC/MPCore modeling discussions) and they actually
form the canonical paths used by link<> properties.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Andreas Färber]

Peter,

Am 29.11.2013 10:43, schrieb armbru@redhat.com:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Pointer properties can be set only by code, not by device_add.  A
> device with a pointer property can work with device_add only when the
> property may remain null.
> 
> This is the case for property "interrupt_vector" of device
> "etraxfs,pic".  Add a comment there.
> 
> Set cannot_instantiate_with_device_add_yet for the other devices with
> pointer properties, with a comment explaining why.
> 
> Juha Riihimäki and Peter Maydell deserve my thanks for making "pointer
> property must not remain null" blatantly obvious in the OMAP devices.
> 
> Only device "smbus-eeprom" is actually changed.  The others are all
> sysbus devices, which get cannot_instantiate_with_device_add_yet set
> in their abstract base's class init function.  Setting it again in
> their class init function is technically redundant, but serves as
> insurance for when sysbus devices become available with device_add,
> and as documentation.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/audio/marvell_88w8618.c | 2 ++
>  hw/dma/sparc32_dma.c       | 2 ++
>  hw/gpio/omap_gpio.c        | 4 ++++
>  hw/i2c/omap_i2c.c          | 2 ++
>  hw/i2c/smbus_eeprom.c      | 2 ++
>  hw/intc/etraxfs_pic.c      | 4 ++++
>  hw/intc/grlib_irqmp.c      | 2 ++
>  hw/intc/omap_intc.c        | 4 ++++
>  hw/net/etraxfs_eth.c       | 2 ++
>  hw/net/lance.c             | 2 ++
>  10 files changed, 26 insertions(+)

Since you're mentioned by name, should I wait for you to review the
three OMAP parts?

Andreas

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

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Andreas Färber]

Am 29.11.2013 10:43, schrieb armbru@redhat.com:
> From: Markus Armbruster <armbru@redhat.com>
> 
> Pointer properties can be set only by code, not by device_add.  A
> device with a pointer property can't work with device_add only unless
> the property may remain null.  cannot_instantiate_with_device_add_yet
> needs to be set then.  PATCH 1/2 sets it when needed and else
> documents why not.  PATCH 2/2 documents this for future users of
> pointer properties.
> 
> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
> series.
> 
> Markus Armbruster (2):
>   hw: cannot_instantiate_with_device_add_yet due to pointer props
>   qdev: Document that pointer properties kill device_add

Queued both while still waiting for reply from PMM. I somewhat doubt
that anyone will read the documentation you're adding in 2/2, but at
least we can then point them to it.

https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks,
Andreas

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

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Peter Maydell]

On 15 December 2013 20:55, Andreas Färber <afaerber@suse.de> wrote:
> Since you're mentioned by name, should I wait for you to review the
> three OMAP parts?

There's nothing particularly omap-specific in them.

I kind of think this whole thing is backwards anyway:
we should really say "the user can only instantiate
devices via command line or monitor that are specifically
intended to be hot-pluggable", rather than having an
enormous list of devices we flag as not instantiable
by the user. Even if someday we manage to make it technically
possible to instantiate an omap_i2c device (say) from the
command line, it will still be a completely bizarre thing to do
because it's only intended to work as a part of the omap SoC.

Being able to write board models in something other than C
would be nice, but I really think that if we pursue the idea of
being able to do it all on the command line we'll just end up with
a horrifically confusing command line syntax.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Marcel Apfelbaum]

On Sun, 2013-12-15 at 21:51 +0100, Andreas Färber wrote:
> Am 02.12.2013 09:52, schrieb Marcel Apfelbaum:
> > On Sun, 2013-12-01 at 16:14 +0100, Andreas Färber wrote:
> >> The alternative BTW is dropping all those pointer properties and
> >> replacing them with link<> properties. Paolo tried that for the OMAP
> >> timers once but I fear that series was never picked up...?
> > I heard about these link<> properties, can someone point me to their implementation?
> 
> Start from object_property_add_link().
> 
> link<foo> properties represent a cross-reference to an object of type
> foo, which is representated as a pointer in C and as a textual canonical
> path representation in QMP. By contrast we model child<foo> properties
> as value fields (cf. SoC/MPCore modeling discussions) and they actually
> form the canonical paths used by link<> properties.
Thanks Andreas!
Marcel


> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 15 December 2013 20:55, Andreas Färber <afaerber@suse.de> wrote:
>> Since you're mentioned by name, should I wait for you to review the
>> three OMAP parts?
>
> There's nothing particularly omap-specific in them.

The only OMAP-specific issue I can think of would be me misreading the
intent of the code.

> I kind of think this whole thing is backwards anyway:
> we should really say "the user can only instantiate
> devices via command line or monitor that are specifically
> intended to be hot-pluggable", rather than having an
> enormous list of devices we flag as not instantiable
> by the user. Even if someday we manage to make it technically
> possible to instantiate an omap_i2c device (say) from the
> command line, it will still be a completely bizarre thing to do
> because it's only intended to work as a part of the omap SoC.

"Hot-pluggable" doesn't apply here.  There are plenty of devices that
can only be cold-plugged, yet are absolutely meant to be user-pluggable.
Real ISA cards, for instance.

I share your doubts on the wisdom of letting users plug components via
command line or monitor that are really just parts of bigger components,
like the OMAP SoC.

However, the current code lets users plug absolutely everything, even
stuff that is known not to work.  The code still has the remnants of a
mechanism meant to protect users from known-not-to-work plugs, but it
got broken some time ago.  My "Clean up and fix no_user" series fixes
that regression in a way that's hopefully agreeable with Anthony, who
has been quite insistent on letting device_add plug more rather than
less.  This series merely patches some holes on top.

The list of non-pluggable devices may be larger than the list of
pluggable ones, but: I count just 48 instances of
"cannot_instantiate_with_device_add_yet = true".  I doubt marking
devices that can be plugged instead of the ones than can't be would take
fewer marks.  Moreover, each one comes with a comment explaining *why*
the device cannot be plugged.  Sure nice to have when such a "why" goes
away.  Some of them are expected to go away eventually.

> Being able to write board models in something other than C
> would be nice, but I really think that if we pursue the idea of
> being able to do it all on the command line we'll just end up with
> a horrifically confusing command line syntax.

Command line / monitor could be divorced from hardware configuration.
Doesn't make much sense with the rather primitive device_add we got now.
May well make sense when our means to wire up hardware by configuration
rather than code have moved closer to the complex wirings we need to
model.

Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 29.11.2013 10:43, schrieb armbru@redhat.com:
>> From: Markus Armbruster <armbru@redhat.com>
>> 
>> Pointer properties can be set only by code, not by device_add.  A
>> device with a pointer property can't work with device_add only unless
>> the property may remain null.  cannot_instantiate_with_device_add_yet
>> needs to be set then.  PATCH 1/2 sets it when needed and else
>> documents why not.  PATCH 2/2 documents this for future users of
>> pointer properties.
>> 
>> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user"
>> series.
>> 
>> Markus Armbruster (2):
>>   hw: cannot_instantiate_with_device_add_yet due to pointer props
>>   qdev: Document that pointer properties kill device_add
>
> Queued both while still waiting for reply from PMM. I somewhat doubt
> that anyone will read the documentation you're adding in 2/2, but at
> least we can then point them to it.

Yup.  I wash my hands before the multitude ;)

> https://github.com/afaerber/qemu-cpu/commits/qom-next

Thanks!

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Peter Maydell]

On 16 December 2013 08:48, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> I kind of think this whole thing is backwards anyway:
>> we should really say "the user can only instantiate
>> devices via command line or monitor that are specifically
>> intended to be hot-pluggable", rather than having an
>> enormous list of devices we flag as not instantiable
>> by the user. Even if someday we manage to make it technically
>> possible to instantiate an omap_i2c device (say) from the
>> command line, it will still be a completely bizarre thing to do
>> because it's only intended to work as a part of the omap SoC.
>
> "Hot-pluggable" doesn't apply here.  There are plenty of devices that
> can only be cold-plugged, yet are absolutely meant to be user-pluggable.
> Real ISA cards, for instance.

Mmm. Just plain "pluggable" would be more what I meant:
modelling something that on real hardware is really a
simple pluggable socket.

> However, the current code lets users plug absolutely everything, even
> stuff that is known not to work.  The code still has the remnants of a
> mechanism meant to protect users from known-not-to-work plugs, but it
> got broken some time ago.  My "Clean up and fix no_user" series fixes
> that regression in a way that's hopefully agreeable with Anthony, who
> has been quite insistent on letting device_add plug more rather than
> less.  This series merely patches some holes on top.
>
> The list of non-pluggable devices may be larger than the list of
> pluggable ones, but: I count just 48 instances of
> "cannot_instantiate_with_device_add_yet = true".  I doubt marking
> devices that can be plugged instead of the ones than can't be would take
> fewer marks.  Moreover, each one comes with a comment explaining *why*
> the device cannot be plugged.  Sure nice to have when such a "why" goes
> away.  Some of them are expected to go away eventually.

I would expect 99% of actually pluggable devices to be pluggable
because they're using a pluggable bus: ISA, PCI, USB, ...

Anyway, I don't actively object to this series. I just think
Anthony's going in the wrong direction which is why I haven't
been particularly eager to actively mark it as reviewed-by me
either...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 16 December 2013 08:48, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I kind of think this whole thing is backwards anyway:
>>> we should really say "the user can only instantiate
>>> devices via command line or monitor that are specifically
>>> intended to be hot-pluggable", rather than having an
>>> enormous list of devices we flag as not instantiable
>>> by the user. Even if someday we manage to make it technically
>>> possible to instantiate an omap_i2c device (say) from the
>>> command line, it will still be a completely bizarre thing to do
>>> because it's only intended to work as a part of the omap SoC.
>>
>> "Hot-pluggable" doesn't apply here.  There are plenty of devices that
>> can only be cold-plugged, yet are absolutely meant to be user-pluggable.
>> Real ISA cards, for instance.
>
> Mmm. Just plain "pluggable" would be more what I meant:
> modelling something that on real hardware is really a
> simple pluggable socket.

That makes sense to me.

>> However, the current code lets users plug absolutely everything, even
>> stuff that is known not to work.  The code still has the remnants of a
>> mechanism meant to protect users from known-not-to-work plugs, but it
>> got broken some time ago.  My "Clean up and fix no_user" series fixes
>> that regression in a way that's hopefully agreeable with Anthony, who
>> has been quite insistent on letting device_add plug more rather than
>> less.  This series merely patches some holes on top.
>>
>> The list of non-pluggable devices may be larger than the list of
>> pluggable ones, but: I count just 48 instances of
>> "cannot_instantiate_with_device_add_yet = true".  I doubt marking
>> devices that can be plugged instead of the ones than can't be would take
>> fewer marks.  Moreover, each one comes with a comment explaining *why*
>> the device cannot be plugged.  Sure nice to have when such a "why" goes
>> away.  Some of them are expected to go away eventually.
>
> I would expect 99% of actually pluggable devices to be pluggable
> because they're using a pluggable bus: ISA, PCI, USB, ...
>
> Anyway, I don't actively object to this series. I just think
> Anthony's going in the wrong direction which is why I haven't
> been particularly eager to actively mark it as reviewed-by me
> either...

Understandable :)

Thanks!

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Andreas Färber]

Am 16.12.2013 10:33, schrieb Peter Maydell:
> Anyway, I don't actively object to this series. I just think
> Anthony's going in the wrong direction which is why I haven't
> been particularly eager to actively mark it as reviewed-by me
> either...

Sorry for not taking the time to reply to these concerns earlier. I
thought it was self-speaking that the enterprise Linux distributors
among us want a safeguard to avoid customers from crashing a
long-running VM with some avoidable device_add.

I can't say that I am thrilled about the lengthy name, but this
refactoring has raised awareness of what no_user is supposed to be used
for and where not. As a reminder, Anthony didn't want the direct patch
to simply honor no_user in device_add again, an apparent regression, and
this appears to address his request to Markus' and my understanding.

We can still rename the field again, split or complement its use,
refactor devices and, e.g., CPU/SysBus/timer device infrastructure, etc.
all as follow-ups.

I might have a bad reputation of being strict in my patch review, but
requiring a patch to be the ultimate, final-set-in-stone solution has
not been one of them, if it does not affect users. :) Apart from the
intended regression fix, the choices discussed affect developers only.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Peter Maydell]

On 7 January 2014 12:33, Andreas Färber <afaerber@suse.de> wrote:
> Am 16.12.2013 10:33, schrieb Peter Maydell:
>> Anyway, I don't actively object to this series. I just think
>> Anthony's going in the wrong direction which is why I haven't
>> been particularly eager to actively mark it as reviewed-by me
>> either...
>
> Sorry for not taking the time to reply to these concerns earlier. I
> thought it was self-speaking that the enterprise Linux distributors
> among us want a safeguard to avoid customers from crashing a
> long-running VM with some avoidable device_add.

Sure. I think the right way to do that is to only allow
them to plug in devices that are truly pluggable (ie which
are on some pluggable bus like PCI or USB), rather than
this way round, which is trying to blacklist devices rather
than whitelist bus types.

In short, we shouldn't be trying to cram all of "hotplug",
"I want an extra PCI card in my VM" and "I want to do
complete from-scratch construction of a machine model
including wiring up all the interrupts and defining the
memory map" into the same interface, because the flexibility
you need for the last one of these is going to cause endless
user errors when attempting the first two.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Andreas Färber]

Am 07.01.2014 13:43, schrieb Peter Maydell:
> On 7 January 2014 12:33, Andreas Färber <afaerber@suse.de> wrote:
>> Am 16.12.2013 10:33, schrieb Peter Maydell:
>>> Anyway, I don't actively object to this series. I just think
>>> Anthony's going in the wrong direction which is why I haven't
>>> been particularly eager to actively mark it as reviewed-by me
>>> either...
>>
>> Sorry for not taking the time to reply to these concerns earlier. I
>> thought it was self-speaking that the enterprise Linux distributors
>> among us want a safeguard to avoid customers from crashing a
>> long-running VM with some avoidable device_add.
> 
> Sure. I think the right way to do that is to only allow
> them to plug in devices that are truly pluggable (ie which
> are on some pluggable bus like PCI or USB), rather than
> this way round, which is trying to blacklist devices rather
> than whitelist bus types.
> 
> In short, we shouldn't be trying to cram all of "hotplug",
> "I want an extra PCI card in my VM" and "I want to do
> complete from-scratch construction of a machine model
> including wiring up all the interrupts and defining the
> memory map" into the same interface, because the flexibility
> you need for the last one of these is going to cause endless
> user errors when attempting the first two.

Agreed that there may be better solutions. But in qemu.git we do have a
lengthy, inconsistent blacklist, which is only partially honored. Markus
refactored the blacklist to be less inconsistent, less lengthy.

In particular I like that the previous/base series makes it clear not to
mark individual PHBs as no_user, something that has come up in my PReP
review. Your ARM device patches have also benefited, I believe.

Like I said, this doesn't rule out switching to a whitelist later. His
patchset has been on the list for quite a while and no one has actually
submitted code for a different solution, yourself included. So if I get
to choose between an acceptable sparrow on-list and the pigeon on the
roof ... ;)

That said, I have been sprouting the idea of, e.g., QOM'ifying qemu_irq
in-place (rather than waiting for Pin concept), which would tackle half
of the SysBus problem. QOM'ifying MemoryRegions would be the other half.
Volunteers welcome, I am still not done with QOM realize and CPUState.

Andreas

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

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Peter Crosthwaite]

On Tue, Jan 7, 2014 at 10:43 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 January 2014 12:33, Andreas Färber <afaerber@suse.de> wrote:
>> Am 16.12.2013 10:33, schrieb Peter Maydell:
>>> Anyway, I don't actively object to this series. I just think
>>> Anthony's going in the wrong direction which is why I haven't
>>> been particularly eager to actively mark it as reviewed-by me
>>> either...
>>
>> Sorry for not taking the time to reply to these concerns earlier. I
>> thought it was self-speaking that the enterprise Linux distributors
>> among us want a safeguard to avoid customers from crashing a
>> long-running VM with some avoidable device_add.
>
> Sure. I think the right way to do that is to only allow
> them to plug in devices that are truly pluggable (ie which
> are on some pluggable bus like PCI or USB), rather than
> this way round, which is trying to blacklist devices rather
> than whitelist bus types.
>

If you bring FPGAs into the game, SYSBUS itself is ultimately
pluggable. All sysbus devices under the sun are therefore legitimately
"pluggable" in target-microblaze.

Regards,
Peter


> In short, we shouldn't be trying to cram all of "hotplug",
> "I want an extra PCI card in my VM" and "I want to do
> complete from-scratch construction of a machine model
> including wiring up all the interrupts and defining the
> memory map" into the same interface, because the flexibility
> you need for the last one of these is going to cause endless
> user errors when attempting the first two.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2014 12:33, Andreas Färber <afaerber@suse.de> wrote:
>> Am 16.12.2013 10:33, schrieb Peter Maydell:
>>> Anyway, I don't actively object to this series. I just think
>>> Anthony's going in the wrong direction which is why I haven't
>>> been particularly eager to actively mark it as reviewed-by me
>>> either...
>>
>> Sorry for not taking the time to reply to these concerns earlier. I
>> thought it was self-speaking that the enterprise Linux distributors
>> among us want a safeguard to avoid customers from crashing a
>> long-running VM with some avoidable device_add.
>
> Sure. I think the right way to do that is to only allow
> them to plug in devices that are truly pluggable (ie which
> are on some pluggable bus like PCI or USB), rather than
> this way round, which is trying to blacklist devices rather
> than whitelist bus types.
>
> In short, we shouldn't be trying to cram all of "hotplug",
> "I want an extra PCI card in my VM" and "I want to do
> complete from-scratch construction of a machine model
> including wiring up all the interrupts and defining the
> memory map" into the same interface, because the flexibility
> you need for the last one of these is going to cause endless
> user errors when attempting the first two.

Your point is that having one tool (device_add) for two rather different
purposes ("wire components into a machine" and "plug stuff into sockets
meant for that") is problematic.  Makes sense to me.

Now let's imagine we had a separate tool for each purpose.  Let's call
them "wire up" and "plug".  Let's further imagine QOM/qdev and the
device are basically still in their current state.  Then you need a
separate predicate "tool is applicable to qdev" for each of the two
tools!

The predicate for "plug" could perhaps be "is this a usable plug/socket
pair", as you suggest.

The predicate for "wire up" will be pretty much exactly what
cannot_instantiate_with_device_add_yet is now: a long list of devices
that can only be wired up by special-purpose code, mostly due to
limitations of the "wire up" tool's general-purpose code, and the
QOM/qdev infrastructure below it.  As these limitations are addressed,
the list should shrink.  Anthony wants it to shrink to nothing
eventually.

In short, I doubt we're disagreeing on anything substantial.

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Paolo Bonzini]

Il 07/01/2014 13:43, Peter Maydell ha scritto:
> Sure. I think the right way to do that is to only allow
> them to plug in devices that are truly pluggable (ie which
> are on some pluggable bus like PCI or USB), rather than
> this way round, which is trying to blacklist devices rather
> than whitelist bus types.

Pluggable buses include ISA, PCI, USB, SCSI, SPI, I2C, pSeries vio, i.e.
basically all buses that QEMU supports except SysBus.

Paolo

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Andreas Färber]

Peter,

Am 07.01.2014 14:05, schrieb Peter Crosthwaite:
> If you bring FPGAs into the game, SYSBUS itself is ultimately
> pluggable. All sysbus devices under the sun are therefore legitimately
> "pluggable" in target-microblaze.

You'll have to expand on this statement. :) From what I understand from
documentation for Zynq based boards, the SoC-integrated FPGA is
initialized with a big binary(?) blob from U-Boot firmware. You then
need to supply a flat device tree to Linux, specifying what devices are
exposed by SoC/FPGA, and from then on there is no hot-plug of such
platform devices any more. So unless Xilinx is capable of parsing their
FPGA logic blob back into individual QEMU devices, it seems to me you
are talking about an initial set of devices rather than device_add'ing
devices after Linux is running. To cope with that requirement, devices
should be made instantiatable from command line or config file (I think
we all agree on that goal), but it does not make SysBus hotpluggable.

Or am I missing something? Is it possible to alter FPGA configuration
from inside arm Linux and, if so, how does Linux cope with that?

In the case of the existing MicroBlaze machine(s), I assume that it
represents an FPGA board that has been pre-programmed with the softcore
processor code. So reprogramming the FPGA at runtime would mean changing
the processor that is executing the code changing the processor...
sounds dangerous on hardware and thus hopefully not something we need to
worry about for now, do we?

Regards,
Andreas

P.S. Note that I had been arguing that if a SysBusDevice does not have a
mappable MMIO region or IRQs then we wouldn't need to set
cannot_instantiate_... The pending series does it for all SysBusDevices
though, so if you do have such exceptional devices, we can override
dc->cannot_instantiate_... in class_init.

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

Re: [Qemu-devel] [PATCH 1/2] hw: cannot_instantiate_with_device_add_yet due to pointer props

[Peter Crosthwaite]

Hi Andreas,

On Fri, Jan 10, 2014 at 7:10 PM, Andreas Färber <afaerber@suse.de> wrote:
> Peter,
>
> Am 07.01.2014 14:05, schrieb Peter Crosthwaite:
>> If you bring FPGAs into the game, SYSBUS itself is ultimately
>> pluggable. All sysbus devices under the sun are therefore legitimately
>> "pluggable" in target-microblaze.
>
> You'll have to expand on this statement. :) From what I understand from
> documentation for Zynq based boards, the SoC-integrated FPGA is
> initialized with a big binary(?)

Yes a "bitstream" for short.

 > blob from U-Boot firmware.

So u-boot isn't firmware. u-boot (if you use it) is stored on off-chip
and loaded by earlier boot stages.

> You then
> need to supply a flat device tree to Linux, specifying what devices are
> exposed by SoC/FPGA, and from then on there is no hot-plug of such
> platform devices any more.

That's only a limitation of current-gen Linux. I'm not sure what the
state of the kernel WRT to devcfg support is TBH. Michal may know
more.

> So unless Xilinx is capable of parsing their
> FPGA logic blob back into individual QEMU devices,

No not possible , the bitstream is not self-identifying.

 it seems to me you
> are talking about an initial set of devices rather than device_add'ing
> devices after Linux is running.

But it is possible with some driver patches. The devcfg peripheral
(the FPGA bitstream loader) is just another peripheral in the ZYNQ
SoC. Any guest can program it at anytime. If we just focus on the
hardware capability, zynq is very hotpluggable.

> To cope with that requirement, devices
> should be made instantiatable from command line or config file (I think
> we all agree on that goal), but it does not make SysBus hotpluggable.
>
> Or am I missing something? Is it possible to alter FPGA configuration
> from inside arm Linux and, if so, how does Linux cope with that?
>
> In the case of the existing MicroBlaze machine(s), I assume that it
> represents an FPGA board that has been pre-programmed with the softcore
> processor code.

So I think earlier in this thread we started making the distinction
between "pluggable" and "hot pluggable". Zynq, based on the guests
ability to add peripherals straight onto the system bus is definitely
hot-pugging.

But even with MB, the ability to easily come up with a new microblaze
designs with extra stuff on the system bus to me satisfies some good
definitions of "pluggable". It's advantageous to therefore be able to
dynamically construct MB machines rather than the embedded status quo
of fixed machine models. TBH we've been doing it for a long time now
out of tree with the -M fdt_generic machine models (constructs the
full machine from the -dtb argument).

I would generally assume any such device-adding mechanisms whether
they be device-tree or command line or whatever to back onto the
pluggability frameworks we are talking about here.

> So reprogramming the FPGA at runtime would mean changing
> the processor that is executing the code changing the processor...
> sounds dangerous on hardware and thus hopefully not something we need to
> worry about for now, do we?
>

It is theoretically possible as FPGAs can be partially reconfigured.
That said, its neither well used or supported and its not something I
can see myself worrying about anytime soon.

> Regards,
> Andreas
>
> P.S. Note that I had been arguing that if a SysBusDevice does not have a
> mappable MMIO region or IRQs then we wouldn't need to set
> cannot_instantiate_... The pending series does it for all SysBusDevices
> though, so if you do have such exceptional devices, we can override
> dc->cannot_instantiate_... in class_init.
>

It's hard to identify all such exceptions. For example, Xilinx has a
soft EHCI solution that would be nice to emulate with just a

-device sysbus-ehci,interrupt-parent=/path/to/intc,bus=/path/to/bus,address=0xf00,...

There is advantage to making it work for all sysbus or at least MB
peripherals and standard HCI peripherals.

Regards,
Peter

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