[Qemu-devel] [PATCH 0/4] Introduce the hw/firmware/ namespace

[Qemu-devel] [PATCH 0/4] Introduce the hw/firmware/ namespace

[Philippe Mathieu-Daudé]

QEMU interacts with various firmwares. We already have some helpers
for some firmwares. Later we will add UEFI helpers.
This series introduce the hw/firmware namespace for this.

There is no need to move hw/smbios sources into a specific directory,
we only focus on the headers.

Philippe Mathieu-Daudé (4):
  tests: Remove unused include
  hw/smbios: Restrict access to "smbios_ipmi.h"
  hw/smbios: Remove "smbios_ipmi.h"
  hw/smbios: Move to the hw/firmware/ namespace

 MAINTAINERS                              |  2 +-
 hw/arm/virt.c                            |  2 +-
 hw/i386/pc.c                             |  2 +-
 hw/i386/pc_piix.c                        |  2 +-
 hw/i386/pc_q35.c                         |  2 +-
 hw/smbios/smbios-stub.c                  |  2 +-
 hw/smbios/smbios.c                       |  3 +--
 hw/smbios/smbios_build.h                 |  4 ++++
 hw/smbios/smbios_type_38-stub.c          |  2 +-
 hw/smbios/smbios_type_38.c               |  3 +--
 include/hw/{smbios => firmware}/smbios.h |  0
 include/hw/smbios/ipmi.h                 | 15 ---------------
 tests/acpi-utils.c                       |  1 -
 tests/bios-tables-test.c                 |  2 +-
 vl.c                                     |  2 +-
 15 files changed, 15 insertions(+), 29 deletions(-)
 rename include/hw/{smbios => firmware}/smbios.h (100%)
 delete mode 100644 include/hw/smbios/ipmi.h

-- 
2.17.2

[Qemu-devel] [PATCH 1/4] tests: Remove unused include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/acpi-utils.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
index 41dc1ea9b4..297af55d39 100644
--- a/tests/acpi-utils.c
+++ b/tests/acpi-utils.c
@@ -15,7 +15,6 @@
 #include "qemu/osdep.h"
 #include <glib/gstdio.h>
 #include "qemu-common.h"
-#include "hw/smbios/smbios.h"
 #include "qemu/bitmap.h"
 #include "acpi-utils.h"
 #include "boot-sector.h"
-- 
2.17.2

[Qemu-devel] [PATCH 2/4] hw/smbios: Restrict access to "smbios_ipmi.h"

[Philippe Mathieu-Daudé]

All the consumers of "smbios_ipmi.h" are located in hw/smbios/.
There is no need to have this include publicly exposed,
reduce the visibility by moving it in hw/smbios/.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/smbios/smbios.c                                  | 2 +-
 include/hw/smbios/ipmi.h => hw/smbios/smbios_ipmi.h | 0
 hw/smbios/smbios_type_38-stub.c                     | 2 +-
 hw/smbios/smbios_type_38.c                          | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename include/hw/smbios/ipmi.h => hw/smbios/smbios_ipmi.h (100%)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 920939454e..30bd4731cf 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -28,7 +28,7 @@
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
 #include "smbios_build.h"
-#include "hw/smbios/ipmi.h"
+#include "smbios_ipmi.h"
 
 /* legacy structures and constants for <= 2.0 machines */
 struct smbios_header {
diff --git a/include/hw/smbios/ipmi.h b/hw/smbios/smbios_ipmi.h
similarity index 100%
rename from include/hw/smbios/ipmi.h
rename to hw/smbios/smbios_ipmi.h
diff --git a/hw/smbios/smbios_type_38-stub.c b/hw/smbios/smbios_type_38-stub.c
index 5b83c9b1f1..fc4516bc8a 100644
--- a/hw/smbios/smbios_type_38-stub.c
+++ b/hw/smbios/smbios_type_38-stub.c
@@ -8,7 +8,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/smbios/ipmi.h"
+#include "smbios_ipmi.h"
 
 void smbios_build_type_38_table(void)
 {
diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
index 56e8609c00..d84e87d608 100644
--- a/hw/smbios/smbios_type_38.c
+++ b/hw/smbios/smbios_type_38.c
@@ -9,10 +9,10 @@
 
 #include "qemu/osdep.h"
 #include "hw/ipmi/ipmi.h"
-#include "hw/smbios/ipmi.h"
 #include "hw/smbios/smbios.h"
 #include "qemu/error-report.h"
 #include "smbios_build.h"
+#include "smbios_ipmi.h"
 
 /* SMBIOS type 38 - IPMI */
 struct smbios_type_38 {
-- 
2.17.2

[Qemu-devel] [PATCH 3/4] hw/smbios: Remove "smbios_ipmi.h"

[Philippe Mathieu-Daudé]

This header only declare a single function: smbios_build_type_38_table().
We already have a header that declares such functions: "smbios_build.h".
Move the declaration and remove the header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/smbios/smbios.c              |  1 -
 hw/smbios/smbios_build.h        |  4 ++++
 hw/smbios/smbios_ipmi.h         | 15 ---------------
 hw/smbios/smbios_type_38-stub.c |  2 +-
 hw/smbios/smbios_type_38.c      |  1 -
 5 files changed, 5 insertions(+), 18 deletions(-)
 delete mode 100644 hw/smbios/smbios_ipmi.h

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 30bd4731cf..9d737642cb 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -28,7 +28,6 @@
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
 #include "smbios_build.h"
-#include "smbios_ipmi.h"
 
 /* legacy structures and constants for <= 2.0 machines */
 struct smbios_header {
diff --git a/hw/smbios/smbios_build.h b/hw/smbios/smbios_build.h
index 93b360d520..56b5a1e3f3 100644
--- a/hw/smbios/smbios_build.h
+++ b/hw/smbios/smbios_build.h
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
  * Copyright (C) 2013 Red Hat, Inc.
+ * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
  *
  * Authors:
  *  Alex Williamson <alex.williamson@hp.com>
@@ -96,4 +97,7 @@ extern unsigned smbios_table_cnt;
         smbios_table_cnt++;                                               \
     } while (0)
 
+/* IPMI SMBIOS firmware handling */
+void smbios_build_type_38_table(void);
+
 #endif /* QEMU_SMBIOS_BUILD_H */
diff --git a/hw/smbios/smbios_ipmi.h b/hw/smbios/smbios_ipmi.h
deleted file mode 100644
index 1c9aae38f2..0000000000
--- a/hw/smbios/smbios_ipmi.h
+++ /dev/null
@@ -1,15 +0,0 @@
-/*
- * IPMI SMBIOS firmware handling
- *
- * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#ifndef QEMU_SMBIOS_IPMI_H
-#define QEMU_SMBIOS_IPMI_H
-
-void smbios_build_type_38_table(void);
-
-#endif /* QEMU_SMBIOS_IPMI_H */
diff --git a/hw/smbios/smbios_type_38-stub.c b/hw/smbios/smbios_type_38-stub.c
index fc4516bc8a..14b53d004b 100644
--- a/hw/smbios/smbios_type_38-stub.c
+++ b/hw/smbios/smbios_type_38-stub.c
@@ -8,7 +8,7 @@
  */
 
 #include "qemu/osdep.h"
-#include "smbios_ipmi.h"
+#include "smbios_build.h"
 
 void smbios_build_type_38_table(void)
 {
diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
index d84e87d608..a1ad28d059 100644
--- a/hw/smbios/smbios_type_38.c
+++ b/hw/smbios/smbios_type_38.c
@@ -12,7 +12,6 @@
 #include "hw/smbios/smbios.h"
 #include "qemu/error-report.h"
 #include "smbios_build.h"
-#include "smbios_ipmi.h"
 
 /* SMBIOS type 38 - IPMI */
 struct smbios_type_38 {
-- 
2.17.2

[Qemu-devel] [PATCH 4/4] hw/smbios: Move to the hw/firmware/ namespace

[Philippe Mathieu-Daudé]

SMBIOS is just another firmware used by some QEMU models.
We will later introduce more firmwares in this namespace.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS                              | 2 +-
 hw/arm/virt.c                            | 2 +-
 hw/i386/pc.c                             | 2 +-
 hw/i386/pc_piix.c                        | 2 +-
 hw/i386/pc_q35.c                         | 2 +-
 hw/smbios/smbios-stub.c                  | 2 +-
 hw/smbios/smbios.c                       | 2 +-
 hw/smbios/smbios_type_38.c               | 2 +-
 include/hw/{smbios => firmware}/smbios.h | 0
 tests/bios-tables-test.c                 | 2 +-
 vl.c                                     | 2 +-
 11 files changed, 10 insertions(+), 10 deletions(-)
 rename include/hw/{smbios => firmware}/smbios.h (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 63effdc473..f7ba25c146 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1238,7 +1238,7 @@ M: Michael S. Tsirkin <mst@redhat.com>
 M: Igor Mammedov <imammedo@redhat.com>
 S: Supported
 F: include/hw/acpi/*
-F: include/hw/smbios/*
+F: include/hw/firmware/smbios.h
 F: hw/mem/*
 F: hw/acpi/*
 F: hw/smbios/*
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f69e7eb399..49813bca1b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -55,7 +55,7 @@
 #include "hw/intc/arm_gic.h"
 #include "hw/intc/arm_gicv3_common.h"
 #include "kvm_arm.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "qapi/visitor.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f095725dba..afa1098082 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -37,7 +37,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "multiboot.h"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7092d6d13f..e3ee3091d2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -30,7 +30,7 @@
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
 #include "hw/display/ramfb.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4702bb13c4..bc31cd5822 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -47,7 +47,7 @@
 #include "hw/i386/amd_iommu.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/display/ramfb.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
diff --git a/hw/smbios/smbios-stub.c b/hw/smbios/smbios-stub.c
index d3a385441a..64e5ba93ec 100644
--- a/hw/smbios/smbios-stub.c
+++ b/hw/smbios/smbios-stub.c
@@ -23,7 +23,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 
 void smbios_entry_add(QemuOpts *opts, Error **errp)
 {
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 9d737642cb..2a61f38637 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -24,7 +24,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/uuid.h"
 #include "sysemu/cpus.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
 #include "smbios_build.h"
diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
index a1ad28d059..0c08f282de 100644
--- a/hw/smbios/smbios_type_38.c
+++ b/hw/smbios/smbios_type_38.c
@@ -9,7 +9,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/ipmi/ipmi.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "qemu/error-report.h"
 #include "smbios_build.h"
 
diff --git a/include/hw/smbios/smbios.h b/include/hw/firmware/smbios.h
similarity index 100%
rename from include/hw/smbios/smbios.h
rename to include/hw/firmware/smbios.h
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d661d9be62..dfa74a5bec 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -13,7 +13,7 @@
 #include "qemu/osdep.h"
 #include <glib/gstdio.h>
 #include "qemu-common.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "qemu/bitmap.h"
 #include "acpi-utils.h"
 #include "boot-sector.h"
diff --git a/vl.c b/vl.c
index a5ae5f23d2..bfc3114dd4 100644
--- a/vl.c
+++ b/vl.c
@@ -61,7 +61,7 @@ int main(int argc, char **argv)
 #include "hw/display/vga.h"
 #include "hw/bt.h"
 #include "sysemu/watchdog.h"
-#include "hw/smbios/smbios.h"
+#include "hw/firmware/smbios.h"
 #include "hw/acpi/acpi.h"
 #include "hw/xen/xen.h"
 #include "hw/qdev.h"
-- 
2.17.2

Re: [Qemu-devel] [PATCH 3/4] hw/smbios: Remove "smbios_ipmi.h"

[Corey Minyard]

On 12/7/18 10:51 AM, Philippe Mathieu-Daudé wrote:
> This header only declare a single function: smbios_build_type_38_table().
> We already have a header that declares such functions: "smbios_build.h".
> Move the declaration and remove the header.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

The two IPMI changes look good to me, a definite improvement. I'm not
sure of the value of having two separate patches, as a single patch would
not be any bigger than the second patch and I don't see any clarity two
patches bring.  But fine either way.

Reviewed-by: Corey Minyard <cminyard@mvista.com>


> ---
>   hw/smbios/smbios.c              |  1 -
>   hw/smbios/smbios_build.h        |  4 ++++
>   hw/smbios/smbios_ipmi.h         | 15 ---------------
>   hw/smbios/smbios_type_38-stub.c |  2 +-
>   hw/smbios/smbios_type_38.c      |  1 -
>   5 files changed, 5 insertions(+), 18 deletions(-)
>   delete mode 100644 hw/smbios/smbios_ipmi.h
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 30bd4731cf..9d737642cb 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -28,7 +28,6 @@
>   #include "hw/loader.h"
>   #include "exec/cpu-common.h"
>   #include "smbios_build.h"
> -#include "smbios_ipmi.h"
>   
>   /* legacy structures and constants for <= 2.0 machines */
>   struct smbios_header {
> diff --git a/hw/smbios/smbios_build.h b/hw/smbios/smbios_build.h
> index 93b360d520..56b5a1e3f3 100644
> --- a/hw/smbios/smbios_build.h
> +++ b/hw/smbios/smbios_build.h
> @@ -3,6 +3,7 @@
>    *
>    * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
>    * Copyright (C) 2013 Red Hat, Inc.
> + * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
>    *
>    * Authors:
>    *  Alex Williamson <alex.williamson@hp.com>
> @@ -96,4 +97,7 @@ extern unsigned smbios_table_cnt;
>           smbios_table_cnt++;                                               \
>       } while (0)
>   
> +/* IPMI SMBIOS firmware handling */
> +void smbios_build_type_38_table(void);
> +
>   #endif /* QEMU_SMBIOS_BUILD_H */
> diff --git a/hw/smbios/smbios_ipmi.h b/hw/smbios/smbios_ipmi.h
> deleted file mode 100644
> index 1c9aae38f2..0000000000
> --- a/hw/smbios/smbios_ipmi.h
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/*
> - * IPMI SMBIOS firmware handling
> - *
> - * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2 or later.
> - * See the COPYING file in the top-level directory.
> - */
> -
> -#ifndef QEMU_SMBIOS_IPMI_H
> -#define QEMU_SMBIOS_IPMI_H
> -
> -void smbios_build_type_38_table(void);
> -
> -#endif /* QEMU_SMBIOS_IPMI_H */
> diff --git a/hw/smbios/smbios_type_38-stub.c b/hw/smbios/smbios_type_38-stub.c
> index fc4516bc8a..14b53d004b 100644
> --- a/hw/smbios/smbios_type_38-stub.c
> +++ b/hw/smbios/smbios_type_38-stub.c
> @@ -8,7 +8,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "smbios_ipmi.h"
> +#include "smbios_build.h"
>   
>   void smbios_build_type_38_table(void)
>   {
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index d84e87d608..a1ad28d059 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -12,7 +12,6 @@
>   #include "hw/smbios/smbios.h"
>   #include "qemu/error-report.h"
>   #include "smbios_build.h"
> -#include "smbios_ipmi.h"
>   
>   /* SMBIOS type 38 - IPMI */
>   struct smbios_type_38 {

Re: [Qemu-devel] [PATCH 0/4] Introduce the hw/firmware/ namespace

[Michael S. Tsirkin]

On Fri, Dec 07, 2018 at 05:50:59PM +0100, Philippe Mathieu-Daudé wrote:
> QEMU interacts with various firmwares. We already have some helpers
> for some firmwares. Later we will add UEFI helpers.
> This series introduce the hw/firmware namespace for this.
> 
> There is no need to move hw/smbios sources into a specific directory,
> we only focus on the headers.

I like this

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

will queue after giving people a bit of time for review.

> Philippe Mathieu-Daudé (4):
>   tests: Remove unused include
>   hw/smbios: Restrict access to "smbios_ipmi.h"
>   hw/smbios: Remove "smbios_ipmi.h"
>   hw/smbios: Move to the hw/firmware/ namespace
> 
>  MAINTAINERS                              |  2 +-
>  hw/arm/virt.c                            |  2 +-
>  hw/i386/pc.c                             |  2 +-
>  hw/i386/pc_piix.c                        |  2 +-
>  hw/i386/pc_q35.c                         |  2 +-
>  hw/smbios/smbios-stub.c                  |  2 +-
>  hw/smbios/smbios.c                       |  3 +--
>  hw/smbios/smbios_build.h                 |  4 ++++
>  hw/smbios/smbios_type_38-stub.c          |  2 +-
>  hw/smbios/smbios_type_38.c               |  3 +--
>  include/hw/{smbios => firmware}/smbios.h |  0
>  include/hw/smbios/ipmi.h                 | 15 ---------------
>  tests/acpi-utils.c                       |  1 -
>  tests/bios-tables-test.c                 |  2 +-
>  vl.c                                     |  2 +-
>  15 files changed, 15 insertions(+), 29 deletions(-)
>  rename include/hw/{smbios => firmware}/smbios.h (100%)
>  delete mode 100644 include/hw/smbios/ipmi.h
> 
> -- 
> 2.17.2

Re: [Qemu-devel] [PATCH 1/4] tests: Remove unused include

[Laszlo Ersek]

On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/acpi-utils.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index 41dc1ea9b4..297af55d39 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -15,7 +15,6 @@
>  #include "qemu/osdep.h"
>  #include <glib/gstdio.h>
>  #include "qemu-common.h"
> -#include "hw/smbios/smbios.h"
>  #include "qemu/bitmap.h"
>  #include "acpi-utils.h"
>  #include "boot-sector.h"
> 

Opinions vary whether empty commit message bodies are good style or not.
Personally I prefer to put at least one sentence in there, even if it
only repeats the subject line. Up to subsystem maintainers to decide I
guess.

With the commit message updated, or not:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] hw/smbios: Restrict access to "smbios_ipmi.h"

[Laszlo Ersek]

On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
> All the consumers of "smbios_ipmi.h" are located in hw/smbios/.

I tried to verify this statement by grepping the tree for
"smbios_ipmi.h". There were zero hits. Please use the more precise
pathname "hw/smbios/ipmi.h". (I can't suggest just "ipmi.h", because
that isn't unique.)

With this update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> There is no need to have this include publicly exposed,
> reduce the visibility by moving it in hw/smbios/.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/smbios/smbios.c                                  | 2 +-
>  include/hw/smbios/ipmi.h => hw/smbios/smbios_ipmi.h | 0
>  hw/smbios/smbios_type_38-stub.c                     | 2 +-
>  hw/smbios/smbios_type_38.c                          | 2 +-
>  4 files changed, 3 insertions(+), 3 deletions(-)
>  rename include/hw/smbios/ipmi.h => hw/smbios/smbios_ipmi.h (100%)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 920939454e..30bd4731cf 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -28,7 +28,7 @@
>  #include "hw/loader.h"
>  #include "exec/cpu-common.h"
>  #include "smbios_build.h"
> -#include "hw/smbios/ipmi.h"
> +#include "smbios_ipmi.h"
>  
>  /* legacy structures and constants for <= 2.0 machines */
>  struct smbios_header {
> diff --git a/include/hw/smbios/ipmi.h b/hw/smbios/smbios_ipmi.h
> similarity index 100%
> rename from include/hw/smbios/ipmi.h
> rename to hw/smbios/smbios_ipmi.h
> diff --git a/hw/smbios/smbios_type_38-stub.c b/hw/smbios/smbios_type_38-stub.c
> index 5b83c9b1f1..fc4516bc8a 100644
> --- a/hw/smbios/smbios_type_38-stub.c
> +++ b/hw/smbios/smbios_type_38-stub.c
> @@ -8,7 +8,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "hw/smbios/ipmi.h"
> +#include "smbios_ipmi.h"
>  
>  void smbios_build_type_38_table(void)
>  {
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index 56e8609c00..d84e87d608 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -9,10 +9,10 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/ipmi/ipmi.h"
> -#include "hw/smbios/ipmi.h"
>  #include "hw/smbios/smbios.h"
>  #include "qemu/error-report.h"
>  #include "smbios_build.h"
> +#include "smbios_ipmi.h"
>  
>  /* SMBIOS type 38 - IPMI */
>  struct smbios_type_38 {
> 

Re: [Qemu-devel] [PATCH 3/4] hw/smbios: Remove "smbios_ipmi.h"

[Laszlo Ersek]

On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
> This header only declare a single function: smbios_build_type_38_table().
> We already have a header that declares such functions: "smbios_build.h".
> Move the declaration and remove the header.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/smbios/smbios.c              |  1 -
>  hw/smbios/smbios_build.h        |  4 ++++
>  hw/smbios/smbios_ipmi.h         | 15 ---------------
>  hw/smbios/smbios_type_38-stub.c |  2 +-
>  hw/smbios/smbios_type_38.c      |  1 -
>  5 files changed, 5 insertions(+), 18 deletions(-)
>  delete mode 100644 hw/smbios/smbios_ipmi.h
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 30bd4731cf..9d737642cb 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -28,7 +28,6 @@
>  #include "hw/loader.h"
>  #include "exec/cpu-common.h"
>  #include "smbios_build.h"
> -#include "smbios_ipmi.h"
>  
>  /* legacy structures and constants for <= 2.0 machines */
>  struct smbios_header {
> diff --git a/hw/smbios/smbios_build.h b/hw/smbios/smbios_build.h
> index 93b360d520..56b5a1e3f3 100644
> --- a/hw/smbios/smbios_build.h
> +++ b/hw/smbios/smbios_build.h
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
>   * Copyright (C) 2013 Red Hat, Inc.
> + * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
>   *
>   * Authors:
>   *  Alex Williamson <alex.williamson@hp.com>
> @@ -96,4 +97,7 @@ extern unsigned smbios_table_cnt;
>          smbios_table_cnt++;                                               \
>      } while (0)
>  
> +/* IPMI SMBIOS firmware handling */
> +void smbios_build_type_38_table(void);
> +
>  #endif /* QEMU_SMBIOS_BUILD_H */
> diff --git a/hw/smbios/smbios_ipmi.h b/hw/smbios/smbios_ipmi.h
> deleted file mode 100644
> index 1c9aae38f2..0000000000
> --- a/hw/smbios/smbios_ipmi.h
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/*
> - * IPMI SMBIOS firmware handling
> - *
> - * Copyright (c) 2015,2016 Corey Minyard, MontaVista Software, LLC
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2 or later.
> - * See the COPYING file in the top-level directory.
> - */
> -
> -#ifndef QEMU_SMBIOS_IPMI_H
> -#define QEMU_SMBIOS_IPMI_H
> -
> -void smbios_build_type_38_table(void);
> -
> -#endif /* QEMU_SMBIOS_IPMI_H */
> diff --git a/hw/smbios/smbios_type_38-stub.c b/hw/smbios/smbios_type_38-stub.c
> index fc4516bc8a..14b53d004b 100644
> --- a/hw/smbios/smbios_type_38-stub.c
> +++ b/hw/smbios/smbios_type_38-stub.c
> @@ -8,7 +8,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "smbios_ipmi.h"
> +#include "smbios_build.h"
>  
>  void smbios_build_type_38_table(void)
>  {
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index d84e87d608..a1ad28d059 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -12,7 +12,6 @@
>  #include "hw/smbios/smbios.h"
>  #include "qemu/error-report.h"
>  #include "smbios_build.h"
> -#include "smbios_ipmi.h"
>  
>  /* SMBIOS type 38 - IPMI */
>  struct smbios_type_38 {
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] hw/smbios: Move to the hw/firmware/ namespace

[Laszlo Ersek]

On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
> SMBIOS is just another firmware used by some QEMU models.
> We will later introduce more firmwares in this namespace.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  MAINTAINERS                              | 2 +-
>  hw/arm/virt.c                            | 2 +-
>  hw/i386/pc.c                             | 2 +-
>  hw/i386/pc_piix.c                        | 2 +-
>  hw/i386/pc_q35.c                         | 2 +-
>  hw/smbios/smbios-stub.c                  | 2 +-
>  hw/smbios/smbios.c                       | 2 +-
>  hw/smbios/smbios_type_38.c               | 2 +-
>  include/hw/{smbios => firmware}/smbios.h | 0
>  tests/bios-tables-test.c                 | 2 +-
>  vl.c                                     | 2 +-
>  11 files changed, 10 insertions(+), 10 deletions(-)
>  rename include/hw/{smbios => firmware}/smbios.h (100%)

(1) Please replace the word "namespace" in the subject, and in the
commit message too, with "subdirectory".

(2) Please replace "firmware" -- both instances -- in the commit message
with "firmware interface", or perhaps "firmware specification".

With the updates:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo


> diff --git a/MAINTAINERS b/MAINTAINERS
> index 63effdc473..f7ba25c146 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1238,7 +1238,7 @@ M: Michael S. Tsirkin <mst@redhat.com>
>  M: Igor Mammedov <imammedo@redhat.com>
>  S: Supported
>  F: include/hw/acpi/*
> -F: include/hw/smbios/*
> +F: include/hw/firmware/smbios.h
>  F: hw/mem/*
>  F: hw/acpi/*
>  F: hw/smbios/*
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f69e7eb399..49813bca1b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -55,7 +55,7 @@
>  #include "hw/intc/arm_gic.h"
>  #include "hw/intc/arm_gicv3_common.h"
>  #include "kvm_arm.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "qapi/visitor.h"
>  #include "standard-headers/linux/input.h"
>  #include "hw/arm/smmuv3.h"
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f095725dba..afa1098082 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -37,7 +37,7 @@
>  #include "hw/pci/pci_bus.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/timer/hpet.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "hw/loader.h"
>  #include "elf.h"
>  #include "multiboot.h"
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7092d6d13f..e3ee3091d2 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -30,7 +30,7 @@
>  #include "hw/i386/pc.h"
>  #include "hw/i386/apic.h"
>  #include "hw/display/ramfb.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_ids.h"
>  #include "hw/usb.h"
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 4702bb13c4..bc31cd5822 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -47,7 +47,7 @@
>  #include "hw/i386/amd_iommu.h"
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/display/ramfb.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "hw/ide/pci.h"
>  #include "hw/ide/ahci.h"
>  #include "hw/usb.h"
> diff --git a/hw/smbios/smbios-stub.c b/hw/smbios/smbios-stub.c
> index d3a385441a..64e5ba93ec 100644
> --- a/hw/smbios/smbios-stub.c
> +++ b/hw/smbios/smbios-stub.c
> @@ -23,7 +23,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  
>  void smbios_entry_add(QemuOpts *opts, Error **errp)
>  {
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 9d737642cb..2a61f38637 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -24,7 +24,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qemu/uuid.h"
>  #include "sysemu/cpus.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "hw/loader.h"
>  #include "exec/cpu-common.h"
>  #include "smbios_build.h"
> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
> index a1ad28d059..0c08f282de 100644
> --- a/hw/smbios/smbios_type_38.c
> +++ b/hw/smbios/smbios_type_38.c
> @@ -9,7 +9,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/ipmi/ipmi.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "qemu/error-report.h"
>  #include "smbios_build.h"
>  
> diff --git a/include/hw/smbios/smbios.h b/include/hw/firmware/smbios.h
> similarity index 100%
> rename from include/hw/smbios/smbios.h
> rename to include/hw/firmware/smbios.h
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d661d9be62..dfa74a5bec 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -13,7 +13,7 @@
>  #include "qemu/osdep.h"
>  #include <glib/gstdio.h>
>  #include "qemu-common.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "qemu/bitmap.h"
>  #include "acpi-utils.h"
>  #include "boot-sector.h"
> diff --git a/vl.c b/vl.c
> index a5ae5f23d2..bfc3114dd4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -61,7 +61,7 @@ int main(int argc, char **argv)
>  #include "hw/display/vga.h"
>  #include "hw/bt.h"
>  #include "sysemu/watchdog.h"
> -#include "hw/smbios/smbios.h"
> +#include "hw/firmware/smbios.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/xen/xen.h"
>  #include "hw/qdev.h"
> 

Re: [Qemu-devel] [PATCH 1/4] tests: Remove unused include

[Philippe Mathieu-Daudé]

On 12/10/18 3:55 PM, Laszlo Ersek wrote:
> On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  tests/acpi-utils.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
>> index 41dc1ea9b4..297af55d39 100644
>> --- a/tests/acpi-utils.c
>> +++ b/tests/acpi-utils.c
>> @@ -15,7 +15,6 @@
>>  #include "qemu/osdep.h"
>>  #include <glib/gstdio.h>
>>  #include "qemu-common.h"
>> -#include "hw/smbios/smbios.h"
>>  #include "qemu/bitmap.h"
>>  #include "acpi-utils.h"
>>  #include "boot-sector.h"
>>
> 
> Opinions vary whether empty commit message bodies are good style or not.
> Personally I prefer to put at least one sentence in there, even if it
> only repeats the subject line. Up to subsystem maintainers to decide I
> guess.

OK, understood.

> 
> With the commit message updated, or not:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 2/4] hw/smbios: Restrict access to "smbios_ipmi.h"

[Philippe Mathieu-Daudé]

On 12/10/18 4:00 PM, Laszlo Ersek wrote:
> On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
>> All the consumers of "smbios_ipmi.h" are located in hw/smbios/.
> 
> I tried to verify this statement by grepping the tree for
> "smbios_ipmi.h". There were zero hits. Please use the more precise
> pathname "hw/smbios/ipmi.h". (I can't suggest just "ipmi.h", because
> that isn't unique.)

I did not notice I used the post-patch pathname, thanks.

> 
> With this update:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH 4/4] hw/smbios: Move to the hw/firmware/ namespace

[Philippe Mathieu-Daudé]

On 12/10/18 4:10 PM, Laszlo Ersek wrote:
> On 12/07/18 17:51, Philippe Mathieu-Daudé wrote:
>> SMBIOS is just another firmware used by some QEMU models.
>> We will later introduce more firmwares in this namespace.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  MAINTAINERS                              | 2 +-
>>  hw/arm/virt.c                            | 2 +-
>>  hw/i386/pc.c                             | 2 +-
>>  hw/i386/pc_piix.c                        | 2 +-
>>  hw/i386/pc_q35.c                         | 2 +-
>>  hw/smbios/smbios-stub.c                  | 2 +-
>>  hw/smbios/smbios.c                       | 2 +-
>>  hw/smbios/smbios_type_38.c               | 2 +-
>>  include/hw/{smbios => firmware}/smbios.h | 0
>>  tests/bios-tables-test.c                 | 2 +-
>>  vl.c                                     | 2 +-
>>  11 files changed, 10 insertions(+), 10 deletions(-)
>>  rename include/hw/{smbios => firmware}/smbios.h (100%)
> 
> (1) Please replace the word "namespace" in the subject, and in the
> commit message too, with "subdirectory".
> 
> (2) Please replace "firmware" -- both instances -- in the commit message
> with "firmware interface", or perhaps "firmware specification".

I'll go with "firmware interface", because I'd like to eventually put
here helpers for firmwares with no public specification.

> With the updates:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks,
> Laszlo