diff mbox

[4.12,26/84] x86/xen/efi: Initialize only the EFI struct members used by Xen

Message ID 20170719092323.396710042@linuxfoundation.org (mailing list archive)
State New, archived
Headers show

Commit Message

Greg KH July 19, 2017, 9:43 a.m. UTC
4.12-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Daniel Kiper <daniel.kiper@oracle.com>

commit 6c64447ec58b0bac612732303f7ab04562124587 upstream.

The current approach, which is the wholesale efi struct initialization from
a 'efi_xen' local template is not robust. Usually if new member is defined
then it is properly initialized in drivers/firmware/efi/efi.c, but not in
arch/x86/xen/efi.c.

The effect is that the Xen initialization clears any fields the generic code
might have set and the Xen code does not know about yet.

I saw this happen a few times, so let's initialize only the EFI struct members
used by Xen and maintain no local duplicate, to avoid such issues in the future.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: andrew.cooper3@citrix.com
Cc: jgross@suse.com
Cc: linux-efi@vger.kernel.org
Cc: matt@codeblueprint.co.uk
Cc: xen-devel@lists.xenproject.org
Link: http://lkml.kernel.org/r/1498128697-12943-3-git-send-email-daniel.kiper@oracle.com
[ Clarified the changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/x86/xen/efi.c |   45 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

Comments

Daniel Kiper July 19, 2017, 10:37 a.m. UTC | #1
Hey Greg,

On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> 4.12-stable review patch.  If anyone has any objections, please let me know.

Why did you skip this patch for 4.11? IMO it should be applied there too.

Daniel
Greg KH July 19, 2017, 11:12 a.m. UTC | #2
On Wed, Jul 19, 2017 at 12:37:47PM +0200, Daniel Kiper wrote:
> Hey Greg,
> 
> On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > 4.12-stable review patch.  If anyone has any objections, please let me know.
> 
> Why did you skip this patch for 4.11? IMO it should be applied there too.

Are you sure it actually applied?  (hint, it did not...)

If you want it in 4.11, or older kernels, please provide a working
backport.

thanks,

greg k-h
Greg KH July 19, 2017, 11:19 a.m. UTC | #3
On Wed, Jul 19, 2017 at 01:12:14PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 19, 2017 at 12:37:47PM +0200, Daniel Kiper wrote:
> > Hey Greg,
> > 
> > On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > > 4.12-stable review patch.  If anyone has any objections, please let me know.
> > 
> > Why did you skip this patch for 4.11? IMO it should be applied there too.
> 
> Are you sure it actually applied?  (hint, it did not...)
> 
> If you want it in 4.11, or older kernels, please provide a working
> backport.

And, in the future, if you want it to be applied to older kernels, or be
notified if it can not be, please add a kernel version number in the
stable marking:
	Cc: stable@vger.kernel.org # 4.0+
or use the Fixes: tag:
	Fixes: SHASHAHSA ("short description")
which I pick up on and let you know if the patch does not actually apply
back to the kernel that the fixes: tag was in.

hope this helps,

greg k-h
Daniel Kiper July 19, 2017, 11:28 a.m. UTC | #4
On Wed, Jul 19, 2017 at 01:12:14PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 19, 2017 at 12:37:47PM +0200, Daniel Kiper wrote:
> > Hey Greg,
> >
> > On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > > 4.12-stable review patch.  If anyone has any objections, please let me know.
> >
> > Why did you skip this patch for 4.11? IMO it should be applied there too.
>
> Are you sure it actually applied?  (hint, it did not...)
>
> If you want it in 4.11, or older kernels, please provide a working
> backport.

OK, if it did not apply then probably there were some changes in the code
here and there. Though, IIRC, fix itself is perfectly valid for 4.11.
So, I will post updated patch for it.

Thanks,

Daniel
Daniel Kiper July 19, 2017, 11:57 a.m. UTC | #5
On Wed, Jul 19, 2017 at 01:19:58PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 19, 2017 at 01:12:14PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jul 19, 2017 at 12:37:47PM +0200, Daniel Kiper wrote:
> > > Hey Greg,
> > >
> > > On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > > > 4.12-stable review patch.  If anyone has any objections, please let me know.
> > >
> > > Why did you skip this patch for 4.11? IMO it should be applied there too.
> >
> > Are you sure it actually applied?  (hint, it did not...)
> >
> > If you want it in 4.11, or older kernels, please provide a working
> > backport.
>
> And, in the future, if you want it to be applied to older kernels, or be
> notified if it can not be, please add a kernel version number in the
> stable marking:
> 	Cc: stable@vger.kernel.org # 4.0+
> or use the Fixes: tag:
> 	Fixes: SHASHAHSA ("short description")
> which I pick up on and let you know if the patch does not actually apply
> back to the kernel that the fixes: tag was in.
>
> hope this helps,

Sure thing! Thanks a lot!

Daniel
Ingo Molnar July 20, 2017, 8:39 a.m. UTC | #6
* Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Hey Greg,
> 
> On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > 4.12-stable review patch.  If anyone has any objections, please let me know.
> 
> Why did you skip this patch for 4.11? IMO it should be applied there too.

The thing is, this patch should probaly not even be in v4.12, as it should only 
make any difference if there's a separate _bug_ in the kernel.

This patch makes things more robust going forward, but I question that it needs to 
be in -stable.

Thanks,

	Ingo
Greg KH July 20, 2017, 9:16 a.m. UTC | #7
On Thu, Jul 20, 2017 at 10:39:10AM +0200, Ingo Molnar wrote:
> 
> * Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> > Hey Greg,
> > 
> > On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > > 4.12-stable review patch.  If anyone has any objections, please let me know.
> > 
> > Why did you skip this patch for 4.11? IMO it should be applied there too.
> 
> The thing is, this patch should probaly not even be in v4.12, as it should only 
> make any difference if there's a separate _bug_ in the kernel.
> 
> This patch makes things more robust going forward, but I question that it needs to 
> be in -stable.

Yeah, good point, I'm going to go drop it entirely from the 4.12-stable
tree as it obviously isn't stable material, sorry for not catching that
before.

thanks,

greg k-h
Ingo Molnar July 20, 2017, 9:41 a.m. UTC | #8
* Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 20, 2017 at 10:39:10AM +0200, Ingo Molnar wrote:
> > 
> > * Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > 
> > > Hey Greg,
> > > 
> > > On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > > > 4.12-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > Why did you skip this patch for 4.11? IMO it should be applied there too.
> > 
> > The thing is, this patch should probaly not even be in v4.12, as it should only 
> > make any difference if there's a separate _bug_ in the kernel.
> > 
> > This patch makes things more robust going forward, but I question that it needs to 
> > be in -stable.
> 
> Yeah, good point, I'm going to go drop it entirely from the 4.12-stable
> tree as it obviously isn't stable material, sorry for not catching that
> before.

I should have caught the tag as well when applying the upstream patch to begin 
with.

Thanks!

	Ingo
Daniel Kiper July 20, 2017, 12:33 p.m. UTC | #9
On Thu, Jul 20, 2017 at 11:16:39AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 20, 2017 at 10:39:10AM +0200, Ingo Molnar wrote:
> >
> > * Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > > Hey Greg,
> > >
> > > On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
> > > > 4.12-stable review patch.  If anyone has any objections, please let me know.
> > >
> > > Why did you skip this patch for 4.11? IMO it should be applied there too.
> >
> > The thing is, this patch should probaly not even be in v4.12, as it should only
> > make any difference if there's a separate _bug_ in the kernel.
> >
> > This patch makes things more robust going forward, but I question that it needs to
> > be in -stable.
>
> Yeah, good point, I'm going to go drop it entirely from the 4.12-stable
> tree as it obviously isn't stable material, sorry for not catching that
> before.

Wait a minute. IIRC, Juergen told me last week that this patch fixes a bug
found/assigned by/to him. Juergen? If it is true then I would apply it to
stable. If I am wrong you can drop it.

Thanks,

Daniel
Jürgen Groß July 21, 2017, 6:32 a.m. UTC | #10
On 20/07/17 14:33, Daniel Kiper wrote:
> On Thu, Jul 20, 2017 at 11:16:39AM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Jul 20, 2017 at 10:39:10AM +0200, Ingo Molnar wrote:
>>>
>>> * Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>
>>>> Hey Greg,
>>>>
>>>> On Wed, Jul 19, 2017 at 11:43:32AM +0200, Greg Kroah-Hartman wrote:
>>>>> 4.12-stable review patch.  If anyone has any objections, please let me know.
>>>>
>>>> Why did you skip this patch for 4.11? IMO it should be applied there too.
>>>
>>> The thing is, this patch should probaly not even be in v4.12, as it should only
>>> make any difference if there's a separate _bug_ in the kernel.
>>>
>>> This patch makes things more robust going forward, but I question that it needs to
>>> be in -stable.
>>
>> Yeah, good point, I'm going to go drop it entirely from the 4.12-stable
>> tree as it obviously isn't stable material, sorry for not catching that
>> before.
> 
> Wait a minute. IIRC, Juergen told me last week that this patch fixes a bug
> found/assigned by/to him. Juergen? If it is true then I would apply it to
> stable. If I am wrong you can drop it.

I'm not sure both patches (this one and upstream 457ea3f7e97881f) are
needed. I have got a report about a failing boot and with both patches
applied the boot is working. I will ask if 457ea3f7e97881f alone works,
too. If it doesn't I'll request this patch to be added to stable again.


Juergen
diff mbox

Patch

--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -54,38 +54,6 @@  static efi_system_table_t efi_systab_xen
 	.tables		= EFI_INVALID_TABLE_ADDR  /* Initialized later. */
 };
 
-static const struct efi efi_xen __initconst = {
-	.systab                   = NULL, /* Initialized later. */
-	.runtime_version	  = 0,    /* Initialized later. */
-	.mps                      = EFI_INVALID_TABLE_ADDR,
-	.acpi                     = EFI_INVALID_TABLE_ADDR,
-	.acpi20                   = EFI_INVALID_TABLE_ADDR,
-	.smbios                   = EFI_INVALID_TABLE_ADDR,
-	.smbios3                  = EFI_INVALID_TABLE_ADDR,
-	.sal_systab               = EFI_INVALID_TABLE_ADDR,
-	.boot_info                = EFI_INVALID_TABLE_ADDR,
-	.hcdp                     = EFI_INVALID_TABLE_ADDR,
-	.uga                      = EFI_INVALID_TABLE_ADDR,
-	.uv_systab                = EFI_INVALID_TABLE_ADDR,
-	.fw_vendor                = EFI_INVALID_TABLE_ADDR,
-	.runtime                  = EFI_INVALID_TABLE_ADDR,
-	.config_table             = EFI_INVALID_TABLE_ADDR,
-	.get_time                 = xen_efi_get_time,
-	.set_time                 = xen_efi_set_time,
-	.get_wakeup_time          = xen_efi_get_wakeup_time,
-	.set_wakeup_time          = xen_efi_set_wakeup_time,
-	.get_variable             = xen_efi_get_variable,
-	.get_next_variable        = xen_efi_get_next_variable,
-	.set_variable             = xen_efi_set_variable,
-	.query_variable_info      = xen_efi_query_variable_info,
-	.update_capsule           = xen_efi_update_capsule,
-	.query_capsule_caps       = xen_efi_query_capsule_caps,
-	.get_next_high_mono_count = xen_efi_get_next_high_mono_count,
-	.reset_system             = xen_efi_reset_system,
-	.set_virtual_address_map  = NULL, /* Not used under Xen. */
-	.flags			  = 0     /* Initialized later. */
-};
-
 static efi_system_table_t __init *xen_efi_probe(void)
 {
 	struct xen_platform_op op = {
@@ -102,7 +70,18 @@  static efi_system_table_t __init *xen_ef
 
 	/* Here we know that Xen runs on EFI platform. */
 
-	efi = efi_xen;
+	efi.get_time                 = xen_efi_get_time;
+	efi.set_time                 = xen_efi_set_time;
+	efi.get_wakeup_time          = xen_efi_get_wakeup_time;
+	efi.set_wakeup_time          = xen_efi_set_wakeup_time;
+	efi.get_variable             = xen_efi_get_variable;
+	efi.get_next_variable        = xen_efi_get_next_variable;
+	efi.set_variable             = xen_efi_set_variable;
+	efi.query_variable_info      = xen_efi_query_variable_info;
+	efi.update_capsule           = xen_efi_update_capsule;
+	efi.query_capsule_caps       = xen_efi_query_capsule_caps;
+	efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
+	efi.reset_system             = xen_efi_reset_system;
 
 	efi_systab_xen.tables = info->cfg.addr;
 	efi_systab_xen.nr_tables = info->cfg.nent;