linux-kernel - RE: [PATCH v2 03/28] ACPICA: Hardware: Enable 64-bit firmware waking vector for selected FACS.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1AE640813FDE7649BE1B193DEA596E88027340D8@SHSMSX101.ccr.corp.intel.com>
Date:	Fri, 26 Jun 2015 01:39:22 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Brown, Len" <len.brown@...el.com>, Lv Zheng <zetalog@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"Moore, Robert" <robert.moore@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"Luck, Tony" <tony.luck@...el.com>,
	"Yu, Fenghua" <fenghua.yu@...el.com>,
	"linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>
Subject: RE: [PATCH v2 03/28] ACPICA: Hardware: Enable 64-bit firmware
 waking vector for selected FACS.

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> Sent: Friday, June 26, 2015 9:21 AM
> 
> On Thursday, June 25, 2015 12:29:02 AM Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> > > Sent: Thursday, June 25, 2015 7:24 AM
> > > To: Zheng, Lv
> 
> [cut]
> 
> > >
> > > In fact, I don't see why we need to redefine the symbols at all.
> > >
> > > Couldn't acpi_set_firmware_waking_vector() be defined to take u32 and u64 so
> > > we could just pass acpi_wakeup_address (as already defined) as the first argument
> > > and 0 as the second argument to it?  The back-and-forth type casts from and
> > > to acpi_physical_address don't look entirely clean to me.
> > >
> > > Moreover, I don't really see a functional difference between the old and the
> > > new code.
> > >
> > > The old code does "set the 32-bit waking vector and clear the 64-bit waking
> > > vector if present".  The new code does "set the 32-bit waking vector and
> > > either clear the 64-bit one if present, or assign the second function argument
> > > to it", but we always pass 0 as the second argument (which is *extremely*
> > > obfuscated in your patch), so I really don't see the difference here.
> > >
> > > Am I missing anything?
> >
> > The story is:
> > According to the test, if both 32-bit waking vector and 64-bit waking vector is
> > set by the OSPM,
> 
> The current code in Linux never does that.
> 
> It never calls acpi_set_firmware_waking_vector64() and the acpi_set_firmware_waking_vector()
> (before your patch) explicitly clears the 64-bit vector address.

In ACPICA upstream, this is an issue before applying this patch.
acpi_set_firmware_waking_vector64() and acpi_set_firmware_waking_vector() depends on ACPI_MACHINE_WIDTH definition, thus cannot be used for this purpose.

> 
> > BIOSes only support 32-bit resume environment will jump to the 32-bit waking
> > vector address and BIOSes support 64-bit resume environment will jump to
> > 64-bit waking vector.
> 
> Which doesn't matter for Linux one whit.

The bug report is against the 64-bit address favor mechanism.
But if the OSPM can support resuming from 64-bit waking vector, the 64-bit address favor mechanism doesn't seem to be buggy.

> 
> > So they can be set by the OSPMs simultaneously to indicate that the OSPM can
> > support both resume environments.  That's why ACPICA interface is changed.
> 
> It shouldn't.  It just forces host OSes to make pointless changes to their
> non-ACPICA code.
> 
> As I said elsewhere, the old acpi_set_firmware_waking_vector() should still be
> available to the OSes that don't care about the 64-bit waking vector and a *new*
> interface should be added for those OSes that do care about it.  And *internally*
> acpi_set_firmware_waking_vector() can be defined in terms of the new interface,
> but there's no reason at all for a host OS to care about that.

OK, we can refine the interface inside of ACPICA.

> 
> So the $subject patch is entirely poitless.  It doesn't fix anything and it
> doesn't even change the way the code works today in Linux.  It just adds
> complexity and pointlessly redefines some stuff.
> 

It doesn't fix any functionality problem right here in this patch.
But it fixes the code logic problem that acpi_set_firmware_waking_vector64()/acpi_set_firmware_waking_vector() depends on ACPI_MACHINE_WIDTH definition.
And it facilitates the OSPMs with the capability to support both 32-bit/64-bit resuming environment.

> So I'm not going to apply it.

OK, I'll go back to refine the interface change.

Thanks and best regards
-Lv