In certain configurations, VTL0 and VTL2 can share the same VM
partition and hence share the same memory address space. In such
systems VTL2 has visibility of all of the VTL0 memory space.
When CONFIG_X86_MPPARSE is enabled for VTL2, the kernel performs
a scan of low memory to search for MP tables. However, in systems
where VTL0 controls the low memory and may contain valid tables
specific to VTL0, this scanning process can lead to confusion
within the VTL2 kernel.
In !ACPI system, there is no way to disable CONFIG_X86_MPPARSE
hence add the noop function instead.
Signed-off-by: Saurabh Sengar <[email protected]>
---
[V2]: Improve commit message
arch/x86/hyperv/hv_vtl.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 85d38b9f3586..db5d2ea39fc0 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -25,6 +25,10 @@ void __init hv_vtl_init_platform(void)
x86_init.irqs.pre_vector_init = x86_init_noop;
x86_init.timers.timer_init = x86_init_noop;
+ /* Avoid searching for BIOS MP tables */
+ x86_init.mpparse.find_smp_config = x86_init_noop;
+ x86_init.mpparse.get_smp_config = x86_init_uint_noop;
+
x86_platform.get_wallclock = get_rtc_noop;
x86_platform.set_wallclock = set_rtc_noop;
x86_platform.get_nmi_reason = hv_get_nmi_reason;
--
2.34.1
On 6/13/23 00:13, Saurabh Sengar wrote:
> In certain configurations, VTL0 and VTL2 can share the same VM
> partition and hence share the same memory address space. In such
> systems VTL2 has visibility of all of the VTL0 memory space.
FWIW, this is pretty much gibberish to me. The way I suggest avoiding
producing gibberish is avoiding acronyms:
Hyper-V can run VMs at different privilege "levels". Sometimes,
it chooses to run two different VMs at different levels but
they share some of their address space. This <insert reason
that someone might want to do this>.
That's not gibberish.
> When CONFIG_X86_MPPARSE is enabled for VTL2, the kernel performs
> a scan of low memory to search for MP tables. However, in systems
> where VTL0 controls the low memory and may contain valid tables
> specific to VTL0, this scanning process can lead to confusion
> within the VTL2 kernel.
What is the end-user-visible effect of this "confusion"? A crash? A
warning? An error message? What is the impact on end users?
This information will help the maintainers decide how to disposition
your patch. Should we send it upstream immediately because it's
impacting millions of users? Or can we do it in a bit more leisurely
fashion because nobody cares?
> In !ACPI system, there is no way to disable CONFIG_X86_MPPARSE
> hence add the noop function instead.
This makes zero sense to me.
Like I told you before, we don't compile things out just because they
don't work on one weirdo system. If we did that, we'd have a billion
incompatible x86 kernel images that can't boot across systems.
> -----Original Message-----
> From: Dave Hansen <[email protected]>
> Sent: Tuesday, June 13, 2023 11:03 PM
> To: Saurabh Sengar <[email protected]>; KY Srinivasan
> <[email protected]>; Haiyang Zhang <[email protected]>;
> [email protected]; Dexuan Cui <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Michael Kelley (LINUX) <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> Subject: [EXTERNAL] Re: [PATCH v2] x86/hyperv: add noop functions to
> x86_init mpparse functions
>
> On 6/13/23 00:13, Saurabh Sengar wrote:
> > In certain configurations, VTL0 and VTL2 can share the same VM
> > partition and hence share the same memory address space. In such
> > systems VTL2 has visibility of all of the VTL0 memory space.
>
> FWIW, this is pretty much gibberish to me. The way I suggest avoiding
> producing gibberish is avoiding acronyms:
>
> Hyper-V can run VMs at different privilege "levels". Sometimes,
> it chooses to run two different VMs at different levels but
> they share some of their address space. This <insert reason
> that someone might want to do this>.
>
> That's not gibberish.
Thanks for your suggestion I can add this in v3.
>
> > When CONFIG_X86_MPPARSE is enabled for VTL2, the kernel performs a
> > scan of low memory to search for MP tables. However, in systems where
> > VTL0 controls the low memory and may contain valid tables specific to
> > VTL0, this scanning process can lead to confusion within the VTL2
> > kernel.
>
> What is the end-user-visible effect of this "confusion"? A crash? A warning?
> An error message? What is the impact on end users?
The VTL2 kernel is currently scanning the VTL0 MP table and incorporating that
information, which is incorrect because VTL2 doesn't support MP tables and
instead, is booted with DT. While I don't have an immediate crash or error
message to present, this situation could potentially result in incorrect behaviour.
>
> This information will help the maintainers decide how to disposition your
> patch. Should we send it upstream immediately because it's impacting
> millions of users? Or can we do it in a bit more leisurely fashion because
> nobody cares?
I understand, I have provided all the information I have please consider what is
appropriate in this case.
>
> > In !ACPI system, there is no way to disable CONFIG_X86_MPPARSE hence
> > add the noop function instead.
>
> This makes zero sense to me.
My intention was to tell that this fix is required because we are in !ACPI system.
If it was ACPI system we could have simply disable this CONFIG_X86_MPPARSE
Option. But as you suggested I am fine removing these 2 lines.
>
> Like I told you before, we don't compile things out just because they don't
> work on one weirdo system. If we did that, we'd have a billion incompatible
> x86 kernel images that can't boot across systems.
>
Understood, thank you. I was just considering the option of keeping the default
setting for CONFIG_X86_MPPARSE as 'Y' and allowing the choice to change it to
'N' if someone specifically desires to do so. I also considered that nowadays, not
many kernels are likely to utilize MP tables for booting x86 machines, which is why
I thought this change wouldn't have a significant impact. Moreover, there is a
potential benefit in terms of reducing the kernel's size. However, I completely
respect and am open to whatever you decide, having better visibility of wider
kernel community needs.
- Saurabh
> -----Original Message-----
> From: Saurabh Singh Sengar <[email protected]>
> Sent: Wednesday, June 14, 2023 9:36 AM
> To: Dave Hansen <[email protected]>; Saurabh Sengar
> <[email protected]>; KY Srinivasan <[email protected]>;
> Haiyang Zhang <[email protected]>; [email protected]; Dexuan Cui
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Michael Kelley
> (LINUX) <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> Subject: RE: [EXTERNAL] Re: [PATCH v2] x86/hyperv: add noop functions to
> x86_init mpparse functions
>
>
>
> > -----Original Message-----
> > From: Dave Hansen <[email protected]>
> > Sent: Tuesday, June 13, 2023 11:03 PM
> > To: Saurabh Sengar <[email protected]>; KY Srinivasan
> > <[email protected]>; Haiyang Zhang <[email protected]>;
> > [email protected]; Dexuan Cui <[email protected]>;
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Michael Kelley (LINUX)
> > <[email protected]>; linux- [email protected];
> > [email protected]; [email protected]
> > Subject: [EXTERNAL] Re: [PATCH v2] x86/hyperv: add noop functions to
> > x86_init mpparse functions
> >
> > On 6/13/23 00:13, Saurabh Sengar wrote:
> > > In certain configurations, VTL0 and VTL2 can share the same VM
> > > partition and hence share the same memory address space. In such
> > > systems VTL2 has visibility of all of the VTL0 memory space.
> >
> > FWIW, this is pretty much gibberish to me. The way I suggest avoiding
> > producing gibberish is avoiding acronyms:
> >
> > Hyper-V can run VMs at different privilege "levels". Sometimes,
> > it chooses to run two different VMs at different levels but
> > they share some of their address space. This <insert reason
> > that someone might want to do this>.
> >
> > That's not gibberish.
>
> Thanks for your suggestion I can add this in v3.
>
> >
> > > When CONFIG_X86_MPPARSE is enabled for VTL2, the kernel performs a
> > > scan of low memory to search for MP tables. However, in systems
> > > where
> > > VTL0 controls the low memory and may contain valid tables specific
> > > to VTL0, this scanning process can lead to confusion within the VTL2
> > > kernel.
> >
> > What is the end-user-visible effect of this "confusion"? A crash? A
> warning?
> > An error message? What is the impact on end users?
>
> The VTL2 kernel is currently scanning the VTL0 MP table and incorporating
> that information, which is incorrect because VTL2 doesn't support MP tables
> and instead, is booted with DT. While I don't have an immediate crash or
> error message to present, this situation could potentially result in incorrect
> behaviour.
>
> >
> > This information will help the maintainers decide how to disposition
> > your patch. Should we send it upstream immediately because it's
> > impacting millions of users? Or can we do it in a bit more leisurely
> > fashion because nobody cares?
>
> I understand, I have provided all the information I have please consider what
> is appropriate in this case.
>
> >
> > > In !ACPI system, there is no way to disable CONFIG_X86_MPPARSE hence
> > > add the noop function instead.
> >
> > This makes zero sense to me.
>
> My intention was to tell that this fix is required because we are in !ACPI
> system.
> If it was ACPI system we could have simply disable this
> CONFIG_X86_MPPARSE Option. But as you suggested I am fine removing
> these 2 lines.
>
> >
> > Like I told you before, we don't compile things out just because they
> > don't work on one weirdo system. If we did that, we'd have a billion
> > incompatible
> > x86 kernel images that can't boot across systems.
> >
>
> Understood, thank you. I was just considering the option of keeping the
> default setting for CONFIG_X86_MPPARSE as 'Y' and allowing the choice to
> change it to 'N' if someone specifically desires to do so. I also considered that
> nowadays, not many kernels are likely to utilize MP tables for booting x86
> machines, which is why I thought this change wouldn't have a significant
> impact. Moreover, there is a potential benefit in terms of reducing the
> kernel's size. However, I completely respect and am open to whatever you
> decide, having better visibility of wider kernel community needs.
>
> - Saurabh
Below is the updated commit message, if there are no more concerns I will
send the V3 with it.
Hyper-V can run VMs at different privilege "levels" known as Virtual
Trust Levels (VTL). Sometimes, it chooses to run two different VMs
at different levels but they share some of their address space. In
such setups VTL2 (higher level VM) has visibility of all of the
VTL0 (level 0) memory space.
When the CONFIG_X86_MPPARSE is enabled for VTL2, the VTL2 kernel
performs a search within the low memory to locate MP tables. However,
in systems where VTL0 manages the low memory and may contain valid
tables, this scanning can result in incorrect MP table information
being provided to the VTL2 kernel, mistakenly considering VTL0's MP
table as its own
Add noop functions to avoid MP parse scan by VTL2.
- Saurabh