[Qemu-devel] [PATCHv8 0/3] fw_cfg: qdev-related tidy-ups

[Qemu-devel] [PATCHv8 0/3] fw_cfg: qdev-related tidy-ups

[Mark Cave-Ayland]

As part of some ongoing sun4u work, I need to be able to wire the fw_cfg
IO interface to a separate IO space by instantiating the qdev device instead
of calling fw_cfg_init_io(). This patchset brings FW_CFG_IO in line with
FW_CFG_MEM and tidies up the realize methods accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Depends-on: https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01994.html <20170707213052.13087-1-ehabkost@redhat.com>

v8:
- Rebase onto master
- Drop patches 1 and 2 since they have already been applied
- Drop patch 4 since fw_cfg_unattached_at_realize() isn't required (it was a
  bug in object_resolve_path_type())
- Add comment on the return value of fw_cfg_find()
- Add Reviewed-By from Igor

v7:
- Remove instance_init() function with assert()
- Switch fw_cfg_find() over to use object_resolve_path_type() which removes the
  need for the fw_cfg device to exist at a fixed QOM path
- Switch check for existence of another fw_cfg device over to use the new
  fw_cfg_find()
- Add check for fw_cfg parent at realize time

v6:
- Revert move of FWCfgEntry from fw_cfg.c to fw_cfg.h from v5
- Add Reviewed-by tag from Laszlo for patch 5
- Add Tested-by tags from Laszlo for the series

v5:
- Remove unused FWCfgIoState iobase and dma_iobase fields
- Add Reviewed-By tags from Laszlo
- Update commit message in patch 5 as suggested by Laszlo
- Move FWCfgEntry typedef from fw_cfg.h to typedefs.h with the others

v4:
- Undo accidental typedef change in patch 5 caught in v3 rework

v3:
- Rework patch 1 to use sysbus_add_io() as suggested by Laszlo
- Add Reviewed-By from Laszlo for patch 2
- Fix assert() when instantiating > 1 fw_cfg device (new patch 3)
- Rename fw_cfg_init1() to fw_cfg_common_realize() as part of patch 4

v2:
- Fix the QOM bug in patch 1 as indicated by Laszlo
- Minimise code churn compared to v1


Mark Cave-Ayland (3):
  fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type
    rather than path
  fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers
  fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

 hw/nvram/fw_cfg.c         |   91 +++++++++++++++------------------------------
 include/hw/nvram/fw_cfg.h |   50 +++++++++++++++++++++++++
 include/qemu/typedefs.h   |    1 +
 3 files changed, 82 insertions(+), 60 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Mark Cave-Ayland]

This will enable the fw_cfg device to be placed anywhere within the QOM tree
regardless of its machine location.

Note that we also add a comment to document the behaviour that we return NULL to
indicate failure where either no fw_cfg device or multiple fw_cfg devices are
found.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nvram/fw_cfg.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 99bdbc2..8ef889a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
 
 FWCfgState *fw_cfg_find(void)
 {
-    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
+    /* Returns FWCfgState if only one fw_cfg device type exists. If zero or
+       more than one fw_cfg device are found then NULL is returned as per the
+       object_resolve_path_type() documentation. This behaviour is correct as
+       it ensures that we detect both missing fw_cfg devices and multiple
+       fw_cfg devices which could result in unpredictable behaviour. */
+    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

[Qemu-devel] [PATCHv8 2/3] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Mark Cave-Ayland]

When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
able to wire it up differently, it is much more convenient for the caller to
instantiate the device and have the fw_cfg default files already preloaded
during realize.

Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
fw_cfg_io_realize() functions so it no longer needs to be called manually
when instantiating the device, and also rename it to fw_cfg_common_realize()
which better describes its new purpose.

Since it is now the responsibility of the machine to wire up the fw_cfg device
it is necessary to introduce a object_property_add_child() call into
fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root
machine object as before.

Finally with the previous change to fw_cfg_find() we can now remove the
assert() preventing multiple fw_cfg devices being instantiated and replace
them with a simple call to fw_cfg_find() at realize time instead. This allows
us to remove FW_CFG_NAME and FW_CFG_PATH since they are no longer required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/nvram/fw_cfg.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 8ef889a..6e96b92 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -37,9 +37,6 @@
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
-#define FW_CFG_NAME "fw_cfg"
-#define FW_CFG_PATH "/machine/" FW_CFG_NAME
-
 #define TYPE_FW_CFG     "fw_cfg"
 #define TYPE_FW_CFG_IO  "fw_cfg_io"
 #define TYPE_FW_CFG_MEM "fw_cfg_mem"
@@ -909,17 +906,16 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 
 
 
-static void fw_cfg_init1(DeviceState *dev)
+static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
 {
     FWCfgState *s = FW_CFG(dev);
     MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t version = FW_CFG_VERSION;
 
-    assert(!object_resolve_path(FW_CFG_PATH, NULL));
-
-    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
-
-    qdev_init_nofail(dev);
+    if (!fw_cfg_find()) {
+        error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG);
+        return;
+    }
 
     fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
@@ -952,7 +948,9 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     ios = FW_CFG_IO(dev);
@@ -990,7 +988,9 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
-    fw_cfg_init1(dev);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
@@ -1025,6 +1025,7 @@ FWCfgState *fw_cfg_find(void)
     return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
 }
 
+
 static void fw_cfg_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1096,6 +1097,12 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
                               &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
                               sizeof(dma_addr_t));
     }
+
+    fw_cfg_common_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
@@ -1162,6 +1169,12 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
                               sizeof(dma_addr_t));
         sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
     }
+
+    fw_cfg_common_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 }
 
 static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
-- 
1.7.10.4

[Qemu-devel] [PATCHv8 3/3] fw_cfg: move QOM type defines and fw_cfg types into fw_cfg.h

[Mark Cave-Ayland]

By exposing FWCfgIoState and FWCfgMemState internals we allow the possibility
for the internal MemoryRegion fields to be mapped by name for boards that wish
to wire up the fw_cfg device themselves.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/nvram/fw_cfg.c         |   49 +-------------------------------------------
 include/hw/nvram/fw_cfg.h |   50 +++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/typedefs.h   |    1 +
 3 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 6e96b92..7d348fd 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -37,14 +37,6 @@
 
 #define FW_CFG_FILE_SLOTS_DFLT 0x20
 
-#define TYPE_FW_CFG     "fw_cfg"
-#define TYPE_FW_CFG_IO  "fw_cfg_io"
-#define TYPE_FW_CFG_MEM "fw_cfg_mem"
-
-#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
-#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
-#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
-
 /* FW_CFG_VERSION bits */
 #define FW_CFG_VERSION      0x01
 #define FW_CFG_VERSION_DMA  0x02
@@ -58,51 +50,12 @@
 
 #define FW_CFG_DMA_SIGNATURE 0x51454d5520434647ULL /* "QEMU CFG" */
 
-typedef struct FWCfgEntry {
+struct FWCfgEntry {
     uint32_t len;
     bool allow_write;
     uint8_t *data;
     void *callback_opaque;
     FWCfgReadCallback read_callback;
-} FWCfgEntry;
-
-struct FWCfgState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    uint16_t file_slots;
-    FWCfgEntry *entries[2];
-    int *entry_order;
-    FWCfgFiles *files;
-    uint16_t cur_entry;
-    uint32_t cur_offset;
-    Notifier machine_ready;
-
-    int fw_cfg_order_override;
-
-    bool dma_enabled;
-    dma_addr_t dma_addr;
-    AddressSpace *dma_as;
-    MemoryRegion dma_iomem;
-};
-
-struct FWCfgIoState {
-    /*< private >*/
-    FWCfgState parent_obj;
-    /*< public >*/
-
-    MemoryRegion comb_iomem;
-};
-
-struct FWCfgMemState {
-    /*< private >*/
-    FWCfgState parent_obj;
-    /*< public >*/
-
-    MemoryRegion ctl_iomem, data_iomem;
-    uint32_t data_width;
-    MemoryRegionOps wide_data_ops;
 };
 
 #define JPG_FILE 0
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index b980cba..b77ea48 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -1,8 +1,19 @@
 #ifndef FW_CFG_H
 #define FW_CFG_H
 
+#include "qemu/typedefs.h"
 #include "exec/hwaddr.h"
 #include "hw/nvram/fw_cfg_keys.h"
+#include "hw/sysbus.h"
+#include "sysemu/dma.h"
+
+#define TYPE_FW_CFG     "fw_cfg"
+#define TYPE_FW_CFG_IO  "fw_cfg_io"
+#define TYPE_FW_CFG_MEM "fw_cfg_mem"
+
+#define FW_CFG(obj)     OBJECT_CHECK(FWCfgState,    (obj), TYPE_FW_CFG)
+#define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
+#define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
 typedef struct FWCfgFile {
     uint32_t  size;        /* file size */
@@ -35,6 +46,45 @@ typedef struct FWCfgDmaAccess {
 
 typedef void (*FWCfgReadCallback)(void *opaque);
 
+struct FWCfgState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    uint16_t file_slots;
+    FWCfgEntry *entries[2];
+    int *entry_order;
+    FWCfgFiles *files;
+    uint16_t cur_entry;
+    uint32_t cur_offset;
+    Notifier machine_ready;
+
+    int fw_cfg_order_override;
+
+    bool dma_enabled;
+    dma_addr_t dma_addr;
+    AddressSpace *dma_as;
+    MemoryRegion dma_iomem;
+};
+
+struct FWCfgIoState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion comb_iomem;
+};
+
+struct FWCfgMemState {
+    /*< private >*/
+    FWCfgState parent_obj;
+    /*< public >*/
+
+    MemoryRegion ctl_iomem, data_iomem;
+    uint32_t data_width;
+    MemoryRegionOps wide_data_ops;
+};
+
 /**
  * fw_cfg_add_bytes:
  * @s: fw_cfg device being modified
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 2706aab..0a23020 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -30,6 +30,7 @@ typedef struct DisplaySurface DisplaySurface;
 typedef struct DriveInfo DriveInfo;
 typedef struct Error Error;
 typedef struct EventNotifier EventNotifier;
+typedef struct FWCfgEntry FWCfgEntry;
 typedef struct FWCfgIoState FWCfgIoState;
 typedef struct FWCfgMemState FWCfgMemState;
 typedef struct FWCfgState FWCfgState;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Eduardo Habkost]

On Tue, Jul 11, 2017 at 09:02:11PM +0100, Mark Cave-Ayland wrote:
> This will enable the fw_cfg device to be placed anywhere within the QOM tree
> regardless of its machine location.
> 
> Note that we also add a comment to document the behaviour that we return NULL to
> indicate failure where either no fw_cfg device or multiple fw_cfg devices are
> found.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/nvram/fw_cfg.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 99bdbc2..8ef889a 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
>  
>  FWCfgState *fw_cfg_find(void)
>  {
> -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> +    /* Returns FWCfgState if only one fw_cfg device type exists. If zero or
> +       more than one fw_cfg device are found then NULL is returned as per the
> +       object_resolve_path_type() documentation. This behaviour is correct as
> +       it ensures that we detect both missing fw_cfg devices and multiple
> +       fw_cfg devices which could result in unpredictable behaviour. */
> +    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>  }

I believe a one-line "returns NULL unless there is exactly one
fw_cfg device" (similar to the one at find_vmgenid_dev()) would
suffice.  :)

-- 
Eduardo

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Mark Cave-Ayland]

On 11/07/17 21:10, Eduardo Habkost wrote:

> On Tue, Jul 11, 2017 at 09:02:11PM +0100, Mark Cave-Ayland wrote:
>> This will enable the fw_cfg device to be placed anywhere within the QOM tree
>> regardless of its machine location.
>>
>> Note that we also add a comment to document the behaviour that we return NULL to
>> indicate failure where either no fw_cfg device or multiple fw_cfg devices are
>> found.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/nvram/fw_cfg.c |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 99bdbc2..8ef889a 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
>>  
>>  FWCfgState *fw_cfg_find(void)
>>  {
>> -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
>> +    /* Returns FWCfgState if only one fw_cfg device type exists. If zero or
>> +       more than one fw_cfg device are found then NULL is returned as per the
>> +       object_resolve_path_type() documentation. This behaviour is correct as
>> +       it ensures that we detect both missing fw_cfg devices and multiple
>> +       fw_cfg devices which could result in unpredictable behaviour. */
>> +    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
>>  }
> 
> I believe a one-line "returns NULL unless there is exactly one
> fw_cfg device" (similar to the one at find_vmgenid_dev()) would
> suffice.  :)

I got the impression from the thread that Igor thought the semantics of
fw_cfg_find() weren't clear enough, so wanted to make sure a good
explanation was included in the patch.

I'll wait to see if there's any further feedback before a v9...


ATB,

Mark.

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Michael S. Tsirkin]

On Tue, Jul 11, 2017 at 05:10:59PM -0300, Eduardo Habkost wrote:
> On Tue, Jul 11, 2017 at 09:02:11PM +0100, Mark Cave-Ayland wrote:
> > This will enable the fw_cfg device to be placed anywhere within the QOM tree
> > regardless of its machine location.
> > 
> > Note that we also add a comment to document the behaviour that we return NULL to
> > indicate failure where either no fw_cfg device or multiple fw_cfg devices are
> > found.
> > 
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >  hw/nvram/fw_cfg.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 99bdbc2..8ef889a 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -1017,7 +1017,12 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr)
> >  
> >  FWCfgState *fw_cfg_find(void)
> >  {
> > -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> > +    /* Returns FWCfgState if only one fw_cfg device type exists. If zero or
> > +       more than one fw_cfg device are found then NULL is returned as per the
> > +       object_resolve_path_type() documentation. This behaviour is correct as
> > +       it ensures that we detect both missing fw_cfg devices and multiple
> > +       fw_cfg devices which could result in unpredictable behaviour. */
> > +    return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL));
> >  }
> 
> I believe a one-line "returns NULL unless there is exactly one
> fw_cfg device" (similar to the one at find_vmgenid_dev()) would
> suffice.  :)

Multi-line comments also should formatted

/*
 * like
 * this
 */

not

/* like
 * this */

> -- 
> Eduardo

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Mark Cave-Ayland]

On 11/07/17 21:18, Michael S. Tsirkin wrote:

> Multi-line comments also should formatted
> 
> /*
>  * like
>  * this
>  */
> 
> not
> 
> /* like
>  * this */
> 

Interesting, I never knew there was a preferred format for comments (I
see both styles throughout the codebase).

FWIW it's probably worth pointing out that the style for multi-line
comments isn't given in CODING_STYLE, and checkpatch didn't complain
either...


ATB,

Mark.

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Eduardo Habkost]

On Tue, Jul 11, 2017 at 09:26:22PM +0100, Mark Cave-Ayland wrote:
> On 11/07/17 21:18, Michael S. Tsirkin wrote:
> 
> > Multi-line comments also should formatted
> > 
> > /*
> >  * like
> >  * this
> >  */
> > 
> > not
> > 
> > /* like
> >  * this */
> > 
> 
> Interesting, I never knew there was a preferred format for comments (I
> see both styles throughout the codebase).
> 
> FWIW it's probably worth pointing out that the style for multi-line
> comments isn't given in CODING_STYLE, and checkpatch didn't complain
> either...

I just noticed that, too.  I will submit a patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Peter Maydell]

On 11 July 2017 at 21:26, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 11/07/17 21:18, Michael S. Tsirkin wrote:
>
>> Multi-line comments also should formatted
>>
>> /*
>>  * like
>>  * this
>>  */
>>
>> not
>>
>> /* like
>>  * this */
>>
>
> Interesting, I never knew there was a preferred format for comments (I
> see both styles throughout the codebase).

It's basically GNU coding standard style vs Linux kernel style;
there's a mix because some contributors are more used to working
on the kernel, and some more used to working with gcc, glibc,
etc, and we haven't made a firm "comments must be like this"
statement (and of course historical practice in the codebase
is all over the place).

We also have both of the flavours the kernel style guide
documents:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
(I usually use the style the kernel has for net/ personally.)

So I don't think that "we" the project have a preferred
format; but "we" individual contributors probably
have individual preferences ;-)

thanks
-- PMM

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Eduardo Habkost]

On Tue, Jul 11, 2017 at 09:49:30PM +0100, Peter Maydell wrote:
> On 11 July 2017 at 21:26, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> > On 11/07/17 21:18, Michael S. Tsirkin wrote:
> >
> >> Multi-line comments also should formatted
> >>
> >> /*
> >>  * like
> >>  * this
> >>  */
> >>
> >> not
> >>
> >> /* like
> >>  * this */
> >>
> >
> > Interesting, I never knew there was a preferred format for comments (I
> > see both styles throughout the codebase).
> 
> It's basically GNU coding standard style vs Linux kernel style;
> there's a mix because some contributors are more used to working
> on the kernel, and some more used to working with gcc, glibc,
> etc, and we haven't made a firm "comments must be like this"
> statement (and of course historical practice in the codebase
> is all over the place).
> 
> We also have both of the flavours the kernel style guide
> documents:
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
> (I usually use the style the kernel has for net/ personally.)
> 
> So I don't think that "we" the project have a preferred
> format; but "we" individual contributors probably
> have individual preferences ;-)

Thanks for the info.  Please forget when I said I was going to
send a CODING_STYLE patch for that.  :)

-- 
Eduardo

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Peter Maydell]

On 11 July 2017 at 21:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Jul 11, 2017 at 09:49:30PM +0100, Peter Maydell wrote:
>> It's basically GNU coding standard style vs Linux kernel style;
>> there's a mix because some contributors are more used to working
>> on the kernel, and some more used to working with gcc, glibc,
>> etc, and we haven't made a firm "comments must be like this"
>> statement (and of course historical practice in the codebase
>> is all over the place).
>>
>> We also have both of the flavours the kernel style guide
>> documents:
>> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
>> (I usually use the style the kernel has for net/ personally.)
>>
>> So I don't think that "we" the project have a preferred
>> format; but "we" individual contributors probably
>> have individual preferences ;-)
>
> Thanks for the info.  Please forget when I said I was going to
> send a CODING_STYLE patch for that.  :)

I was just giving the historical context, not necessarily
arguing that we shouldn't try to impose a standard style
now. (I have no strong opinion on that, beyond that style
recommendations backed up by patches to checkpatch win
over those without.)

thanks
-- PMM

Re: [Qemu-devel] [PATCHv8 2/3] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Eduardo Habkost]

On Tue, Jul 11, 2017 at 09:02:12PM +0100, Mark Cave-Ayland wrote:
> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
> able to wire it up differently, it is much more convenient for the caller to
> instantiate the device and have the fw_cfg default files already preloaded
> during realize.
> 
> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> fw_cfg_io_realize() functions so it no longer needs to be called manually
> when instantiating the device, and also rename it to fw_cfg_common_realize()
> which better describes its new purpose.
> 
> Since it is now the responsibility of the machine to wire up the fw_cfg device
> it is necessary to introduce a object_property_add_child() call into
> fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root
> machine object as before.
> 
> Finally with the previous change to fw_cfg_find() we can now remove the
> assert() preventing multiple fw_cfg devices being instantiated and replace
> them with a simple call to fw_cfg_find() at realize time instead. This allows
> us to remove FW_CFG_NAME and FW_CFG_PATH since they are no longer required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
[...]
> @@ -1096,6 +1097,12 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>                                &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
>                                sizeof(dma_addr_t));
>      }
> +
> +    fw_cfg_common_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }

"if (local_err)" is redundant, as error_propagate() does nothing
if local_err is NULL.  "return" is also unnecessary here.  You
also don't need local_err/error_propagate() if you are not
looking at the value of local_err at all.  This means those 5
lines can be simply written as:

    fw_cfg_common_realize(dev, errp);

(Sorry for not noticing that in v7)

>  
>  static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1162,6 +1169,12 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>                                sizeof(dma_addr_t));
>          sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
>      }
> +
> +    fw_cfg_common_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Same as above.


>  }
>  
>  static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
> -- 
> 1.7.10.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCHv8 1/3] fw_cfg: switch fw_cfg_find() to locate the fw_cfg device by type rather than path

[Laszlo Ersek]

(off-topic)

On 07/11/17 22:49, Peter Maydell wrote:
> On 11 July 2017 at 21:26, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> On 11/07/17 21:18, Michael S. Tsirkin wrote:
>>
>>> Multi-line comments also should formatted
>>>
>>> /*
>>>  * like
>>>  * this
>>>  */
>>>
>>> not
>>>
>>> /* like
>>>  * this */
>>>
>>
>> Interesting, I never knew there was a preferred format for comments (I
>> see both styles throughout the codebase).
> 
> It's basically GNU coding standard style vs Linux kernel style;
> there's a mix because some contributors are more used to working
> on the kernel, and some more used to working with gcc, glibc,
> etc, and we haven't made a firm "comments must be like this"
> statement (and of course historical practice in the codebase
> is all over the place).
> 
> We also have both of the flavours the kernel style guide
> documents:
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
> (I usually use the style the kernel has for net/ personally.)
> 
> So I don't think that "we" the project have a preferred
> format; but "we" individual contributors probably
> have individual preferences ;-)

Hey I'm feeling oppressed now; can I add edk2-style comments

//
// like this
//

along with CamelCaseVariables, TYPEDEFS_FOR_STRUCTS, and
CamelCaseEnumConstants? ;)

(Just kidding! :) I should be sleeping now. Sorry.)

Laszlo