* [PATCH] xen: Work around Clang generating .data.rel.ro section for init-only files
@ 2016-02-22 16:57 Andrew Cooper
2016-02-23 9:47 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2016-02-22 16:57 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
these contain no global symbols, they should be .data.rel.ro.local. This
breaks the SPECIAL_DATA_SECTIONS check when converting the transition units to
being init-only.
For alternatives.c, explicitly move the nops arrays into __initconst. For efi
boot.c, manually create the optimisation performed by Clang by collapsing the
switch statement into a lookup table. The double use of const is required to
avoid breaking the ARM build by creating a section type conflict with
fdt_guid.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
v2: Fix ARM build
---
xen/arch/x86/alternative.c | 4 ++--
xen/common/efi/boot.c | 58 +++++++++++++++-------------------------------
2 files changed, 21 insertions(+), 41 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 46ac0fd..02b5e92 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
K8_NOP7,
K8_NOP8
};
-static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
+static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
NULL,
k8nops,
k8nops + 1,
@@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
P6_NOP7,
P6_NOP8
};
-static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
+static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = {
NULL,
p6nops,
p6nops + 1,
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 53c7452..e0fdf40 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -241,53 +241,33 @@ static void __init noreturn blexit(const CHAR16 *str)
/* generic routine for printing error messages */
static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
{
+ static const CHAR16* const ErrCodeToStr[] __initconst = {
+ [~EFI_ERROR_MASK & EFI_NOT_FOUND] = L"Not found",
+ [~EFI_ERROR_MASK & EFI_NO_MEDIA] = L"The device has no media",
+ [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED] = L"Media changed",
+ [~EFI_ERROR_MASK & EFI_DEVICE_ERROR] = L"Device error",
+ [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED] = L"Volume corrupted",
+ [~EFI_ERROR_MASK & EFI_ACCESS_DENIED] = L"Access denied",
+ [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES] = L"Out of resources",
+ [~EFI_ERROR_MASK & EFI_VOLUME_FULL] = L"Volume is full",
+ [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION] = L"Security violation",
+ [~EFI_ERROR_MASK & EFI_CRC_ERROR] = L"CRC error",
+ [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA] = L"Compromised data",
+ [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL] = L"Buffer too small",
+ };
+ EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
+
StdOut = StdErr;
PrintErr((CHAR16 *)mesg);
PrintErr(L": ");
- switch (ErrCode)
+ if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
+ mesg = ErrCodeToStr[ErrIdx];
+ else
{
- case EFI_NOT_FOUND:
- mesg = L"Not found";
- break;
- case EFI_NO_MEDIA:
- mesg = L"The device has no media";
- break;
- case EFI_MEDIA_CHANGED:
- mesg = L"Media changed";
- break;
- case EFI_DEVICE_ERROR:
- mesg = L"Device error";
- break;
- case EFI_VOLUME_CORRUPTED:
- mesg = L"Volume corrupted";
- break;
- case EFI_ACCESS_DENIED:
- mesg = L"Access denied";
- break;
- case EFI_OUT_OF_RESOURCES:
- mesg = L"Out of resources";
- break;
- case EFI_VOLUME_FULL:
- mesg = L"Volume is full";
- break;
- case EFI_SECURITY_VIOLATION:
- mesg = L"Security violation";
- break;
- case EFI_CRC_ERROR:
- mesg = L"CRC error";
- break;
- case EFI_COMPROMISED_DATA:
- mesg = L"Compromised data";
- break;
- case EFI_BUFFER_TOO_SMALL:
- mesg = L"Buffer too small";
- break;
- default:
PrintErr(L"ErrCode: ");
DisplayUint(ErrCode, 0);
mesg = NULL;
- break;
}
blexit(mesg);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-22 16:57 [PATCH] xen: Work around Clang generating .data.rel.ro section for init-only files Andrew Cooper
@ 2016-02-23 9:47 ` Jan Beulich
2016-02-23 10:10 ` Andrew Cooper
2016-02-23 18:36 ` [PATCH v3] " Andrew Cooper
0 siblings, 2 replies; 11+ messages in thread
From: Jan Beulich @ 2016-02-23 9:47 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 22.02.16 at 17:57, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
> K8_NOP7,
> K8_NOP8
> };
> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
> +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
> NULL,
> k8nops,
> k8nops + 1,
> @@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
> P6_NOP7,
> P6_NOP8
> };
> -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
> +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = {
> NULL,
> p6nops,
> p6nops + 1,
Afaict this will cause the same section type conflict issue as did
the command line parameter constification change that I needed
to revert yesterday. I'm afraid there's no way around introducing
a sibling to __initconst (e.g. __initrelro or __initconst_r) specifying
another section name (e.g. .init.rodata.rel), which then needs to
be used on data objects incurring relocations. And iirc that was
also the reason why these annotation have got left off originally
here.
The issue is that with -fPIC these sections needing relocations
need to be marked writable even if the objects are "const".
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 9:47 ` Jan Beulich
@ 2016-02-23 10:10 ` Andrew Cooper
2016-02-23 10:32 ` Jan Beulich
2016-02-23 18:36 ` [PATCH v3] " Andrew Cooper
1 sibling, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2016-02-23 10:10 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 23/02/16 09:47, Jan Beulich wrote:
>>>> On 22.02.16 at 17:57, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
>> K8_NOP7,
>> K8_NOP8
>> };
>> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
>> +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
>> NULL,
>> k8nops,
>> k8nops + 1,
>> @@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
>> P6_NOP7,
>> P6_NOP8
>> };
>> -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
>> +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = {
>> NULL,
>> p6nops,
>> p6nops + 1,
> Afaict this will cause the same section type conflict issue as did
> the command line parameter constification change that I needed
> to revert yesterday. I'm afraid there's no way around introducing
> a sibling to __initconst (e.g. __initrelro or __initconst_r) specifying
> another section name (e.g. .init.rodata.rel), which then needs to
> be used on data objects incurring relocations. And iirc that was
> also the reason why these annotation have got left off originally
> here.
>
> The issue is that with -fPIC these sections needing relocations
> need to be marked writable even if the objects are "const".
Surely we would be better seeing about fixing the build not to use
-fPIC. Linux doesn't, so it is clearly possible.
~Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 10:10 ` Andrew Cooper
@ 2016-02-23 10:32 ` Jan Beulich
0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2016-02-23 10:32 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 23.02.16 at 11:10, <andrew.cooper3@citrix.com> wrote:
> On 23/02/16 09:47, Jan Beulich wrote:
>>>>> On 22.02.16 at 17:57, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/alternative.c
>>> +++ b/xen/arch/x86/alternative.c
>>> @@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
>>> K8_NOP7,
>>> K8_NOP8
>>> };
>>> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
>>> +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
>>> NULL,
>>> k8nops,
>>> k8nops + 1,
>>> @@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
>>> P6_NOP7,
>>> P6_NOP8
>>> };
>>> -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
>>> +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = {
>>> NULL,
>>> p6nops,
>>> p6nops + 1,
>> Afaict this will cause the same section type conflict issue as did
>> the command line parameter constification change that I needed
>> to revert yesterday. I'm afraid there's no way around introducing
>> a sibling to __initconst (e.g. __initrelro or __initconst_r) specifying
>> another section name (e.g. .init.rodata.rel), which then needs to
>> be used on data objects incurring relocations. And iirc that was
>> also the reason why these annotation have got left off originally
>> here.
>>
>> The issue is that with -fPIC these sections needing relocations
>> need to be marked writable even if the objects are "const".
>
> Surely we would be better seeing about fixing the build not to use
> -fPIC. Linux doesn't, so it is clearly possible.
Linux runs in the top 2Gb of address space, using -mcmodel=kernel.
We can't do that.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 9:47 ` Jan Beulich
2016-02-23 10:10 ` Andrew Cooper
@ 2016-02-23 18:36 ` Andrew Cooper
2016-02-23 18:37 ` Andrew Cooper
` (2 more replies)
1 sibling, 3 replies; 11+ messages in thread
From: Andrew Cooper @ 2016-02-23 18:36 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
these contain no global symbols, they should be .data.rel.ro.local. This
breaks the SPECIAL_DATA_SECTIONS check when converting the transition units to
being init-only.
For alternatives.c, explicitly move the nops arrays into __initconst. For efi
boot.c, manually create the optimisation performed by Clang by collapsing the
switch statement into a lookup table. The double use of const is required to
avoid breaking the ARM build by creating a section type conflict with
fdt_guid.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
v2: Fix ARM build
v3: Introduce and use __initconstrel
---
xen/arch/arm/xen.lds.S | 1 +
xen/arch/x86/alternative.c | 4 ++--
xen/arch/x86/xen.lds.S | 1 +
xen/common/efi/boot.c | 58 +++++++++++++++-------------------------------
xen/include/xen/init.h | 1 +
5 files changed, 24 insertions(+), 41 deletions(-)
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index f501a2f..9f7f915 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -114,6 +114,7 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
.init.data : {
*(.init.rodata)
+ *(.init.rodata.rel)
*(.init.rodata.str*)
*(.init.data)
*(.init.data.rel)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 46ac0fd..9d54df1 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
K8_NOP7,
K8_NOP8
};
-static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
+static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconstrel = {
NULL,
k8nops,
k8nops + 1,
@@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
P6_NOP7,
P6_NOP8
};
-static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
+static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconstrel = {
NULL,
p6nops,
p6nops + 1,
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 9fde1db..b3eb207 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -119,6 +119,7 @@ SECTIONS
} :text
.init.data : {
*(.init.rodata)
+ *(.init.rodata.rel)
*(.init.rodata.str*)
*(.init.data)
*(.init.data.rel)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 53c7452..125c9ce 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -241,53 +241,33 @@ static void __init noreturn blexit(const CHAR16 *str)
/* generic routine for printing error messages */
static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
{
+ static const CHAR16* const ErrCodeToStr[] __initconstrel = {
+ [~EFI_ERROR_MASK & EFI_NOT_FOUND] = L"Not found",
+ [~EFI_ERROR_MASK & EFI_NO_MEDIA] = L"The device has no media",
+ [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED] = L"Media changed",
+ [~EFI_ERROR_MASK & EFI_DEVICE_ERROR] = L"Device error",
+ [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED] = L"Volume corrupted",
+ [~EFI_ERROR_MASK & EFI_ACCESS_DENIED] = L"Access denied",
+ [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES] = L"Out of resources",
+ [~EFI_ERROR_MASK & EFI_VOLUME_FULL] = L"Volume is full",
+ [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION] = L"Security violation",
+ [~EFI_ERROR_MASK & EFI_CRC_ERROR] = L"CRC error",
+ [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA] = L"Compromised data",
+ [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL] = L"Buffer too small",
+ };
+ EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
+
StdOut = StdErr;
PrintErr((CHAR16 *)mesg);
PrintErr(L": ");
- switch (ErrCode)
+ if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
+ mesg = ErrCodeToStr[ErrIdx];
+ else
{
- case EFI_NOT_FOUND:
- mesg = L"Not found";
- break;
- case EFI_NO_MEDIA:
- mesg = L"The device has no media";
- break;
- case EFI_MEDIA_CHANGED:
- mesg = L"Media changed";
- break;
- case EFI_DEVICE_ERROR:
- mesg = L"Device error";
- break;
- case EFI_VOLUME_CORRUPTED:
- mesg = L"Volume corrupted";
- break;
- case EFI_ACCESS_DENIED:
- mesg = L"Access denied";
- break;
- case EFI_OUT_OF_RESOURCES:
- mesg = L"Out of resources";
- break;
- case EFI_VOLUME_FULL:
- mesg = L"Volume is full";
- break;
- case EFI_SECURITY_VIOLATION:
- mesg = L"Security violation";
- break;
- case EFI_CRC_ERROR:
- mesg = L"CRC error";
- break;
- case EFI_COMPROMISED_DATA:
- mesg = L"Compromised data";
- break;
- case EFI_BUFFER_TOO_SMALL:
- mesg = L"Buffer too small";
- break;
- default:
PrintErr(L"ErrCode: ");
DisplayUint(ErrCode, 0);
mesg = NULL;
- break;
}
blexit(mesg);
}
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index fd8b614..671ac81 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -11,6 +11,7 @@
#define __exit __text_section(".exit.text")
#define __initdata __section(".init.data")
#define __initconst __section(".init.rodata")
+#define __initconstrel __section(".init.rodata.rel")
#define __exitdata __used_section(".exit.data")
#define __initsetup __used_section(".init.setup")
#define __init_call(lvl) __used_section(".initcall" lvl ".init")
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 18:36 ` [PATCH v3] " Andrew Cooper
@ 2016-02-23 18:37 ` Andrew Cooper
2016-02-24 9:32 ` Jan Beulich
2016-02-24 9:49 ` Jan Beulich
2016-02-24 13:10 ` Jan Beulich
2 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2016-02-23 18:37 UTC (permalink / raw)
To: Xen-devel; +Cc: Jan Beulich
On 23/02/16 18:36, Andrew Cooper wrote:
> Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
> these contain no global symbols, they should be .data.rel.ro.local. This
> breaks the SPECIAL_DATA_SECTIONS check when converting the transition units to
> being init-only.
>
> For alternatives.c, explicitly move the nops arrays into __initconst. For efi
> boot.c, manually create the optimisation performed by Clang by collapsing the
> switch statement into a lookup table. The double use of const is required to
> avoid breaking the ARM build by creating a section type conflict with
> fdt_guid.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
>
Would you mind testing this on whichever older compiler suffered the
original symptoms? I can't find such a compiler to test with.
~Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 18:37 ` Andrew Cooper
@ 2016-02-24 9:32 ` Jan Beulich
0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2016-02-24 9:32 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 23.02.16 at 19:37, <andrew.cooper3@citrix.com> wrote:
> On 23/02/16 18:36, Andrew Cooper wrote:
>> Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
>> these contain no global symbols, they should be .data.rel.ro.local. This
>> breaks the SPECIAL_DATA_SECTIONS check when converting the transition units to
>> being init-only.
>>
>> For alternatives.c, explicitly move the nops arrays into __initconst. For efi
>> boot.c, manually create the optimisation performed by Clang by collapsing the
>> switch statement into a lookup table. The double use of const is required to
>> avoid breaking the ARM build by creating a section type conflict with
>> fdt_guid.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>>
>
> Would you mind testing this on whichever older compiler suffered the
> original symptoms? I can't find such a compiler to test with.
Sure.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 18:36 ` [PATCH v3] " Andrew Cooper
2016-02-23 18:37 ` Andrew Cooper
@ 2016-02-24 9:49 ` Jan Beulich
2016-02-24 10:23 ` Andrew Cooper
2016-02-24 13:10 ` Jan Beulich
2 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2016-02-24 9:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 23.02.16 at 19:36, <andrew.cooper3@citrix.com> wrote:
> Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
> these contain no global symbols, they should be .data.rel.ro.local. This
> breaks the SPECIAL_DATA_SECTIONS check when converting the transition units
> to
> being init-only.
>
> For alternatives.c, explicitly move the nops arrays into __initconst. For
> efi
> boot.c, manually create the optimisation performed by Clang by collapsing
> the
> switch statement into a lookup table. The double use of const is required
> to
> avoid breaking the ARM build by creating a section type conflict with
> fdt_guid.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Build-tested-by: Jan Beulich <jbeulich@suse.com>
I've also re-worded the second paragraph to refer to the newly
introduced section, and replaced "transition" with "translation" in
the first one. I hope that's okay with you.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-24 9:49 ` Jan Beulich
@ 2016-02-24 10:23 ` Andrew Cooper
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2016-02-24 10:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 24/02/16 09:49, Jan Beulich wrote:
>>>> On 23.02.16 at 19:36, <andrew.cooper3@citrix.com> wrote:
>> Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
>> these contain no global symbols, they should be .data.rel.ro.local. This
>> breaks the SPECIAL_DATA_SECTIONS check when converting the transition units
>> to
>> being init-only.
>>
>> For alternatives.c, explicitly move the nops arrays into __initconst. For
>> efi
>> boot.c, manually create the optimisation performed by Clang by collapsing
>> the
>> switch statement into a lookup table. The double use of const is required
>> to
>> avoid breaking the ARM build by creating a section type conflict with
>> fdt_guid.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Build-tested-by: Jan Beulich <jbeulich@suse.com>
>
> I've also re-worded the second paragraph to refer to the newly
> introduced section, and replaced "transition" with "translation" in
> the first one. I hope that's okay with you.
Yes - fine by me.
~Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-23 18:36 ` [PATCH v3] " Andrew Cooper
2016-02-23 18:37 ` Andrew Cooper
2016-02-24 9:49 ` Jan Beulich
@ 2016-02-24 13:10 ` Jan Beulich
2016-02-24 14:59 ` Stefano Stabellini
2 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2016-02-24 13:10 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Stefano Stabellini
>>> On 23.02.16 at 19:36, <andrew.cooper3@citrix.com> wrote:
> Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
> these contain no global symbols, they should be .data.rel.ro.local. This
> breaks the SPECIAL_DATA_SECTIONS check when converting the transition units
> to
> being init-only.
>
> For alternatives.c, explicitly move the nops arrays into __initconst. For
> efi
> boot.c, manually create the optimisation performed by Clang by collapsing
> the
> switch statement into a lookup table. The double use of const is required
> to
> avoid breaking the ARM build by creating a section type conflict with
> fdt_guid.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> v2: Fix ARM build
> v3: Introduce and use __initconstrel
> ---
> xen/arch/arm/xen.lds.S | 1 +
> xen/arch/x86/alternative.c | 4 ++--
> xen/arch/x86/xen.lds.S | 1 +
> xen/common/efi/boot.c | 58 +++++++++++++++-------------------------------
> xen/include/xen/init.h | 1 +
> 5 files changed, 24 insertions(+), 41 deletions(-)
>
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index f501a2f..9f7f915 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -114,6 +114,7 @@ SECTIONS
> . = ALIGN(PAGE_SIZE);
> .init.data : {
> *(.init.rodata)
> + *(.init.rodata.rel)
> *(.init.rodata.str*)
> *(.init.data)
> *(.init.data.rel)
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 46ac0fd..9d54df1 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
> K8_NOP7,
> K8_NOP8
> };
> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
> +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconstrel =
> {
> NULL,
> k8nops,
> k8nops + 1,
> @@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
> P6_NOP7,
> P6_NOP8
> };
> -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
> +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconstrel =
> {
> NULL,
> p6nops,
> p6nops + 1,
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 9fde1db..b3eb207 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -119,6 +119,7 @@ SECTIONS
> } :text
> .init.data : {
> *(.init.rodata)
> + *(.init.rodata.rel)
> *(.init.rodata.str*)
> *(.init.data)
> *(.init.data.rel)
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 53c7452..125c9ce 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -241,53 +241,33 @@ static void __init noreturn blexit(const CHAR16 *str)
> /* generic routine for printing error messages */
> static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
> {
> + static const CHAR16* const ErrCodeToStr[] __initconstrel = {
> + [~EFI_ERROR_MASK & EFI_NOT_FOUND] = L"Not found",
> + [~EFI_ERROR_MASK & EFI_NO_MEDIA] = L"The device has no
> media",
> + [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED] = L"Media changed",
> + [~EFI_ERROR_MASK & EFI_DEVICE_ERROR] = L"Device error",
> + [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED] = L"Volume corrupted",
> + [~EFI_ERROR_MASK & EFI_ACCESS_DENIED] = L"Access denied",
> + [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES] = L"Out of resources",
> + [~EFI_ERROR_MASK & EFI_VOLUME_FULL] = L"Volume is full",
> + [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION] = L"Security violation",
> + [~EFI_ERROR_MASK & EFI_CRC_ERROR] = L"CRC error",
> + [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA] = L"Compromised data",
> + [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL] = L"Buffer too small",
> + };
> + EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
> +
> StdOut = StdErr;
> PrintErr((CHAR16 *)mesg);
> PrintErr(L": ");
>
> - switch (ErrCode)
> + if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
> + mesg = ErrCodeToStr[ErrIdx];
> + else
> {
> - case EFI_NOT_FOUND:
> - mesg = L"Not found";
> - break;
> - case EFI_NO_MEDIA:
> - mesg = L"The device has no media";
> - break;
> - case EFI_MEDIA_CHANGED:
> - mesg = L"Media changed";
> - break;
> - case EFI_DEVICE_ERROR:
> - mesg = L"Device error";
> - break;
> - case EFI_VOLUME_CORRUPTED:
> - mesg = L"Volume corrupted";
> - break;
> - case EFI_ACCESS_DENIED:
> - mesg = L"Access denied";
> - break;
> - case EFI_OUT_OF_RESOURCES:
> - mesg = L"Out of resources";
> - break;
> - case EFI_VOLUME_FULL:
> - mesg = L"Volume is full";
> - break;
> - case EFI_SECURITY_VIOLATION:
> - mesg = L"Security violation";
> - break;
> - case EFI_CRC_ERROR:
> - mesg = L"CRC error";
> - break;
> - case EFI_COMPROMISED_DATA:
> - mesg = L"Compromised data";
> - break;
> - case EFI_BUFFER_TOO_SMALL:
> - mesg = L"Buffer too small";
> - break;
> - default:
> PrintErr(L"ErrCode: ");
> DisplayUint(ErrCode, 0);
> mesg = NULL;
> - break;
> }
> blexit(mesg);
> }
> diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
> index fd8b614..671ac81 100644
> --- a/xen/include/xen/init.h
> +++ b/xen/include/xen/init.h
> @@ -11,6 +11,7 @@
> #define __exit __text_section(".exit.text")
> #define __initdata __section(".init.data")
> #define __initconst __section(".init.rodata")
> +#define __initconstrel __section(".init.rodata.rel")
> #define __exitdata __used_section(".exit.data")
> #define __initsetup __used_section(".init.setup")
> #define __init_call(lvl) __used_section(".initcall" lvl ".init")
> --
> 2.1.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] xen: Work around Clang generating .data.rel.ro section for init-only files
2016-02-24 13:10 ` Jan Beulich
@ 2016-02-24 14:59 ` Stefano Stabellini
0 siblings, 0 replies; 11+ messages in thread
From: Stefano Stabellini @ 2016-02-24 14:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Xen-devel, Stefano Stabellini
On Wed, 24 Feb 2016, Jan Beulich wrote:
> >>> On 23.02.16 at 19:36, <andrew.cooper3@citrix.com> wrote:
> > Clang-3.8 generates several .data.rel.ro sections when compiling Xen. As
> > these contain no global symbols, they should be .data.rel.ro.local. This
> > breaks the SPECIAL_DATA_SECTIONS check when converting the transition units
> > to
> > being init-only.
> >
> > For alternatives.c, explicitly move the nops arrays into __initconst. For
> > efi
> > boot.c, manually create the optimisation performed by Clang by collapsing
> > the
> > switch statement into a lookup table. The double use of const is required
> > to
> > avoid breaking the ARM build by creating a section type conflict with
> > fdt_guid.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > v2: Fix ARM build
> > v3: Introduce and use __initconstrel
> > ---
> > xen/arch/arm/xen.lds.S | 1 +
> > xen/arch/x86/alternative.c | 4 ++--
> > xen/arch/x86/xen.lds.S | 1 +
> > xen/common/efi/boot.c | 58 +++++++++++++++-------------------------------
> > xen/include/xen/init.h | 1 +
> > 5 files changed, 24 insertions(+), 41 deletions(-)
> >
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index f501a2f..9f7f915 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -114,6 +114,7 @@ SECTIONS
> > . = ALIGN(PAGE_SIZE);
> > .init.data : {
> > *(.init.rodata)
> > + *(.init.rodata.rel)
> > *(.init.rodata.str*)
> > *(.init.data)
> > *(.init.data.rel)
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > index 46ac0fd..9d54df1 100644
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
> > K8_NOP7,
> > K8_NOP8
> > };
> > -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
> > +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconstrel =
> > {
> > NULL,
> > k8nops,
> > k8nops + 1,
> > @@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = {
> > P6_NOP7,
> > P6_NOP8
> > };
> > -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = {
> > +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconstrel =
> > {
> > NULL,
> > p6nops,
> > p6nops + 1,
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 9fde1db..b3eb207 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -119,6 +119,7 @@ SECTIONS
> > } :text
> > .init.data : {
> > *(.init.rodata)
> > + *(.init.rodata.rel)
> > *(.init.rodata.str*)
> > *(.init.data)
> > *(.init.data.rel)
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index 53c7452..125c9ce 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -241,53 +241,33 @@ static void __init noreturn blexit(const CHAR16 *str)
> > /* generic routine for printing error messages */
> > static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
> > {
> > + static const CHAR16* const ErrCodeToStr[] __initconstrel = {
> > + [~EFI_ERROR_MASK & EFI_NOT_FOUND] = L"Not found",
> > + [~EFI_ERROR_MASK & EFI_NO_MEDIA] = L"The device has no
> > media",
> > + [~EFI_ERROR_MASK & EFI_MEDIA_CHANGED] = L"Media changed",
> > + [~EFI_ERROR_MASK & EFI_DEVICE_ERROR] = L"Device error",
> > + [~EFI_ERROR_MASK & EFI_VOLUME_CORRUPTED] = L"Volume corrupted",
> > + [~EFI_ERROR_MASK & EFI_ACCESS_DENIED] = L"Access denied",
> > + [~EFI_ERROR_MASK & EFI_OUT_OF_RESOURCES] = L"Out of resources",
> > + [~EFI_ERROR_MASK & EFI_VOLUME_FULL] = L"Volume is full",
> > + [~EFI_ERROR_MASK & EFI_SECURITY_VIOLATION] = L"Security violation",
> > + [~EFI_ERROR_MASK & EFI_CRC_ERROR] = L"CRC error",
> > + [~EFI_ERROR_MASK & EFI_COMPROMISED_DATA] = L"Compromised data",
> > + [~EFI_ERROR_MASK & EFI_BUFFER_TOO_SMALL] = L"Buffer too small",
> > + };
> > + EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
> > +
> > StdOut = StdErr;
> > PrintErr((CHAR16 *)mesg);
> > PrintErr(L": ");
> >
> > - switch (ErrCode)
> > + if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
> > + mesg = ErrCodeToStr[ErrIdx];
> > + else
> > {
> > - case EFI_NOT_FOUND:
> > - mesg = L"Not found";
> > - break;
> > - case EFI_NO_MEDIA:
> > - mesg = L"The device has no media";
> > - break;
> > - case EFI_MEDIA_CHANGED:
> > - mesg = L"Media changed";
> > - break;
> > - case EFI_DEVICE_ERROR:
> > - mesg = L"Device error";
> > - break;
> > - case EFI_VOLUME_CORRUPTED:
> > - mesg = L"Volume corrupted";
> > - break;
> > - case EFI_ACCESS_DENIED:
> > - mesg = L"Access denied";
> > - break;
> > - case EFI_OUT_OF_RESOURCES:
> > - mesg = L"Out of resources";
> > - break;
> > - case EFI_VOLUME_FULL:
> > - mesg = L"Volume is full";
> > - break;
> > - case EFI_SECURITY_VIOLATION:
> > - mesg = L"Security violation";
> > - break;
> > - case EFI_CRC_ERROR:
> > - mesg = L"CRC error";
> > - break;
> > - case EFI_COMPROMISED_DATA:
> > - mesg = L"Compromised data";
> > - break;
> > - case EFI_BUFFER_TOO_SMALL:
> > - mesg = L"Buffer too small";
> > - break;
> > - default:
> > PrintErr(L"ErrCode: ");
> > DisplayUint(ErrCode, 0);
> > mesg = NULL;
> > - break;
> > }
> > blexit(mesg);
> > }
> > diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
> > index fd8b614..671ac81 100644
> > --- a/xen/include/xen/init.h
> > +++ b/xen/include/xen/init.h
> > @@ -11,6 +11,7 @@
> > #define __exit __text_section(".exit.text")
> > #define __initdata __section(".init.data")
> > #define __initconst __section(".init.rodata")
> > +#define __initconstrel __section(".init.rodata.rel")
> > #define __exitdata __used_section(".exit.data")
> > #define __initsetup __used_section(".init.setup")
> > #define __init_call(lvl) __used_section(".initcall" lvl ".init")
> > --
> > 2.1.4
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-02-24 14:59 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-22 16:57 [PATCH] xen: Work around Clang generating .data.rel.ro section for init-only files Andrew Cooper
2016-02-23 9:47 ` Jan Beulich
2016-02-23 10:10 ` Andrew Cooper
2016-02-23 10:32 ` Jan Beulich
2016-02-23 18:36 ` [PATCH v3] " Andrew Cooper
2016-02-23 18:37 ` Andrew Cooper
2016-02-24 9:32 ` Jan Beulich
2016-02-24 9:49 ` Jan Beulich
2016-02-24 10:23 ` Andrew Cooper
2016-02-24 13:10 ` Jan Beulich
2016-02-24 14:59 ` Stefano Stabellini
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.