MIME-Version: 1.0
In-Reply-To: <4284ee79-a87c-a47d-7e96-682bdd8754e1@cn.fujitsu.com>
References: <20180115202335.1645883-1-arnd@arndb.de> <afce4e9a-bb5d-6a53-bef7-1f7804785eb2@cn.fujitsu.com>
 <4284ee79-a87c-a47d-7e96-682bdd8754e1@cn.fujitsu.com>
From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 16 Jan 2018 08:21:20 +0100
Message-ID: <CAK8P3a2gnKcFmnzfKG=DTLh2UQ8bYa7F59hPqEgY77bYELw85g@mail.gmail.com>
Subject: Re: [PATCH] x86/jailhouse: fix building without X86_X2APIC
To: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        jailhouse-dev@googlegroups.com, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Juergen Gross <jgross@suse.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org

On Tue, Jan 16, 2018 at 3:34 AM, Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:
> At 01/16/2018 09:25 AM, Dou Liyang wrote:

>>> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
>>> index 98722773391d..0317d635d9ba 100644
>>> --- a/arch/x86/include/asm/apic.h
>>> +++ b/arch/x86/include/asm/apic.h
>>> @@ -188,6 +188,8 @@ static inline void lapic_assign_system_vectors(void)
>>> { }
>>>   static inline void lapic_assign_legacy_vector(unsigned int i, bool r) {
>>> }
>>>   #endif /* !CONFIG_X86_LOCAL_APIC */
>>> +extern int x2apic_mode;
>>> +extern int x2apic_phys;
>>
>> We can't do that, adding a macro for the X2APIC=n case is enough
>>
> I am sorry when I looked into your code in tip tree. I found this
> measure is not true. please try the the following v2 patch.
>
> 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
those two variables, maybe make them 'static' and remove the extern
declarations?

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

      Arnd