Message-ID: <PUZP153MB074981D5C1823DFF47166668BE5CA@PUZP153MB0749.APCP153.PROD.OUTLOOK.COM>
Date:   Tue, 20 Jun 2023 10:09:11 +0000
From:   Saurabh Singh Sengar <ssengar@...rosoft.com>
To:     Saurabh Singh Sengar <ssengar@...rosoft.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Saurabh Sengar <ssengar@...ux.microsoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "hpa@...or.com" <hpa@...or.com>
Subject: RE: [EXTERNAL] Re: [PATCH v2] x86/hyperv: add noop functions to
 x86_init mpparse functions



> -----Original Message-----
> From: Saurabh Singh Sengar <ssengar@...rosoft.com>
> Sent: Wednesday, June 14, 2023 9:36 AM
> To: Dave Hansen <dave.hansen@...el.com>; Saurabh Sengar
> <ssengar@...ux.microsoft.com>; KY Srinivasan <kys@...rosoft.com>;
> Haiyang Zhang <haiyangz@...rosoft.com>; wei.liu@...nel.org; Dexuan Cui
> <decui@...rosoft.com>; tglx@...utronix.de; mingo@...hat.com;
> bp@...en8.de; dave.hansen@...ux.intel.com; x86@...nel.org; Michael Kelley
> (LINUX) <mikelley@...rosoft.com>; linux-kernel@...r.kernel.org; linux-
> hyperv@...r.kernel.org; hpa@...or.com
> Subject: RE: [EXTERNAL] Re: [PATCH v2] x86/hyperv: add noop functions to
> x86_init mpparse functions
> 
> 
> 
> > -----Original Message-----
> > From: Dave Hansen <dave.hansen@...el.com>
> > Sent: Tuesday, June 13, 2023 11:03 PM
> > To: Saurabh Sengar <ssengar@...ux.microsoft.com>; KY Srinivasan
> > <kys@...rosoft.com>; Haiyang Zhang <haiyangz@...rosoft.com>;
> > wei.liu@...nel.org; Dexuan Cui <decui@...rosoft.com>;
> > tglx@...utronix.de; mingo@...hat.com; bp@...en8.de;
> > dave.hansen@...ux.intel.com; x86@...nel.org; Michael Kelley (LINUX)
> > <mikelley@...rosoft.com>; linux- kernel@...r.kernel.org;
> > linux-hyperv@...r.kernel.org; hpa@...or.com
> > 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

Powered by blists - more mailing lists