[PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Saurabh Sengar]

X86_MPPARSE is only selectable when ACPI is enabled. However,
on Devicetree platforms where ACPI is disabled, it is always
enabled. Allow X86_MPPARSE to be selected by OF platforms as
well.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 arch/x86/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 53bab123a8ee..e60315397399 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -469,7 +469,7 @@ config X86_X2APIC
 	  If you don't know what to do here, say N.
 
 config X86_MPPARSE
-	bool "Enable MPS table" if ACPI
+	bool "Enable MPS table" if ACPI || OF
 	default y
 	depends on X86_LOCAL_APIC
 	help
-- 
2.34.1

[PATCH 2/2] Drivers: hv: Kconfig: Add HYPERV_VTL_MODE dependencies

[Saurabh Sengar]

MTRRs are not per-VTL and are controlled by VTL0, make sure it
is disabled for all other VTLs.

X86_MPPARSE scans low memory for MP tables, which is not required
for Hyper-V VTL platforms.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 drivers/hv/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 00242107d62e..c8d443886b9d 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -16,6 +16,8 @@ config HYPERV
 config HYPERV_VTL_MODE
 	bool "Enable Linux to boot in VTL context"
 	depends on X86_64 && HYPERV
+	depends on !MTRR
+	depends on !X86_MPPARSE
 	default n
 	help
 	  Virtual Secure Mode (VSM) is a set of hypervisor capabilities and
-- 
2.34.1

RE: [PATCH 2/2] Drivers: hv: Kconfig: Add HYPERV_VTL_MODE dependencies

[Michael Kelley (LINUX)]

From: Saurabh Sengar <ssengar@linux.microsoft.com>
> 
> MTRRs are not per-VTL and are controlled by VTL0, make sure it
> is disabled for all other VTLs.

Clarifying the wording a bit:

MTRRs are not per-VTL, but are always controlled by VTL0.   Allow
building for VTLs other than VTL0 only when MTRR functionality
is disabled.

> 
> X86_MPPARSE scans low memory for MP tables, which is not required
> for Hyper-V VTL platforms.

Is the above statement saying that MP tables aren't required, so remove
the code to save space?  Or is there a problem/failure if MP table code
runs and scans low memory?   *Requiring* that it be disabled, and the
reference to scanning low memory makes me think it might be the latter,
in which case I'd suggest this more forceful wording:

The MP table code scans low memory, which causes failures in VTLs
other than VTL0.   Allow building for VTLs other than VTL0 only when
MP table functionality is disabled.

If you can give slightly more detail about "causes failures" (a panic?
some other problem?) that would be good too.

Michael

> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/hv/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 00242107d62e..c8d443886b9d 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -16,6 +16,8 @@ config HYPERV
>  config HYPERV_VTL_MODE
>  	bool "Enable Linux to boot in VTL context"
>  	depends on X86_64 && HYPERV
> +	depends on !MTRR
> +	depends on !X86_MPPARSE
>  	default n
>  	help
>  	  Virtual Secure Mode (VSM) is a set of hypervisor capabilities and
> --
> 2.34.1

RE: [PATCH 2/2] Drivers: hv: Kconfig: Add HYPERV_VTL_MODE dependencies

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Saturday, May 20, 2023 10:20 PM
> To: Saurabh Sengar <ssengar@linux.microsoft.com>; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de; dave.hansen@linux.intel.com;
> x86@kernel.org; hpa@zytor.com; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; linux-kernel@vger.kernel.org; linux-
> hyperv@vger.kernel.org
> Cc: Saurabh Singh Sengar <ssengar@microsoft.com>
> Subject: [EXTERNAL] RE: [PATCH 2/2] Drivers: hv: Kconfig: Add
> HYPERV_VTL_MODE dependencies
> 
> From: Saurabh Sengar <ssengar@linux.microsoft.com>
> >
> > MTRRs are not per-VTL and are controlled by VTL0, make sure it is
> > disabled for all other VTLs.
> 
> Clarifying the wording a bit:
> 
> MTRRs are not per-VTL, but are always controlled by VTL0.   Allow
> building for VTLs other than VTL0 only when MTRR functionality is disabled.

Ok

> 
> >
> > X86_MPPARSE scans low memory for MP tables, which is not required for
> > Hyper-V VTL platforms.
> 
> Is the above statement saying that MP tables aren't required, so remove the
> code to save space?  Or is there a problem/failure if MP table code
> runs and scans low memory?   *Requiring* that it be disabled, and the
> reference to scanning low memory makes me think it might be the latter, in
> which case I'd suggest this more forceful wording:
> 
> The MP table code scans low memory, which causes failures in VTLs
> other than VTL0.   Allow building for VTLs other than VTL0 only when
> MP table functionality is disabled.

Ok

> 
> If you can give slightly more detail about "causes failures" (a panic?
> some other problem?) that would be good too.

When CONFIG_X86_MPPARSE is enabled, the kernel will scan low memory,
looking for MP tables. In Hyper-V VBS setup, lower memory is assigned to
VTL0. This lower memory may contain the actual MPPARSE table for VTL0,
which can confuse the VTLx kernel and cause issues. (x > 0)

> 
> Michael
> 
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  drivers/hv/Kconfig | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig index
> > 00242107d62e..c8d443886b9d 100644
> > --- a/drivers/hv/Kconfig
> > +++ b/drivers/hv/Kconfig
> > @@ -16,6 +16,8 @@ config HYPERV
> >  config HYPERV_VTL_MODE
> >  	bool "Enable Linux to boot in VTL context"
> >  	depends on X86_64 && HYPERV
> > +	depends on !MTRR
> > +	depends on !X86_MPPARSE
> >  	default n
> >  	help
> >  	  Virtual Secure Mode (VSM) is a set of hypervisor capabilities and
> > --
> > 2.34.1

RE: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Michael Kelley (LINUX)]

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, May 11, 2023 7:55 AM
> 
> X86_MPPARSE is only selectable when ACPI is enabled. However,
> on Devicetree platforms where ACPI is disabled, it is always
> enabled. Allow X86_MPPARSE to be selected by OF platforms as
> well.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  arch/x86/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 53bab123a8ee..e60315397399 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -469,7 +469,7 @@ config X86_X2APIC
>  	  If you don't know what to do here, say N.
> 
>  config X86_MPPARSE
> -	bool "Enable MPS table" if ACPI
> +	bool "Enable MPS table" if ACPI || OF
>  	default y
>  	depends on X86_LOCAL_APIC
>  	help
> --
> 2.34.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Dave Hansen]

On 5/11/23 07:54, Saurabh Sengar wrote:
> X86_MPPARSE is only selectable when ACPI is enabled. However,
> on Devicetree platforms where ACPI is disabled, it is always
> enabled. Allow X86_MPPARSE to be selected by OF platforms as
> well.

I'm finding this changelog really hard to read.

In Kconfig, you can "select FOO".  But in this changelog, it means
something different.  I think "selectable" here means that there's a
user prompt for the option.

Could you please rephrase this to be less confusing?

This is also one of those patches where I wonder: Why do _you_ care
about this?  Are you just trying to be nice?  Is this intended as some
kind of cleanup?

RE: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Tuesday, May 23, 2023 11:23 PM
> To: Saurabh Sengar <ssengar@linux.microsoft.com>; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de; dave.hansen@linux.intel.com;
> x86@kernel.org; hpa@zytor.com; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; Michael Kelley (LINUX) <mikelley@microsoft.com>;
> linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Cc: Saurabh Singh Sengar <ssengar@microsoft.com>
> Subject: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow
> CONFIG_X86_MPPARSE disable for OF platforms
> 
> On 5/11/23 07:54, Saurabh Sengar wrote:
> > X86_MPPARSE is only selectable when ACPI is enabled. However, on
> > Devicetree platforms where ACPI is disabled, it is always enabled.
> > Allow X86_MPPARSE to be selected by OF platforms as well.
> 
> I'm finding this changelog really hard to read.
> 
> In Kconfig, you can "select FOO".  But in this changelog, it means something
> different.  I think "selectable" here means that there's a user prompt for the
> option.
> 
> Could you please rephrase this to be less confusing?

Thanks for your review. Currently, in the absence of ACPI, it is impossible to
disable X86_MPPARSE. In the case of ACPI being enabled, one has the
option to either enable or disable X86_MPARSE. My intention is to permit
X86_MPPARSE=n for OF platforms where ACPI=n. To describe the capability
of choosing any desired value for MPPARSE, I used the term 'selectable.'
Perhaps 'configurable' would be a more appropriate word in this context?
I can fix this and include it in V2.

> 
> This is also one of those patches where I wonder: Why do _you_ care about
> this?  Are you just trying to be nice?  Is this intended as some kind of cleanup?


It solves an issue for Hyper-V VBS setup, please refer to the 2/2 of this patch
series.

Regards,
Saurabh

Re: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Dave Hansen]

On 5/24/23 09:23, Saurabh Singh Sengar wrote:
>> Could you please rephrase this to be less confusing?
> 
> Thanks for your review. Currently, in the absence of ACPI, it is impossible to
> disable X86_MPPARSE. In the case of ACPI being enabled, one has the
> option to either enable or disable X86_MPARSE. My intention is to permit
> X86_MPPARSE=n for OF platforms where ACPI=n. To describe the capability
> of choosing any desired value for MPPARSE, I used the term 'selectable.'
> Perhaps 'configurable' would be a more appropriate word in this context?
> I can fix this and include it in V2.

OK, so this particular Hyper-V setup doesn't run normal normal distro
kernels?  It requires a very specialized kernel?  Or it's just _better_
that the kernel is configured this way?

>> This is also one of those patches where I wonder: Why do _you_ care about
>> this?  Are you just trying to be nice?  Is this intended as some kind of cleanup?
> 
> It solves an issue for Hyper-V VBS setup, please refer to the 2/2 of this patch
> series.

Heh, that changelog is even more confusing than _this_ one.  It doesn't
say that there's a problem, only that it removes "not required" code.

I'm still confused by this whole thing.

RE: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Wednesday, May 24, 2023 11:33 PM
> To: Saurabh Singh Sengar <ssengar@microsoft.com>; Saurabh Sengar
> <ssengar@linux.microsoft.com>; tglx@linutronix.de; mingo@redhat.com;
> bp@alien8.de; dave.hansen@linux.intel.com; x86@kernel.org;
> hpa@zytor.com; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; Michael Kelley (LINUX) <mikelley@microsoft.com>;
> linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: Re: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow
> CONFIG_X86_MPPARSE disable for OF platforms
> 
> On 5/24/23 09:23, Saurabh Singh Sengar wrote:
> >> Could you please rephrase this to be less confusing?
> >
> > Thanks for your review. Currently, in the absence of ACPI, it is
> > impossible to disable X86_MPPARSE. In the case of ACPI being enabled,
> > one has the option to either enable or disable X86_MPARSE. My
> > intention is to permit X86_MPPARSE=n for OF platforms where ACPI=n. To
> > describe the capability of choosing any desired value for MPPARSE, I used
> the term 'selectable.'
> > Perhaps 'configurable' would be a more appropriate word in this context?
> > I can fix this and include it in V2.
> 
> OK, so this particular Hyper-V setup doesn't run normal normal distro
> kernels?  It requires a very specialized kernel?  Or it's just _better_ that the
> kernel is configured this way?

This is a system where we provide isolation through VTL. More information
About VTL is here: https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/vsm#virtual-trust-level-vtl

In the context of VTL0, the system runs the normal kernel. However, for
higher VTLs, we enable the kernel with HYPERV_VTL_MODE. It's important
to note that the lower memory of the system is not included in the higher VTLs

It runs the normal kernel as part of VTL0 but for higher VTLs we run the kernel with
HYPERV_VTL_MODE enable. VTL0 is assigned to lower memory of system, the lower
memory not part of higher VTLs. However, MPARSE option disregard this and try to
scan VTL0 memory which causes issue. This will enable any system which
doesn't want lower memory to be scanned for MPPARSE for whatever reason.

> 
> >> This is also one of those patches where I wonder: Why do _you_ care
> >> about this?  Are you just trying to be nice?  Is this intended as some kind of
> cleanup?
> >
> > It solves an issue for Hyper-V VBS setup, please refer to the 2/2 of
> > this patch series.
> 
> Heh, that changelog is even more confusing than _this_ one.  It doesn't say
> that there's a problem, only that it removes "not required" code.
> 
> I'm still confused by this whole thing.

I agree the commit message could be better. And I commented below in that
thread to clarify. Hope it clarifies, please let me know if any more information is
required:

When CONFIG_X86_MPPARSE is enabled, the kernel will scan low memory,
looking for MP tables. In Hyper-V VBS setup, lower memory is assigned to VTL0.
This lower memory may contain the actual MPPARSE table for VTL0,
which can confuse the VTLx kernel and cause issues. (x > 0)

Re: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Dave Hansen]

On 5/25/23 00:06, Saurabh Singh Sengar wrote:
> When CONFIG_X86_MPPARSE is enabled, the kernel will scan low memory,
> looking for MP tables. In Hyper-V VBS setup, lower memory is assigned to VTL0.
> This lower memory may contain the actual MPPARSE table for VTL0,
> which can confuse the VTLx kernel and cause issues. (x > 0)

This still just seems wrong.

If an action crashes the kernel because of changes, we don't just
compile it out and move on.  We add enumeration for the new feature
that's causing it and turn off the action at runtime.

RE: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Thursday, May 25, 2023 7:26 PM
> To: Saurabh Singh Sengar <ssengar@microsoft.com>; Saurabh Sengar
> <ssengar@linux.microsoft.com>; tglx@linutronix.de; mingo@redhat.com;
> bp@alien8.de; dave.hansen@linux.intel.com; x86@kernel.org;
> hpa@zytor.com; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; Michael Kelley (LINUX) <mikelley@microsoft.com>;
> linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: Re: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow
> CONFIG_X86_MPPARSE disable for OF platforms
> 
> On 5/25/23 00:06, Saurabh Singh Sengar wrote:
> > When CONFIG_X86_MPPARSE is enabled, the kernel will scan low memory,
> > looking for MP tables. In Hyper-V VBS setup, lower memory is assigned to
> VTL0.
> > This lower memory may contain the actual MPPARSE table for VTL0, which
> > can confuse the VTLx kernel and cause issues. (x > 0)
> 
> This still just seems wrong.
> 
> If an action crashes the kernel because of changes, we don't just compile it
> out and move on.  We add enumeration for the new feature that's causing it
> and turn off the action at runtime.
> 

Thanks, I greatly appreciate your suggestion and wanted to let you know that
I am actively working on upstreaming the new fix for the VTL driver to bypass
the need for the MP table scan.

Furthermore, I would like to learn about the rationale behind disallowing the
disablement of CONFIG_X86_MPPARSE when MP tables are not in use. Considering
that we compile out the features we don't support, wouldn't it be acceptable to
allow users to customize their configurations in this manner? Allowing the
disablement of CONFIG_X86_MPPARSE would provide greater flexibility and
efficiency for those who do not utilize MP tables.

Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Dave Hansen]

On 6/2/23 05:22, Saurabh Singh Sengar wrote:
> Furthermore, I would like to learn about the rationale behind disallowing the
> disablement of CONFIG_X86_MPPARSE when MP tables are not in use. Considering
> that we compile out the features we don't support, wouldn't it be acceptable to
> allow users to customize their configurations in this manner? Allowing the
> disablement of CONFIG_X86_MPPARSE would provide greater flexibility and
> efficiency for those who do not utilize MP tables.

If someone sent a patch, I'd certainly look and figure out what
"flexibility" and "efficiency" it would provide.  But, honestly, it
would just just be noise if it doesn't solve an _actual_ problem.

Would anyone care or notice the "flexibility" and "efficiency" it would
provide?

RE: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow CONFIG_X86_MPPARSE disable for OF platforms

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Friday, June 2, 2023 7:54 PM
> To: Saurabh Singh Sengar <ssengar@microsoft.com>; Saurabh Sengar
> <ssengar@linux.microsoft.com>; tglx@linutronix.de; mingo@redhat.com;
> bp@alien8.de; dave.hansen@linux.intel.com; x86@kernel.org;
> hpa@zytor.com; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; Michael Kelley (LINUX) <mikelley@microsoft.com>;
> linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH 1/2] x86/Kconfig: Allow
> CONFIG_X86_MPPARSE disable for OF platforms
> 
> On 6/2/23 05:22, Saurabh Singh Sengar wrote:
> > Furthermore, I would like to learn about the rationale behind
> > disallowing the disablement of CONFIG_X86_MPPARSE when MP tables are
> > not in use. Considering that we compile out the features we don't
> > support, wouldn't it be acceptable to allow users to customize their
> > configurations in this manner? Allowing the disablement of
> > CONFIG_X86_MPPARSE would provide greater flexibility and efficiency for
> those who do not utilize MP tables.
> 
> If someone sent a patch, I'd certainly look and figure out what "flexibility" and
> "efficiency" it would provide.  But, honestly, it would just just be noise if it
> doesn't solve an _actual_ problem.
> 
> Would anyone care or notice the "flexibility" and "efficiency" it would
> provide?

After the new approach we have opted, I agree it's not blocking us from any functional
problem. Although one of the things we are trying to achieve is the minimal kernel
where we want to remove all the unnecessary options. Our system is not dependent
MPPARSE and we just don't want this code in our minimal kernel, and this config
option is preventing us from removing it.

Also, in past kernel has provided this flexibility for other options, which made me think
it is fine do it for OF as well.
https://lkml.indiana.edu/hypermail/linux/kernel/1210.3/01987.html

Happy to know your opinion and learn.