linux-kernel - Re: [PATCH] x86/jailhouse: fix building without X86

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Jan 2018 16:17:42 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Jan Kiszka <jan.kiszka@...mens.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        the arch/x86 maintainers <x86@...nel.org>,
        <jailhouse-dev@...glegroups.com>, Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Juergen Gross <jgross@...e.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/jailhouse: fix building without X86_X2APIC

Hi Arnd,

[...]
>> The reason I don't want to expose the x2apic_mode and x2apic_phys is
>> that they may be misused in X2APIC=n case. So I create an interface to
>> wrap it. do you think so? ;-)
> 
> I'm not sure I follow what the intention of that is. If you want to hide

My purpose of that is hiding the variables in X2APIC=n case.

> those two variables, maybe make them 'static' and remove the extern
> declarations?
> 

Yes, that's what I really want, But due to the simple "x2apic_phys= 1"
operation, doing that may be too fat, I guess.

>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>> index 98722773391d..ac25ac2e49af 100644
>> --- a/arch/x86/include/asm/apic.h
>> +++ b/arch/x86/include/asm/apic.h
>> @@ -251,6 +251,11 @@ static inline u64 native_x2apic_icr_read(void)
>>
>>   extern int x2apic_mode;
>>   extern int x2apic_phys;
>> +static inline void apic_set_x2apic_phys(void)
>> +{
>> +       x2apic_phys = 1;
>> +}
>> +
>>   extern void __init check_x2apic(void);
>>   extern void x2apic_setup(void);
>>   static inline int x2apic_enabled(void)
>> @@ -265,7 +270,10 @@ static inline void x2apic_setup(void) { }
>>   static inline int x2apic_enabled(void) { return 0; }
>>
>>   #define x2apic_mode            (0)
>> -#define        x2apic_supported()      (0)
>> +#define x2apic_phys            (0)
>> +#define x2apic_supported()     (0)
>> +
>> +static inline void apic_set_x2apic_phys(void){}
>>   #endif /* !CONFIG_X86_X2APIC */
>>
>>   struct irq_data;
> 
> I see nothing wrong it with this, but also don't see anything it does
> that improves the interface.
> 

Another way we can choice is wrap the code with "CONFIG_X86_X2APIC".

Thanks,
	dou
--------------------------------8<------------------------------------
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index d6d5976a9b51..d4aee43c8912 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -65,6 +65,7 @@ static void __init jailhouse_get_smp_config(unsigned 
int early)
         };
         unsigned int cpu;

+#ifdef CONFIG_X86_X2APIC
         if (x2apic_enabled()) {
                 /*
                  * We do not have access to IR inside Jailhouse 
non-root cells.
@@ -79,6 +80,7 @@ static void __init jailhouse_get_smp_config(unsigned 
int early)
                  */
                 default_acpi_madt_oem_check("", "");
         }
+#endif

         register_lapic_address(0xfee00000);

>        Arnd
> 
> 
>