Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 4/23/19 8:38 PM, Laszlo Ersek wrote:
> 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)?

I asked few people on IRC, than googled and finally kept this link
(understable enough for me):
https://en.wikipedia.org/wiki/Derivative_work#When_does_derivative-work_copyright_apply?

  US Copyright Office Circular 14: Derivative Works notes that:

   [...] To be copyrightable, a derivative work must be different
   enough from the original to be regarded as a "new work" or
   must contain a substantial amount of new material. Making minor
   changes or additions of little substance to a preexisting work
   will not qualify the work as a new version for copyright
   purposes. [...]

Since I'm simply moving lines of code with no logical modification, I
understood it is not sufficient to add a new copyright entry...

> (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!

There is a slow effort to keep API namespaces as simple/strict as
possible, but the cleaning is taking time :)

- hw/i386/*.h contains declarations used by
  hw/i386/{.,kvm,xen,../hyperv}*.c

- include/hw/i386/*.h contains declaration of X86-specific devices which
  are not located in hw/i386:

  - hw/acpi (this will be cleaned with merging NEMU patches)
  - hw/intc (apic, ioapic)
  - hw/timer (hpet)
  - hw/isa (southbridge, superio)
  - hw/pci-host (northbridge)

I am spending my personal time cleaning this, since it is not a project
priority, so it is taking me a lot.

>> +
>> +#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!

> 
> Thanks
> Laszlo
>