RE: [EXTERNAL] Re: [PATCH v2] x86/hyperv: add noop functions to x86_init mpparse functions

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Dave Hansen <dave.hansen@xxxxxxxxx>
> Sent: Tuesday, June 13, 2023 11:03 PM
> To: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>; KY Srinivasan
> <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>;
> wei.liu@xxxxxxxxxx; Dexuan Cui <decui@xxxxxxxxxxxxx>; tglx@xxxxxxxxxxxxx;
> mingo@xxxxxxxxxx; bp@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
> x86@xxxxxxxxxx; Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx; hpa@xxxxxxxxx
> 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