[SLOF] [PATCH] slof: Only close stdout for virtio-serial devices

[Alexey Kardashevskiy]

On 26/03/2020 21:32, Greg Kurz wrote:
> Recent commit cf28264196e5 fixed an issue where a virtio-serial device
> wouldn't shutdown properly during quiesce. The fix is to close stdout
> just before quiesce. As expected this causes some messages to not
> appear anymore, like the well known ones from prom_init():
> 
> Quiescing Open Firmware ...
> Booting Linux via __start() @ 0x0000000002000000 ...
> 
> Actually all messages are discarded until the OS driver finally takes
> control of the device, which may represent a fair amount of logging.
> This is suboptimal but this still better than hanging in SLOF.
> 
> The hammer is a bit too big though because the change also affects
> spapr-vty based consoles, which have no reason to stop working
> after quiesce.
> 
> Move the hack from the common code to the virtio-serial code so that
> it doesn't affect other device types anymore. Register a quiesce hook
> that closes stdout in virtio-serial.fs.
> 
> While here, as suggested by Segher, bring back some robustness in the
> shutdown method.
> 
> Reported-by: Fabiano Rosas <farosas at linux.ibm.com>
> Fixes: cf28264196e5 "virtio-serial: Rework shutdown sequence"
> Signed-off-by: Greg Kurz <groug at kaod.org>
> ---
> 
> David,
> 
> Loosing so much output with spapr-vty is an annoying regression IMHO.
> I really thing we should have this fixed in QEMU 5.0.



Thanks, applied.


I'll send a pull request shortly but to be fair only two lines with ">"
are missing without the patch in this example:

===
instantiating rtas at 0x000000002fff0000... done
prom_hold_cpus: skipped
copying OF device tree...
Building dt strings...
Building dt structure...
Device tree strings 0x000000000b030000 -> 0x000000000b030a2c
Device tree struct  0x000000000b040000 -> 0x000000000b050000
>Quiescing Open Firmware ...
>Booting Linux via __start() @ 0x0000000000400000 ...
 -> fw_cmo_feature_init()
CMO not available
 <- fw_cmo_feature_init()
 <- pseries_init()
===

Thanks,


> ---
>  board-qemu/slof/virtio-serial.fs |   14 +++++++++++---
>  slof/fs/client.fs                |    5 -----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs
> index ac55ffcc8ebb..06bfe762b105 100644
> --- a/board-qemu/slof/virtio-serial.fs
> +++ b/board-qemu/slof/virtio-serial.fs
> @@ -19,9 +19,11 @@ virtio-setup-vd VALUE virtiodev
>  \ Quiescence the virtqueue of this device so that no more background
>  \ transactions can be pending.
>  : shutdown  ( -- )
> -    virtiodev virtio-serial-shutdown
> -    FALSE to initialized?
> -    0 to virtiodev
> +    initialized? IF
> +        virtiodev virtio-serial-shutdown
> +        FALSE to initialized?
> +        0 to virtiodev
> +    THEN
>  ;
>  
>  : virtio-serial-term-emit
> @@ -31,10 +33,16 @@ virtio-setup-vd VALUE virtiodev
>  : virtio-serial-term-key?  virtiodev virtio-serial-haschar ;
>  : virtio-serial-term-key   BEGIN virtio-serial-term-key? UNTIL virtiodev virtio-serial-getchar ;
>  
> +: virtio-serial-close-stdout s" stdout" get-chosen IF decode-int nip nip close-dev THEN ;
> +
>  \ Basic device initialization - which has only to be done once
>  : init  ( -- )
>  virtiodev virtio-serial-init drop
>      TRUE to initialized?
> +    \ Linux closes stdin at some point in prom_init(). This internally triggers a
> +    \ quiesce in SLOF. We must ensure stdout gets closed as well otherwise the
> +    \ device cannot be reset properly and the boot will hang.
> +    ['] virtio-serial-close-stdout add-quiesce-xt
>  ;
>  
>  0 VALUE open-count
> diff --git a/slof/fs/client.fs b/slof/fs/client.fs
> index 76231f9aef75..db7a1925792c 100644
> --- a/slof/fs/client.fs
> +++ b/slof/fs/client.fs
> @@ -203,11 +203,6 @@ ALSO client-voc DEFINITIONS
>  	    \ End of life of SLOF now, call platform quiesce as quiesce
>  	    \ is an undocumented extension and not everybody supports it
>  	    close-dev
> -	    \ Some device, eg. virtio-serial, need all instances to be
> -	    \ closed in order to be reset properly
> -	    s" stdout" get-chosen IF
> -		decode-int nip nip close-dev
> -	    THEN
>  	    quiesce
>  	ELSE
>  	    close-dev
> 

-- 
Alexey