On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
> Extract the architecture-specific fw_cfg definitions to "fw_cfg.h".
>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> ---
> hw/i386/fw_cfg.h | 20 ++++++++++++++++++++
> hw/i386/pc.c | 7 +------
> 2 files changed, 21 insertions(+), 6 deletions(-)
> create mode 100644 hw/i386/fw_cfg.h
>
> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
> new file mode 100644
> index 00000000000..17a4bc32f22
> --- /dev/null
> +++ b/hw/i386/fw_cfg.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU fw_cfg helpers (X86 specific)
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * SPDX-License-Identifier: MIT
> + */
(1) This is a new file -- I understand it could be plain code movement,
but shouldn't you add your (= RH's) copyright notice too (beyond Fabrice's)?
(2) I admit I'm confused by the difference between:
- include/hw/i386/*.h
- hw/i386/*.h
One could say that the latter is "internal" (compare e.g.
"intel_iommu.h" from the former and "intel_iommu_internal.h" from the
latter) -- but then, as a counter-example, we have *both* "ioapic.h" and
"ioapic_internal.h" under the former!
> +
> +#ifndef HW_I386_FW_CFG_H
> +#define HW_I386_FW_CFG_H
> +
> +#include "hw/nvram/fw_cfg.h"
> +
> +#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
> +#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> +#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> +#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> +#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
> +
> +#endif
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2c..acb8fd9667d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -30,6 +30,7 @@
> #include "hw/char/parallel.h"
> #include "hw/i386/apic.h"
> #include "hw/i386/topology.h"
> +#include "hw/i386/fw_cfg.h"
> #include "sysemu/cpus.h"
> #include "hw/block/fdc.h"
> #include "hw/ide.h"
> @@ -88,12 +89,6 @@
> #define DPRINTF(fmt, ...)
> #endif
>
> -#define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
> -#define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> -#define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> -#define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
> -#define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
> -
> #define E820_NR_ENTRIES 16
>
> struct e820_entry {
>
I'm not insisting on any particular code changes here, just please
consider (1) and (2) above in some way. (Stating why the code is fine
as-is is OK by me too.)
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
