All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	qemu-ppc@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>,
	Artyom Tarasenko <atar4qemu@gmail.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Thomas Huth <thuth@redhat.com>,
	Peter Maydell <peter.maydell@linaro.org>
Subject: Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"
Date: Mon, 29 Apr 2019 17:41:59 +0200	[thread overview]
Message-ID: <193e09a9-da42-1fe1-3ef1-313d6835b66b@redhat.com> (raw)
In-Reply-To: <0f78535e-e137-67f1-17bd-7a4ca4b03cae@redhat.com>

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é <philmd@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)?

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 <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 

WARNING: multiple messages have this Message-ID (diff)
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>, qemu-devel@nongnu.org
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Thomas Huth <thuth@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	qemu-ppc@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Artyom Tarasenko <atar4qemu@gmail.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h"
Date: Mon, 29 Apr 2019 17:41:59 +0200	[thread overview]
Message-ID: <193e09a9-da42-1fe1-3ef1-313d6835b66b@redhat.com> (raw)
Message-ID: <20190429154159.0VCX71JZHt2OGVKpZn5Bx6M2xsS8YbO4jTisYrOvDa4@z> (raw)
In-Reply-To: <0f78535e-e137-67f1-17bd-7a4ca4b03cae@redhat.com>

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é <philmd@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)?

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 <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 


  reply	other threads:[~2019-04-29 15:42 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-22 19:50 [Qemu-devel] [PATCH v3 0/7] fw_cfg: Improve tracing Philippe Mathieu-Daudé
2019-04-22 19:50 ` Philippe Mathieu-Daudé
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 1/7] hw/nvram/fw_cfg: Add trace events Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 2/7] hw/nvram/fw_cfg: Add fw_cfg_arch_key_name() Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-23 18:32   ` Laszlo Ersek
2019-04-23 18:32     ` Laszlo Ersek
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 3/7] hw/i386: Extract fw_cfg definitions to local "fw_cfg.h" Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-23 18:38   ` Laszlo Ersek
2019-04-23 18:38     ` Laszlo Ersek
2019-04-29 15:41     ` Philippe Mathieu-Daudé [this message]
2019-04-29 15:41       ` Philippe Mathieu-Daudé
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 4/7] hw/i386: Implement fw_cfg_arch_key_name() Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-23 18:40   ` Laszlo Ersek
2019-04-23 18:40     ` Laszlo Ersek
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 5/7] hw/ppc: " Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-23  1:20   ` David Gibson
2019-04-23  1:20     ` David Gibson
2019-04-23  7:31     ` Philippe Mathieu-Daudé
2019-04-23  7:31       ` Philippe Mathieu-Daudé
2019-04-23 19:02   ` Laszlo Ersek
2019-04-23 19:02     ` Laszlo Ersek
2019-04-29 16:01     ` Philippe Mathieu-Daudé
2019-04-29 16:01       ` Philippe Mathieu-Daudé
2019-04-30  9:41       ` Laszlo Ersek
2019-04-30  9:41         ` Laszlo Ersek
2019-04-30  9:58         ` Philippe Mathieu-Daudé
2019-04-30  9:58           ` Philippe Mathieu-Daudé
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 6/7] hw/sparc: " Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-23 19:05   ` Laszlo Ersek
2019-04-23 19:05     ` Laszlo Ersek
2019-04-22 19:50 ` [Qemu-devel] [PATCH v3 7/7] hw/sparc64: " Philippe Mathieu-Daudé
2019-04-22 19:50   ` Philippe Mathieu-Daudé
2019-04-23 19:06   ` Laszlo Ersek
2019-04-23 19:06     ` Laszlo Ersek
2019-05-22 14:24 ` [Qemu-devel] [PATCH v3 0/7] fw_cfg: Improve tracing Philippe Mathieu-Daudé

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=193e09a9-da42-1fe1-3ef1-313d6835b66b@redhat.com \
    --to=philmd@redhat.com \
    --cc=atar4qemu@gmail.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=ehabkost@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=lersek@redhat.com \
    --cc=mark.cave-ayland@ilande.co.uk \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.