qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to l


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"
Date: Tue, 23 Apr 2019 20:38:47 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

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é <address@hidden>
> ---
>  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 <address@hidden>

Thanks
Laszlo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]