[PATCH] x86/pv: Fix Clang build with !CONFIG_PV32

[PATCH] x86/pv: Fix Clang build with !CONFIG_PV32

[Andrew Cooper]

Clang 3.5 doesn't do enough dead-code-elimination to drop the compat_gdt
reference, resulting in a linker failure:

  hidden symbol `per_cpu__compat_gdt' isn't defined

Drop the local variable, and move evaluation of this_cpu(compat_gdt) to within
the guarded region.

Reported-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/common.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 131ff03fcf..63f3893c7a 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -711,8 +711,6 @@ void load_system_tables(void)
 	struct tss64 *tss = &this_cpu(tss_page).tss;
 	seg_desc_t *gdt =
 		this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY;
-	seg_desc_t *compat_gdt =
-		this_cpu(compat_gdt) - FIRST_RESERVED_GDT_ENTRY;
 
 	const struct desc_ptr gdtr = {
 		.base = (unsigned long)gdt,
@@ -753,8 +751,9 @@ void load_system_tables(void)
 	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
 			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
 	if ( IS_ENABLED(CONFIG_PV32) )
-		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
-				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
+		_set_tssldt_desc(
+			this_cpu(compat_gdt) - FIRST_RESERVED_GDT_ENTRY + TSS_ENTRY,
+			(unsigned long)tss, sizeof(*tss) - 1, SYS_DESC_tss_busy);
 
 	per_cpu(full_gdt_loaded, cpu) = false;
 	lgdt(&gdtr);
-- 
2.11.0

Re: [PATCH] x86/pv: Fix Clang build with !CONFIG_PV32

[Roger Pau Monné]

On Tue, May 05, 2020 at 03:28:10PM +0100, Andrew Cooper wrote:
> Clang 3.5 doesn't do enough dead-code-elimination to drop the compat_gdt
> reference, resulting in a linker failure:
> 
>   hidden symbol `per_cpu__compat_gdt' isn't defined
> 
> Drop the local variable, and move evaluation of this_cpu(compat_gdt) to within
> the guarded region.
> 
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Tested-and-reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks!

Re: [PATCH] x86/pv: Fix Clang build with !CONFIG_PV32

[Jan Beulich]

On 05.05.2020 16:28, Andrew Cooper wrote:
> @@ -753,8 +751,9 @@ void load_system_tables(void)
>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>  	if ( IS_ENABLED(CONFIG_PV32) )
> -		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
> -				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
> +		_set_tssldt_desc(
> +			this_cpu(compat_gdt) - FIRST_RESERVED_GDT_ENTRY + TSS_ENTRY,
> +			(unsigned long)tss, sizeof(*tss) - 1, SYS_DESC_tss_busy);

Isn't indentation here off by 4 compared to what we
normally do with extremely large argument expressions?
Other than this lgtm.

Jan

Re: [PATCH] x86/pv: Fix Clang build with !CONFIG_PV32

[Andrew Cooper]

On 05/05/2020 15:52, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> On 05.05.2020 16:28, Andrew Cooper wrote:
>> @@ -753,8 +751,9 @@ void load_system_tables(void)
>>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>>  	if ( IS_ENABLED(CONFIG_PV32) )
>> -		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>> -				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>> +		_set_tssldt_desc(
>> +			this_cpu(compat_gdt) - FIRST_RESERVED_GDT_ENTRY + TSS_ENTRY,
>> +			(unsigned long)tss, sizeof(*tss) - 1, SYS_DESC_tss_busy);
> Isn't indentation here off by 4 compared to what we
> normally do with extremely large argument expressions?

No.  This is Linux style (therefore 8-space tabs), not Xen style (4 spaces).

~Andrew

Re: [PATCH] x86/pv: Fix Clang build with !CONFIG_PV32

[Jan Beulich]

On 05.05.2020 17:05, Andrew Cooper wrote:
> On 05/05/2020 15:52, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> On 05.05.2020 16:28, Andrew Cooper wrote:
>>> @@ -753,8 +751,9 @@ void load_system_tables(void)
>>>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>>>  	if ( IS_ENABLED(CONFIG_PV32) )
>>> -		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>> -				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>>> +		_set_tssldt_desc(
>>> +			this_cpu(compat_gdt) - FIRST_RESERVED_GDT_ENTRY + TSS_ENTRY,
>>> +			(unsigned long)tss, sizeof(*tss) - 1, SYS_DESC_tss_busy);
>> Isn't indentation here off by 4 compared to what we
>> normally do with extremely large argument expressions?
> 
> No.  This is Linux style (therefore 8-space tabs), not Xen style (4 spaces).

Oh, right - din't pay attention at all to this being tabs, sorry.

Jan