* [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry
@ 2019-07-03 8:04 Simon Horman
2019-07-03 8:34 ` Dave Young
2019-07-10 8:10 ` Simon Horman
0 siblings, 2 replies; 6+ messages in thread
From: Simon Horman @ 2019-07-03 8:04 UTC (permalink / raw)
To: kexec; +Cc: Simon Horman, Dave Young, Kairui Song, Baoquan He, Lianbo Jiang
xenctrl.h defines struct e820entry as:
if defined(__i386__) || defined(__x86_64__)
...
#define E820_RAM 1
...
struct e820entry {
uint64_t addr;
uint64_t size;
uint32_t type;
} __attribute__((packed));
...
#endif
$ dpkg-query -S /usr/include/xenctrl.h
libxen-dev:amd64: /usr/include/xenctrl.h
$ dpkg-query -W libxen-dev:amd64
libxen-dev:amd64 4.8.5+shim4.10.2+xsa282-1+deb9u11
./include/x86/x86-linux.h defines struct e820entry as:
#ifndef E820_RAM
struct e820entry {
uint64_t addr; /* start of memory segment */
uint64_t size; /* size of memory segment */
uint32_t type; /* type of memory segment */
#define E820_RAM 1
...
} __attribute__((packed));
#endif
Since cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
./kexec/arch/i386/kexec-x86-common.c includes
+#include "x86-linux-setup.h"
#include "../../kexec-xen.h"
When xenctrl.h is present the above results in:
$ gcc
...
In file included from kexec/arch/i386/../../kexec-xen.h:5:0,
from kexec/arch/i386/kexec-x86-common.c:43:
/usr/include/xenctrl.h:1271:8: error: redefinition of 'struct e820entry'
struct e820entry {
^~~~~~~~~
In file included from kexec/arch/i386/x86-linux-setup.h:3:0,
from kexec/arch/i386/kexec-x86-common.c:42:
./include/x86/x86-linux.h:16:8: note: originally defined here
struct e820entry {
^~~~~~~~~
...
$ gcc --version | head -1
gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
To militate this this problem re-order the includes so that
x86-linux.h is included after xenctrl.h and thus
struct e820entry will only be defined once due to it
being devined conditionally in x86-linux.h.
In practice the definitions are the same so it should
not matter which is chosen.
It also seems rather unpleasent to me to need to play
with include ordering. Perhaps a better solution in the longer
term would be to rename the local definition of struct e820entry.
Fixes: cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
Signed-off-by: Simon Horman <horms@verge.net.au>
---
kexec/arch/i386/kexec-x86-common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
index 5c55ec8a2cd3..61ea19380ab2 100644
--- a/kexec/arch/i386/kexec-x86-common.c
+++ b/kexec/arch/i386/kexec-x86-common.c
@@ -38,9 +38,9 @@
#include "../../kexec-syscall.h"
#include "../../firmware_memmap.h"
#include "../../crashdump.h"
+#include "../../kexec-xen.h"
#include "kexec-x86.h"
#include "x86-linux-setup.h"
-#include "../../kexec-xen.h"
/* Used below but not present in (older?) xenctrl.h */
#ifndef E820_PMEM
--
2.11.0
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry
2019-07-03 8:04 [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry Simon Horman
@ 2019-07-03 8:34 ` Dave Young
2019-07-03 9:08 ` Kairui Song
2019-07-10 8:10 ` Simon Horman
1 sibling, 1 reply; 6+ messages in thread
From: Dave Young @ 2019-07-03 8:34 UTC (permalink / raw)
To: Simon Horman; +Cc: kexec, Kairui Song, Baoquan He, Lianbo Jiang
On 07/03/19 at 10:04am, Simon Horman wrote:
> xenctrl.h defines struct e820entry as:
>
> if defined(__i386__) || defined(__x86_64__)
> ...
> #define E820_RAM 1
> ...
> struct e820entry {
> uint64_t addr;
> uint64_t size;
> uint32_t type;
> } __attribute__((packed));
> ...
> #endif
>
> $ dpkg-query -S /usr/include/xenctrl.h
> libxen-dev:amd64: /usr/include/xenctrl.h
> $ dpkg-query -W libxen-dev:amd64
> libxen-dev:amd64 4.8.5+shim4.10.2+xsa282-1+deb9u11
>
> ./include/x86/x86-linux.h defines struct e820entry as:
>
> #ifndef E820_RAM
> struct e820entry {
> uint64_t addr; /* start of memory segment */
> uint64_t size; /* size of memory segment */
> uint32_t type; /* type of memory segment */
> #define E820_RAM 1
> ...
> } __attribute__((packed));
> #endif
>
> Since cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> ./kexec/arch/i386/kexec-x86-common.c includes
>
> +#include "x86-linux-setup.h"
> #include "../../kexec-xen.h"
Not sure if those get rsdp code can go to x86-linux-setup.c then no need
the including.
Let's see if Kairui has some thoughts.
>
> When xenctrl.h is present the above results in:
>
> $ gcc
> ...
> In file included from kexec/arch/i386/../../kexec-xen.h:5:0,
> from kexec/arch/i386/kexec-x86-common.c:43:
> /usr/include/xenctrl.h:1271:8: error: redefinition of 'struct e820entry'
> struct e820entry {
> ^~~~~~~~~
>
> In file included from kexec/arch/i386/x86-linux-setup.h:3:0,
> from kexec/arch/i386/kexec-x86-common.c:42:
> ./include/x86/x86-linux.h:16:8: note: originally defined here
> struct e820entry {
> ^~~~~~~~~
> ...
> $ gcc --version | head -1
> gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
>
> To militate this this problem re-order the includes so that
> x86-linux.h is included after xenctrl.h and thus
> struct e820entry will only be defined once due to it
> being devined conditionally in x86-linux.h.
>
> In practice the definitions are the same so it should
> not matter which is chosen.
>
> It also seems rather unpleasent to me to need to play
> with include ordering. Perhaps a better solution in the longer
> term would be to rename the local definition of struct e820entry.
>
> Fixes: cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> Signed-off-by: Simon Horman <horms@verge.net.au>
> ---
> kexec/arch/i386/kexec-x86-common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> index 5c55ec8a2cd3..61ea19380ab2 100644
> --- a/kexec/arch/i386/kexec-x86-common.c
> +++ b/kexec/arch/i386/kexec-x86-common.c
> @@ -38,9 +38,9 @@
> #include "../../kexec-syscall.h"
> #include "../../firmware_memmap.h"
> #include "../../crashdump.h"
> +#include "../../kexec-xen.h"
> #include "kexec-x86.h"
> #include "x86-linux-setup.h"
> -#include "../../kexec-xen.h"
>
> /* Used below but not present in (older?) xenctrl.h */
> #ifndef E820_PMEM
> --
> 2.11.0
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry
2019-07-03 8:34 ` Dave Young
@ 2019-07-03 9:08 ` Kairui Song
2019-07-05 2:28 ` Dave Young
0 siblings, 1 reply; 6+ messages in thread
From: Kairui Song @ 2019-07-03 9:08 UTC (permalink / raw)
To: Dave Young; +Cc: Simon Horman, kexec, Lianbo Jiang, Baoquan He
On Wed, Jul 3, 2019 at 4:34 PM Dave Young <dyoung@redhat.com> wrote:
>
> On 07/03/19 at 10:04am, Simon Horman wrote:
> > xenctrl.h defines struct e820entry as:
> >
> > if defined(__i386__) || defined(__x86_64__)
> > ...
> > #define E820_RAM 1
> > ...
> > struct e820entry {
> > uint64_t addr;
> > uint64_t size;
> > uint32_t type;
> > } __attribute__((packed));
> > ...
> > #endif
> >
> > $ dpkg-query -S /usr/include/xenctrl.h
> > libxen-dev:amd64: /usr/include/xenctrl.h
> > $ dpkg-query -W libxen-dev:amd64
> > libxen-dev:amd64 4.8.5+shim4.10.2+xsa282-1+deb9u11
> >
> > ./include/x86/x86-linux.h defines struct e820entry as:
> >
> > #ifndef E820_RAM
> > struct e820entry {
> > uint64_t addr; /* start of memory segment */
> > uint64_t size; /* size of memory segment */
> > uint32_t type; /* type of memory segment */
> > #define E820_RAM 1
> > ...
> > } __attribute__((packed));
> > #endif
> >
> > Since cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> > ./kexec/arch/i386/kexec-x86-common.c includes
> >
> > +#include "x86-linux-setup.h"
> > #include "../../kexec-xen.h"
>
> Not sure if those get rsdp code can go to x86-linux-setup.c then no need
> the including.
>
> Let's see if Kairui has some thoughts.
>
Yes, move the helper to x86-linux-setup.c should fix it too. So
following patch should also be able to fix it:
---
kexec/arch/i386/kexec-x86-common.c | 45 ------------------------------
kexec/arch/i386/kexec-x86.h | 1 -
kexec/arch/i386/x86-linux-setup.c | 43 ++++++++++++++++++++++++++++
kexec/arch/i386/x86-linux-setup.h | 2 +-
4 files changed, 44 insertions(+), 47 deletions(-)
diff --git a/kexec/arch/i386/kexec-x86-common.c
b/kexec/arch/i386/kexec-x86-common.c
index 5c55ec8..63a2823 100644
--- a/kexec/arch/i386/kexec-x86-common.c
+++ b/kexec/arch/i386/kexec-x86-common.c
@@ -39,7 +39,6 @@
#include "../../firmware_memmap.h"
#include "../../crashdump.h"
#include "kexec-x86.h"
-#include "x86-linux-setup.h"
#include "../../kexec-xen.h"
/* Used below but not present in (older?) xenctrl.h */
@@ -392,47 +391,3 @@ int get_memory_ranges(struct memory_range
**range, int *ranges,
return ret;
}
-
-static uint64_t bootparam_get_acpi_rsdp(void) {
- uint64_t acpi_rsdp = 0;
- off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
-
- if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
- return 0;
-
- return acpi_rsdp;
-}
-
-static uint64_t efi_get_acpi_rsdp(void) {
- FILE *fp;
- char line[MAX_LINE], *s;
- uint64_t acpi_rsdp = 0;
-
- fp = fopen("/sys/firmware/efi/systab", "r");
- if (!fp)
- return acpi_rsdp;
-
- while(fgets(line, sizeof(line), fp) != 0) {
- /* ACPI20= always goes before ACPI= */
- if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
- s = strchr(line, '=') + 1;
- sscanf(s, "0x%lx", &acpi_rsdp);
- break;
- }
- }
- fclose(fp);
-
- return acpi_rsdp;
-}
-
-uint64_t get_acpi_rsdp(void)
-{
- uint64_t acpi_rsdp = 0;
-
- acpi_rsdp = bootparam_get_acpi_rsdp();
-
- if (!acpi_rsdp)
- acpi_rsdp = efi_get_acpi_rsdp();
-
- return acpi_rsdp;
-}
diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
index 1b58c3b..c2bcd37 100644
--- a/kexec/arch/i386/kexec-x86.h
+++ b/kexec/arch/i386/kexec-x86.h
@@ -86,5 +86,4 @@ int nbi_load(int argc, char **argv, const char *buf,
off_t len,
void nbi_usage(void);
extern unsigned xen_e820_to_kexec_type(uint32_t type);
-extern uint64_t get_acpi_rsdp(void);
#endif /* KEXEC_X86_H */
diff --git a/kexec/arch/i386/x86-linux-setup.c
b/kexec/arch/i386/x86-linux-setup.c
index 057ee14..588b1f4 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -846,6 +846,49 @@ out:
return;
}
+static uint64_t bootparam_get_acpi_rsdp(void) {
+ uint64_t acpi_rsdp = 0;
+ off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
+
+ if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
+ return 0;
+
+ return acpi_rsdp;
+}
+
+static uint64_t efi_get_acpi_rsdp(void) {
+ FILE *fp;
+ char line[MAX_LINE], *s;
+ uint64_t acpi_rsdp = 0;
+
+ fp = fopen("/sys/firmware/efi/systab", "r");
+ if (!fp)
+ return acpi_rsdp;
+
+ while(fgets(line, sizeof(line), fp) != 0) {
+ /* ACPI20= always goes before ACPI= */
+ if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
+ s = strchr(line, '=') + 1;
+ sscanf(s, "0x%lx", &acpi_rsdp);
+ break;
+ }
+ }
+ fclose(fp);
+
+ return acpi_rsdp;
+}
+
+uint64_t get_acpi_rsdp(void)
+{
+ uint64_t acpi_rsdp = 0;
+
+ acpi_rsdp = bootparam_get_acpi_rsdp();
+
+ if (!acpi_rsdp)
+ acpi_rsdp = efi_get_acpi_rsdp();
+
+ return acpi_rsdp;
+}
void setup_linux_system_parameters(struct kexec_info *info,
struct x86_linux_param_header *real_mode)
{
diff --git a/kexec/arch/i386/x86-linux-setup.h
b/kexec/arch/i386/x86-linux-setup.h
index 0c651e5..1e81805 100644
--- a/kexec/arch/i386/x86-linux-setup.h
+++ b/kexec/arch/i386/x86-linux-setup.h
@@ -22,7 +22,7 @@ static inline void setup_linux_bootloader_parameters(
void setup_linux_system_parameters(struct kexec_info *info,
struct x86_linux_param_header *real_mode);
int get_bootparam(void *buf, off_t offset, size_t size);
-
+uint64_t get_acpi_rsdp(void);
#define SETUP_BASE 0x90000
#define KERN32_BASE 0x100000 /* 1MB */
--
2.21.0
Best Regards,
Kairui Song
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry
2019-07-03 9:08 ` Kairui Song
@ 2019-07-05 2:28 ` Dave Young
0 siblings, 0 replies; 6+ messages in thread
From: Dave Young @ 2019-07-05 2:28 UTC (permalink / raw)
To: Kairui Song; +Cc: Simon Horman, kexec, Lianbo Jiang, Baoquan He
Hi Kairui,
On 07/03/19 at 05:08pm, Kairui Song wrote:
> On Wed, Jul 3, 2019 at 4:34 PM Dave Young <dyoung@redhat.com> wrote:
> >
> > On 07/03/19 at 10:04am, Simon Horman wrote:
> > > xenctrl.h defines struct e820entry as:
> > >
> > > if defined(__i386__) || defined(__x86_64__)
> > > ...
> > > #define E820_RAM 1
> > > ...
> > > struct e820entry {
> > > uint64_t addr;
> > > uint64_t size;
> > > uint32_t type;
> > > } __attribute__((packed));
> > > ...
> > > #endif
> > >
> > > $ dpkg-query -S /usr/include/xenctrl.h
> > > libxen-dev:amd64: /usr/include/xenctrl.h
> > > $ dpkg-query -W libxen-dev:amd64
> > > libxen-dev:amd64 4.8.5+shim4.10.2+xsa282-1+deb9u11
> > >
> > > ./include/x86/x86-linux.h defines struct e820entry as:
> > >
> > > #ifndef E820_RAM
> > > struct e820entry {
> > > uint64_t addr; /* start of memory segment */
> > > uint64_t size; /* size of memory segment */
> > > uint32_t type; /* type of memory segment */
> > > #define E820_RAM 1
> > > ...
> > > } __attribute__((packed));
> > > #endif
> > >
> > > Since cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> > > ./kexec/arch/i386/kexec-x86-common.c includes
> > >
> > > +#include "x86-linux-setup.h"
> > > #include "../../kexec-xen.h"
> >
> > Not sure if those get rsdp code can go to x86-linux-setup.c then no need
> > the including.
> >
> > Let's see if Kairui has some thoughts.
> >
>
> Yes, move the helper to x86-linux-setup.c should fix it too. So
> following patch should also be able to fix it:
It would be better to send a formal patch, seems the patch format is not
correct. no sob, and no ident.
>
> ---
> kexec/arch/i386/kexec-x86-common.c | 45 ------------------------------
> kexec/arch/i386/kexec-x86.h | 1 -
> kexec/arch/i386/x86-linux-setup.c | 43 ++++++++++++++++++++++++++++
> kexec/arch/i386/x86-linux-setup.h | 2 +-
> 4 files changed, 44 insertions(+), 47 deletions(-)
>
> diff --git a/kexec/arch/i386/kexec-x86-common.c
> b/kexec/arch/i386/kexec-x86-common.c
> index 5c55ec8..63a2823 100644
> --- a/kexec/arch/i386/kexec-x86-common.c
> +++ b/kexec/arch/i386/kexec-x86-common.c
> @@ -39,7 +39,6 @@
> #include "../../firmware_memmap.h"
> #include "../../crashdump.h"
> #include "kexec-x86.h"
> -#include "x86-linux-setup.h"
> #include "../../kexec-xen.h"
>
> /* Used below but not present in (older?) xenctrl.h */
> @@ -392,47 +391,3 @@ int get_memory_ranges(struct memory_range
> **range, int *ranges,
>
> return ret;
> }
> -
> -static uint64_t bootparam_get_acpi_rsdp(void) {
> - uint64_t acpi_rsdp = 0;
> - off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
> -
> - if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
> - return 0;
> -
> - return acpi_rsdp;
> -}
> -
> -static uint64_t efi_get_acpi_rsdp(void) {
> - FILE *fp;
> - char line[MAX_LINE], *s;
> - uint64_t acpi_rsdp = 0;
> -
> - fp = fopen("/sys/firmware/efi/systab", "r");
> - if (!fp)
> - return acpi_rsdp;
> -
> - while(fgets(line, sizeof(line), fp) != 0) {
> - /* ACPI20= always goes before ACPI= */
> - if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
> - s = strchr(line, '=') + 1;
> - sscanf(s, "0x%lx", &acpi_rsdp);
> - break;
> - }
> - }
> - fclose(fp);
> -
> - return acpi_rsdp;
> -}
> -
> -uint64_t get_acpi_rsdp(void)
> -{
> - uint64_t acpi_rsdp = 0;
> -
> - acpi_rsdp = bootparam_get_acpi_rsdp();
> -
> - if (!acpi_rsdp)
> - acpi_rsdp = efi_get_acpi_rsdp();
> -
> - return acpi_rsdp;
> -}
> diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
> index 1b58c3b..c2bcd37 100644
> --- a/kexec/arch/i386/kexec-x86.h
> +++ b/kexec/arch/i386/kexec-x86.h
> @@ -86,5 +86,4 @@ int nbi_load(int argc, char **argv, const char *buf,
> off_t len,
> void nbi_usage(void);
>
> extern unsigned xen_e820_to_kexec_type(uint32_t type);
> -extern uint64_t get_acpi_rsdp(void);
> #endif /* KEXEC_X86_H */
> diff --git a/kexec/arch/i386/x86-linux-setup.c
> b/kexec/arch/i386/x86-linux-setup.c
> index 057ee14..588b1f4 100644
> --- a/kexec/arch/i386/x86-linux-setup.c
> +++ b/kexec/arch/i386/x86-linux-setup.c
> @@ -846,6 +846,49 @@ out:
> return;
> }
>
> +static uint64_t bootparam_get_acpi_rsdp(void) {
> + uint64_t acpi_rsdp = 0;
> + off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
> +
> + if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
> + return 0;
> +
> + return acpi_rsdp;
> +}
> +
> +static uint64_t efi_get_acpi_rsdp(void) {
> + FILE *fp;
> + char line[MAX_LINE], *s;
> + uint64_t acpi_rsdp = 0;
> +
> + fp = fopen("/sys/firmware/efi/systab", "r");
> + if (!fp)
> + return acpi_rsdp;
> +
> + while(fgets(line, sizeof(line), fp) != 0) {
> + /* ACPI20= always goes before ACPI= */
> + if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
> + s = strchr(line, '=') + 1;
> + sscanf(s, "0x%lx", &acpi_rsdp);
> + break;
> + }
> + }
> + fclose(fp);
> +
> + return acpi_rsdp;
> +}
> +
> +uint64_t get_acpi_rsdp(void)
> +{
> + uint64_t acpi_rsdp = 0;
> +
> + acpi_rsdp = bootparam_get_acpi_rsdp();
> +
> + if (!acpi_rsdp)
> + acpi_rsdp = efi_get_acpi_rsdp();
> +
> + return acpi_rsdp;
> +}
> void setup_linux_system_parameters(struct kexec_info *info,
> struct x86_linux_param_header *real_mode)
> {
> diff --git a/kexec/arch/i386/x86-linux-setup.h
> b/kexec/arch/i386/x86-linux-setup.h
> index 0c651e5..1e81805 100644
> --- a/kexec/arch/i386/x86-linux-setup.h
> +++ b/kexec/arch/i386/x86-linux-setup.h
> @@ -22,7 +22,7 @@ static inline void setup_linux_bootloader_parameters(
> void setup_linux_system_parameters(struct kexec_info *info,
> struct x86_linux_param_header *real_mode);
> int get_bootparam(void *buf, off_t offset, size_t size);
> -
> +uint64_t get_acpi_rsdp(void);
>
> #define SETUP_BASE 0x90000
> #define KERN32_BASE 0x100000 /* 1MB */
> --
> 2.21.0
>
> Best Regards,
> Kairui Song
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry
2019-07-03 8:04 [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry Simon Horman
2019-07-03 8:34 ` Dave Young
@ 2019-07-10 8:10 ` Simon Horman
2019-07-10 8:55 ` Kairui Song
1 sibling, 1 reply; 6+ messages in thread
From: Simon Horman @ 2019-07-10 8:10 UTC (permalink / raw)
To: kexec; +Cc: Dave Young, Kairui Song, Baoquan He, Lianbo Jiang
On Wed, Jul 03, 2019 at 10:04:32AM +0200, Simon Horman wrote:
> xenctrl.h defines struct e820entry as:
>
> if defined(__i386__) || defined(__x86_64__)
> ...
> #define E820_RAM 1
> ...
> struct e820entry {
> uint64_t addr;
> uint64_t size;
> uint32_t type;
> } __attribute__((packed));
> ...
> #endif
>
> $ dpkg-query -S /usr/include/xenctrl.h
> libxen-dev:amd64: /usr/include/xenctrl.h
> $ dpkg-query -W libxen-dev:amd64
> libxen-dev:amd64 4.8.5+shim4.10.2+xsa282-1+deb9u11
>
> ./include/x86/x86-linux.h defines struct e820entry as:
>
> #ifndef E820_RAM
> struct e820entry {
> uint64_t addr; /* start of memory segment */
> uint64_t size; /* size of memory segment */
> uint32_t type; /* type of memory segment */
> #define E820_RAM 1
> ...
> } __attribute__((packed));
> #endif
>
> Since cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> ./kexec/arch/i386/kexec-x86-common.c includes
>
> +#include "x86-linux-setup.h"
> #include "../../kexec-xen.h"
>
> When xenctrl.h is present the above results in:
>
> $ gcc
> ...
> In file included from kexec/arch/i386/../../kexec-xen.h:5:0,
> from kexec/arch/i386/kexec-x86-common.c:43:
> /usr/include/xenctrl.h:1271:8: error: redefinition of 'struct e820entry'
> struct e820entry {
> ^~~~~~~~~
>
> In file included from kexec/arch/i386/x86-linux-setup.h:3:0,
> from kexec/arch/i386/kexec-x86-common.c:42:
> ./include/x86/x86-linux.h:16:8: note: originally defined here
> struct e820entry {
> ^~~~~~~~~
> ...
> $ gcc --version | head -1
> gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
>
> To militate this this problem re-order the includes so that
> x86-linux.h is included after xenctrl.h and thus
> struct e820entry will only be defined once due to it
> being devined conditionally in x86-linux.h.
>
> In practice the definitions are the same so it should
> not matter which is chosen.
>
> It also seems rather unpleasent to me to need to play
> with include ordering. Perhaps a better solution in the longer
> term would be to rename the local definition of struct e820entry.
>
> Fixes: cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> Signed-off-by: Simon Horman <horms@verge.net.au>
I have applied this change.
> ---
> kexec/arch/i386/kexec-x86-common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> index 5c55ec8a2cd3..61ea19380ab2 100644
> --- a/kexec/arch/i386/kexec-x86-common.c
> +++ b/kexec/arch/i386/kexec-x86-common.c
> @@ -38,9 +38,9 @@
> #include "../../kexec-syscall.h"
> #include "../../firmware_memmap.h"
> #include "../../crashdump.h"
> +#include "../../kexec-xen.h"
> #include "kexec-x86.h"
> #include "x86-linux-setup.h"
> -#include "../../kexec-xen.h"
>
> /* Used below but not present in (older?) xenctrl.h */
> #ifndef E820_PMEM
> --
> 2.11.0
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry
2019-07-10 8:10 ` Simon Horman
@ 2019-07-10 8:55 ` Kairui Song
0 siblings, 0 replies; 6+ messages in thread
From: Kairui Song @ 2019-07-10 8:55 UTC (permalink / raw)
To: Simon Horman; +Cc: Dave Young, kexec, Lianbo Jiang, Baoquan He
On Wed, Jul 10, 2019 at 4:11 PM Simon Horman <horms@verge.net.au> wrote:
>
> On Wed, Jul 03, 2019 at 10:04:32AM +0200, Simon Horman wrote:
> > xenctrl.h defines struct e820entry as:
> >
> > if defined(__i386__) || defined(__x86_64__)
> > ...
> > #define E820_RAM 1
> > ...
> > struct e820entry {
> > uint64_t addr;
> > uint64_t size;
> > uint32_t type;
> > } __attribute__((packed));
> > ...
> > #endif
> >
> > $ dpkg-query -S /usr/include/xenctrl.h
> > libxen-dev:amd64: /usr/include/xenctrl.h
> > $ dpkg-query -W libxen-dev:amd64
> > libxen-dev:amd64 4.8.5+shim4.10.2+xsa282-1+deb9u11
> >
> > ./include/x86/x86-linux.h defines struct e820entry as:
> >
> > #ifndef E820_RAM
> > struct e820entry {
> > uint64_t addr; /* start of memory segment */
> > uint64_t size; /* size of memory segment */
> > uint32_t type; /* type of memory segment */
> > #define E820_RAM 1
> > ...
> > } __attribute__((packed));
> > #endif
> >
> > Since cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> > ./kexec/arch/i386/kexec-x86-common.c includes
> >
> > +#include "x86-linux-setup.h"
> > #include "../../kexec-xen.h"
> >
> > When xenctrl.h is present the above results in:
> >
> > $ gcc
> > ...
> > In file included from kexec/arch/i386/../../kexec-xen.h:5:0,
> > from kexec/arch/i386/kexec-x86-common.c:43:
> > /usr/include/xenctrl.h:1271:8: error: redefinition of 'struct e820entry'
> > struct e820entry {
> > ^~~~~~~~~
> >
> > In file included from kexec/arch/i386/x86-linux-setup.h:3:0,
> > from kexec/arch/i386/kexec-x86-common.c:42:
> > ./include/x86/x86-linux.h:16:8: note: originally defined here
> > struct e820entry {
> > ^~~~~~~~~
> > ...
> > $ gcc --version | head -1
> > gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
> >
> > To militate this this problem re-order the includes so that
> > x86-linux.h is included after xenctrl.h and thus
> > struct e820entry will only be defined once due to it
> > being devined conditionally in x86-linux.h.
> >
> > In practice the definitions are the same so it should
> > not matter which is chosen.
> >
> > It also seems rather unpleasent to me to need to play
> > with include ordering. Perhaps a better solution in the longer
> > term would be to rename the local definition of struct e820entry.
> >
> > Fixes: cedeee0a3007 ("x86: Introduce helpers for getting RSDP address")
> > Signed-off-by: Simon Horman <horms@verge.net.au>
>
> I have applied this change.
>
Thanks for the fix, it looks good, so the "move the helpers to
x86-linux-setup.c" patch should be not needed now.
--
Best Regards,
Kairui Song
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-10 9:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-03 8:04 [PATCH kexec-tools] x86: re-order includes to avoid duplicate struct e820entry Simon Horman
2019-07-03 8:34 ` Dave Young
2019-07-03 9:08 ` Kairui Song
2019-07-05 2:28 ` Dave Young
2019-07-10 8:10 ` Simon Horman
2019-07-10 8:55 ` Kairui Song
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.