Re: [Qemu-ppc] [Qemu-devel] [PATCH 0/5] nvram: Refactor OpenBIOS NVRAM code to support -prom-env on pseries, too

[Thomas Huth]

On 26.10.2016 02:03, David Gibson wrote:
> On Tue, Oct 25, 2016 at 02:22:08PM +0200, Thomas Huth wrote:
>> On 24.10.2016 14:04, David Gibson wrote:
>>> On Mon, Oct 24, 2016 at 12:36:05PM +0200, Thomas Huth wrote:
>>>> On 24.10.2016 12:22, Bharata B Rao wrote:
>>>>>
>>>>> On Wed, Oct 19, 2016 at 7:46 AM, David Gibson
>>>>> <address@hidden <mailto:address@hidden>> wrote:
>>>>>
>>>>>     On Tue, Oct 18, 2016 at 10:46:39PM +0200, Thomas Huth wrote:
>>>>>     > The OpenBIOS NVRAM set-up is based on the layout defined in the CHRP
>>>>>     > (Common Hardware Reference Platform) specification. This is the same
>>>>>     > layout that is also used by the PAPR specification and thus by the
>>>>>     SLOF
>>>>>     > firmware of the pseries machine. By refactoring the NVRAM code from
>>>>>     > mac_nvram.c, we can use the same functions for setting up the NVRAM
>>>>>     > for both, OpenBIOS and SLOF. This way we can support the "-prom-env"
>>>>>     > parameter of QEMU for SLOF, too, which is very useful to influence
>>>>>     > the firmware boot process.
>>>>>     >
>>>>>     > Thomas Huth (5):
>>>>>     >   nvram: Introduce helper functions for CHRP "system" and "free 
>>>>> space"
>>>>>     >     partitions
>>>>>     >   sparc: Use the new common NVRAM functions for system and free 
>>>>> space
>>>>>     >     partition
>>>>>     >   spapr_nvram: Pre-initialize the NVRAM to support the -prom-env
>>>>>     >     parameter
>>>>>     >   nvram: Move the remaining CHRP NVRAM related code to 
>>>>> chrp_nvram.[ch]
>>>>>     >   nvram: Rename openbios_firmware_abi.h into sun_nvram.h
>>>>>     >
>>>>>     >  hw/nvram/Makefile.objs                             |  1 +
>>>>>     >  hw/nvram/chrp_nvram.c                              | 85
>>>>>     ++++++++++++++++++++++
>>>>>     >  hw/nvram/mac_nvram.c                               | 49 
>>>>> +++----------
>>>>>     >  hw/nvram/spapr_nvram.c                             |  6 ++
>>>>>     >  hw/sparc/sun4m.c                                   | 35 ++-------
>>>>>     >  hw/sparc64/sun4u.c                                 | 35 ++-------
>>>>>     >  include/hw/nvram/chrp_nvram.h                      | 54
>>>>>     ++++++++++++++
>>>>>     >  .../nvram/{openbios_firmware_abi.h => sun_nvram.h} | 47 
>>>>> +-----------
>>>>>     >  tests/postcopy-test.c                              |  8 +-
>>>>>     >  9 files changed, 179 insertions(+), 141 deletions(-)
>>>>>     >  create mode 100644 hw/nvram/chrp_nvram.c
>>>>>     >  create mode 100644 include/hw/nvram/chrp_nvram.h
>>>>>     >  rename include/hw/nvram/{openbios_firmware_abi.h => sun_nvram.h}
>>>>>     (50%)
>>>>>
>>>>>     Series,
>>>>>
>>>>>     Reviewed-by: David Gibson <address@hidden
>>>>>     <mailto:address@hidden>>
>>>>>
>>>>>     I've put it into ppc-for-2.8 tentatively.  However I'd like to get an
>>>>>     Acked-by from Mark for the Sparc bits before I send my next pull
>>>>>     request.
>>>>>
>>>>>
>>>>> I observe an early boot failure in SLOF with a commit from this patchset
>>>>> on ppc-for-2.8 branch.
>>>>>
>>>>> 4e1257ed41bce16baa8a010 - spapr_nvram: Pre-initialize the NVRAM to
>>>>> support the -prom-env parameter
>>>>>
>>>>> SLOF 
>>>>> **********************************************************************
>>>>> QEMU Starting
>>>>>  Build Date = Oct 19 2016 09:58:38
>>>>>  FW Version = git-efd65f49929d7db7
>>>>>  Press "s" to enter Open Firmware.
>>>>>
>>>>> Populating /vdevice methods
>>>>> Populating /vdevice/address@hidden
>>>>> Populating /vdevice/address@hidden
>>>>> Populating /vdevice/address@hidden
>>>>>        SCSI: Looking for devices
>>>>>           8200000000000000 CD-ROM   : "QEMU     QEMU CD-ROM      2.5+"
>>>>> Populating /address@hidden
>>>>>                      00 1000 (D) : 1033 0194    serial bus [ usb-xhci ]
>>>>>                      00 0800 (D) : 1af4 1001    virtio [ block ]
>>>>>                      00 0000 (D) : 1af4 1000    virtio [ net ]
>>>>> Scanning USB
>>>>>   XHCI: Initializing
>>>>> Using default console: /vdevice/address@hidden
>>>>>     
>>>>>   Welcome to Open Firmware
>>>>>
>>>>>   Copyright (c) 2004, 2011 IBM Corporation All rights reserved.
>>>>>   This program and the accompanying materials are made available
>>>>>   under the terms of the BSD License available at
>>>>>   http://www.opensource.org/licenses/bsd-license.php
>>>>>
>>>>>
>>>>> Trying to load:  from: /address@hidden/address@hidden ...   Successfully 
>>>>> loaded
>>>>> error: out of memory.
>>>>> out of memory
>>>>> Aborted. Press any key to exit.
>>>>
>>>> Yuck. Confirmed. Sorry for the inconvenience. Seems like SLOF does not
>>>> create the properties in the /options device tree node anymore in this 
>>>> case.
>>>>
>>>> David, could you please unqueue the "spapr_nvram: Pre-initialize the
>>>> NVRAM to support the -prom-env parameter" patch from the ppc-for-2.8
>>>> branch until I figure out a fix for this problem? Thanks!
>>>
>>> Done.
>>
>> FYI, SLOF patch to fix this issue is on the way:
>> https://patchwork.ozlabs.org/patch/686426/
> 
> Ok.  So it was a matter of this change exposing a bug in SLOF, rather
> than being buggy of itself?

Yes. SLOF should always keep the /options device tree node populated and
up to date, no matter whether a variable is available in the NVRAM
partition or not. (Well, not sure, but it could maybe also have exposed
a bug in grub2 since that simply does not seem to work if the variable
"real-mode?" is not there ... but grub2 seems to work fine if it's
there, no matter if I use -prom-env "real-mode?=false" or -prom-env
"real-mode?=true" ...)

Anyway, I think it might also be better to change this QEMU patch here a
little bit, so that the NVRAM only gets pre-initialized if the user
specified a "-prom-env" parameter. That means, for the normal use-case,
the behavior will stay the same, and we do not have to pull in another
SLOF update to fix this issue. I'll rework my patch and post a new
version when it's ready.

 Thomas

signature.asc
Description: OpenPGP digital signature