* [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.