Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc:     Baoquan He <bhe@redhat.com>, <linux-kernel@vger.kernel.org>,
        <mingo@kernel.org>, <tglx@linutronix.de>, <x86@kernel.org>,
        <joro@8bytes.org>, <uobergfe@redhat.com>, <prarit@redhat.com>
References: <20180209121008.28980-1-bhe@redhat.com>
        <caff6f56-d8e3-1c61-e23b-1d335b6771a2@cn.fujitsu.com>
Date:   Sun, 11 Feb 2018 23:11:41 -0600
In-Reply-To: <caff6f56-d8e3-1c61-e23b-1d335b6771a2@cn.fujitsu.com> (Dou
        Liyang's message of "Mon, 12 Feb 2018 12:06:10 +0800")
Message-ID: <87wozi7pwy.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v3 0/5] x86/apic: Fix restoring boot irq mode in reboot and kexec/kdump
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Dou Liyang <douly.fnst@cn.fujitsu.com> writes:

> Hi all,
>
> One thing confused me.
>
> The disconnect_bsp_APIC() may restore the interrupt delivery mode into
> virtual wire mode. it uses the vector F as the spurious interrput, But,
> IMO, using the vector 0xFF(SPURIOUS_APIC_VECTOR) may more suitable and
> will give us more detail. Why the disconnect_bsp_APIC() use vector F
> here?

I would say this needs a documentation search before changing this.

This code originates in:
208fb93162d5 ("[PATCH] kexec: x86_64: restore apic virtual wire mode on shutdown")

The example in the Multi-Processor Specification v1.4 shows setting
up the SPIV to vector 0x0f.

I don't know what is canonical and what will interact best with DOS,
and that erra of setup.  The vector 0x0f seems an odd choice as
it is below 0x20 putting it in the range of vectors that are
reserved for processor exceptions.

The constant SPURIOUS_APIC_VECTOR is definitely not something we want
to be using at this point as that is a linux specific setting and used
when Linux is up and running.  So it is completely inapplicable.

This is all about restoring how the apics were configured at boot time
so it may be appropriate to copy and store this value, if it was not
architectural.

At a practical level at this point I suspect we are ok as the setting
of the SPIV this way has not caused any known problems in the last
decade.   If someone wants to dig through the archtectural documents
and the real world practice and find a better value and explain the
change I would not oppose it.

All I know for certain is using the constant SPURIOUS_APIC_VECTOR
is completely inappropriate (as that constant is about how linux uses
vectors) and thus the patch below is wrong.

> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 25ddf02598d2..550deaad6a9a 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2130,7 +2130,7 @@ void disconnect_bsp_APIC(int virt_wire_setup)
>         value = apic_read(APIC_SPIV);
>         value &= ~APIC_VECTOR_MASK;
>         value |= APIC_SPIV_APIC_ENABLED;
> -       value |= 0xf;
> +       value |= SPURIOUS_APIC_VECTOR;
>         apic_write(APIC_SPIV, value);
>
>         if (!virt_wire_setup) {
>

Eric