Received-SPF: pass (google.com: domain of linux-kernel+bounces-43308-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Michael Kelley <mhklinux@outlook.com>
To: Baoquan He <bhe@redhat.com>, "kexec@lists.infradead.org"
	<kexec@lists.infradead.org>, "akpm@linux-foundation.org"
	<akpm@linux-foundation.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>, "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linuxppc-dev@lists.ozlabs.org"
	<linuxppc-dev@lists.ozlabs.org>, "linux-s390@vger.kernel.org"
	<linux-s390@vger.kernel.org>, "nathan@kernel.org" <nathan@kernel.org>
Subject: RE: [PATCH linux-next 1/3] x86, crash: don't nest CONFIG_CRASH_DUMP
 ifdef inside CONFIG_KEXEC_CODE ifdef scope
Thread-Topic: [PATCH linux-next 1/3] x86, crash: don't nest CONFIG_CRASH_DUMP
 ifdef inside CONFIG_KEXEC_CODE ifdef scope
Thread-Index: AQHaUronjJlmGLvjNUqyTXz++ApJcbDxGaMg
Date: Mon, 29 Jan 2024 18:27:43 +0000
Message-ID:
 <SN6PR02MB41571397201804BD486C6148D47E2@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240129135033.157195-1-bhe@redhat.com>
In-Reply-To: <20240129135033.157195-1-bhe@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?dIeZuifxIMxxVG54PLpVHugirvrswqLMSCv44bBBUL5Bv2JgEs3id+qJdS1G?=
 =?us-ascii?Q?hzEkkrrRtz3/8JOA9huQMet/glgQYx8bXH3HB/59xBlTmZcotMGCMd4vo1MA?=
 =?us-ascii?Q?ypgGtCkuoJPRWM8BiE3iYQocw9HfGEYQ6T5BOlr54y0/7cpSxmwL/eBL/DHS?=
 =?us-ascii?Q?5XE9Nw8v/Azl7hNNQn6eyN79DBnR097mmtt/sf2lS9S1diQXtwSH7DxN3o6R?=
 =?us-ascii?Q?a5wKHM/ArpeFpyB63FOP2Iw+kqhA6OaJxZQUBvtCumrkAIsu0hwp2LgyCi9m?=
 =?us-ascii?Q?0+W/Vw3ZaVngbNHOOyibUuR2n98Wzq2pfBoPMk/1RXBNVUBFW9E5x7mJDsPO?=
 =?us-ascii?Q?f9oJcGdYenfj1JtkGPe5h5AkM8Og50OjS90LEN+OhiRjLxzgANfbcyzHzqu0?=
 =?us-ascii?Q?IbCSwrAtDCrdEYDTh41ruLvsTWhwudVn/qKiTlzlxpke3ewDb+Fso7m4At0h?=
 =?us-ascii?Q?B+m8Ztac8FZz9TBiMKQI5jycKY1jjlEI00IQ7AT1GD2TjCAqmPLPwAmWSfzT?=
 =?us-ascii?Q?NLKWwJe1t9FkRKktqr52qwgTrR3VwiXcJ288eV5kOKgvGRJX/SviMLTci3S5?=
 =?us-ascii?Q?E9+8vAXtN/f8xpbJU42qcx4KLq/GINHUMmRmmzIZBLY45Or0Gl7+iohIKKmL?=
 =?us-ascii?Q?JNRqDT5zINF77PhqD5RUfgVJsMwLQ2n4DMyE5ToPj04rqrvfaqksC2sj2LSn?=
 =?us-ascii?Q?/2pRg7LbU7rWgmC3BVdgqUnOh9+Ce8ojjkqfLdDhGznLPxZS/LjrJQi3koZB?=
 =?us-ascii?Q?UPUKH6lZyxi8tXbUcCzx7TmqsD46cBJOJyMJeOT+QVo5X2TrkYD5fnC9mfJL?=
 =?us-ascii?Q?/3EL7PIeefCBgERkpunJp3+uQbiYP89GhE+7q0kTcd81NMQmPMjwBXi1Fd4s?=
 =?us-ascii?Q?2s19Df3rYibJoQZBp9IkzwVOQkyQlgzCXtyGaBpieVf+osBTjE9vzmPvzmJk?=
 =?us-ascii?Q?YemM+PWjoDARs0F4yRDzyBeS4EFqXy7IJoQ6BdcnwOJCW8ry9m1tP1SOJHRC?=
 =?us-ascii?Q?Q+nuprDkqSA1VtGHttooALZioTmlDHcEQkh0BhrO35e4Np60P38XXUJuABnA?=
 =?us-ascii?Q?ytkK2QycQRYBIcbxWYjFc9l8fX+a7XWxpRWS4EouCLlXz4YgBVeFhauGRctq?=
 =?us-ascii?Q?8YZphCQOciv4E8nupslPpDnUUcu96DjPMY2Nfd5BCxReCTBD5kLNcpD4T2Xm?=
 =?us-ascii?Q?vVJWpdfyCFnCxH4KpbetqvF3AYgt1/b+xaoRAQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4157.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-Network-Message-Id: 1f0f8b42-44a2-4e82-cc0f-08dc20f7fc2e
X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jan 2024 18:27:43.9438
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR02MB9341

From: Baoquan He <bhe@redhat.com> Sent: Monday, January 29, 2024 5:51 AM
>=20
> Michael pointed out that the #ifdef CONFIG_CRASH_DUMP is nested inside
> arch/x86/xen/enlighten_hvm.c.

Did some words get left out in the above sentence?  It mentions the Xen
case, but not the Hyper-V case.  I'm not sure what you intended.

>=20
> Although the nesting works well too since CONFIG_CRASH_DUMP has
> dependency on CONFIG_KEXEC_CORE, it may cause confuse because there

s/confusion/confuse/

> are places where it's not nested, and people may think it need be nested

s/need be/needs to be/

> even though it doesn't have to.
>=20
> Fix that by moving  CONFIG_CRASH_DUMP ifdeffery of codes out of
> CONFIG_KEXEC_CODE ifdeffery scope.
>=20
> And also fix a building error Nathan reported as below by replacing
> CONFIG_KEXEC_CORE ifdef with CONFIG_VMCORE_INFO ifdef.
>=20
> =3D=3D=3D=3D
> $ curl -LSso .config https://git.alpinelinux.org/aports/plain/community/l=
inux-edge/config-edge.x86_64=20
> $ make -skj"$(nproc)" ARCH=3Dx86_64 CROSS_COMPILE=3Dx86_64-linux-
> olddefconfig all
> ...
> x86_64-linux-ld: arch/x86/xen/mmu_pv.o: in function
> `paddr_vmcoreinfo_note':
> mmu_pv.c:(.text+0x3af3): undefined reference to `vmcoreinfo_note'
> =3D=3D=3D=3D
>=20
> Link: https://lore.kernel.org/all/SN6PR02MB4157931105FA68D72E3D3DB8D47B2@=
SN6PR02MB4157.namprd02.prod.outlook.com/T/#u
> Link: https://lore.kernel.org/all/20240126045551.GA126645@dev-arch.thelio=
-3990X/T/#u
> Signed-off-by: Baoquan He <bhe@redhat.com>

Modulo the commit message nits, LGTM.

Reviewed-by: Michael Kelley <mhklinux@outlook.com>

> ---
>  arch/x86/kernel/cpu/mshyperv.c | 10 ++++++----
>  arch/x86/kernel/reboot.c       |  2 +-
>  arch/x86/xen/enlighten_hvm.c   |  4 ++--
>  arch/x86/xen/mmu_pv.c          |  2 +-
>  4 files changed, 10 insertions(+), 8 deletions(-)
>=20
> diff --git a/arch/x86/kernel/cpu/mshyperv.c
> b/arch/x86/kernel/cpu/mshyperv.c
> index f8163a59026b..2e8cd5a4ae85 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -209,6 +209,7 @@ static void hv_machine_shutdown(void)
>  	if (kexec_in_progress)
>  		hyperv_cleanup();
>  }
> +#endif /* CONFIG_KEXEC_CORE */
>=20
>  #ifdef CONFIG_CRASH_DUMP
>  static void hv_machine_crash_shutdown(struct pt_regs *regs)
> @@ -222,8 +223,7 @@ static void hv_machine_crash_shutdown(struct
> pt_regs *regs)
>  	/* Disable the hypercall page when there is only 1 active CPU. */
>  	hyperv_cleanup();
>  }
> -#endif
> -#endif /* CONFIG_KEXEC_CORE */
> +#endif /* CONFIG_CRASH_DUMP */
>  #endif /* CONFIG_HYPERV */
>=20
>  static uint32_t  __init ms_hyperv_platform(void)
> @@ -497,9 +497,11 @@ static void __init ms_hyperv_init_platform(void)
>  	no_timer_check =3D 1;
>  #endif
>=20
> -#if IS_ENABLED(CONFIG_HYPERV) && defined(CONFIG_KEXEC_CORE)
> +#if IS_ENABLED(CONFIG_HYPERV)
> +#if defined(CONFIG_KEXEC_CORE)
>  	machine_ops.shutdown =3D hv_machine_shutdown;
> -#ifdef CONFIG_CRASH_DUMP
> +#endif
> +#if defined(CONFIG_CRASH_DUMP)
>  	machine_ops.crash_shutdown =3D hv_machine_crash_shutdown;
>  #endif
>  #endif
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 1287b0d5962f..f3130f762784 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -826,7 +826,7 @@ void machine_halt(void)
>  	machine_ops.halt();
>  }
>=20
> -#ifdef CONFIG_KEXEC_CORE
> +#ifdef CONFIG_CRASH_DUMP
>  void machine_crash_shutdown(struct pt_regs *regs)
>  {
>  	machine_ops.crash_shutdown(regs);
> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> index 09e3db7ff990..0b367c1e086d 100644
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -148,6 +148,7 @@ static void xen_hvm_shutdown(void)
>  	if (kexec_in_progress)
>  		xen_reboot(SHUTDOWN_soft_reset);
>  }
> +#endif
>=20
>  #ifdef CONFIG_CRASH_DUMP
>  static void xen_hvm_crash_shutdown(struct pt_regs *regs)
> @@ -156,7 +157,6 @@ static void xen_hvm_crash_shutdown(struct pt_regs
> *regs)
>  	xen_reboot(SHUTDOWN_soft_reset);
>  }
>  #endif
> -#endif
>=20
>  static int xen_cpu_up_prepare_hvm(unsigned int cpu)
>  {
> @@ -238,10 +238,10 @@ static void __init xen_hvm_guest_init(void)
>=20
>  #ifdef CONFIG_KEXEC_CORE
>  	machine_ops.shutdown =3D xen_hvm_shutdown;
> +#endif
>  #ifdef CONFIG_CRASH_DUMP
>  	machine_ops.crash_shutdown =3D xen_hvm_crash_shutdown;
>  #endif
> -#endif
>  }
>=20
>  static __init int xen_parse_nopv(char *arg)
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 218773cfb009..e21974f2cf2d 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -2520,7 +2520,7 @@ int xen_remap_pfn(struct vm_area_struct *vma,
> unsigned long addr,
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_pfn);
>=20
> -#ifdef CONFIG_KEXEC_CORE
> +#ifdef CONFIG_VMCORE_INFO
>  phys_addr_t paddr_vmcoreinfo_note(void)
>  {
>  	if (xen_pv_domain())
> --
> 2.41.0