[SLOF] [PATCH v2 2/2] virtio-serial: Do not close stdout on quiesce

[Jordan Niethe]

On 29/8/23 5:07 pm, Thomas Huth wrote:
> On 29/08/2023 02.12, Jordan Niethe wrote:
>> Commit 76fee95 ("slof: Only close stdout for virtio-serial devices")
>> says that commit cf28264 ("virtio-serial: Rework shutdown sequence")
>> fixed a hang. The problem was believed to be that it was necessary to
>> close stdout to shutdown the underlying virtio device.
>>
>> Commit cf28264 ("virtio-serial: Rework shutdown sequence") closed stdout
>> on quiesce. This meant when prom_init() called write on stdout after
>> quiesce, there is a use after free so this is unreliable, and can also
>> hang (especially after reboots).
>>
>> Quiescing is intended to put hardware into a safe state for the client
>> to take over. It is incorrect for SLOF to close ihandles that the client
>> could still be using, even after a quiesce.
>>
>> Rather than closing the stdout device, all that needs to happen is to
>> ensure virtio-serial-shutdown gets called. On quiesce, close the virtio
>> device, but leave the stdout device itself open.
>>
>> Commit 8174acd ("virtio-serial: Close device completely") handles reads
>> and writes as no-ops if the underlying virtio device is closed so there
>> is no problem with the client calling "write" on stdout after this, but
>> no output will be displayed.
>>
>> Fixes: cf28264 ("virtio-serial: Rework shutdown sequence")
>> Debugged-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
>> Co-developed-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
>> Signed-off-by: Kautuk Consul <kconsul at linux.vnet.ibm.com>
>> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
>> ---
>> v2:
>>     - Reword init comment
>> ---
>>   board-qemu/slof/virtio-serial.fs | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/board-qemu/slof/virtio-serial.fs 
>> b/board-qemu/slof/virtio-serial.fs
>> index 41e2e04..de42cc7 100644
>> --- a/board-qemu/slof/virtio-serial.fs
>> +++ b/board-qemu/slof/virtio-serial.fs
>> @@ -33,16 +33,14 @@ 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
>> +    \ virtiodev must be shutdown at quiesce so the device is reset 
>> properly.
>> +    \ The read and write methods can be called after quiesce so must 
>> handle
>> +    \ virtiodev being closed.
>> +    ['] shutdown add-quiesce-xt
>>   ;
>>   0 VALUE open-count
>> @@ -62,8 +60,8 @@ virtiodev virtio-serial-init drop
>>       open-count 0> IF
>>           open-count 1 - dup to open-count
>>           0= IF shutdown THEN
>> +        close
>>       THEN
>> -    close
>>   ;
>>   : write ( addr len -- actual )
> 
> Reviewed-by: Thomas Huth <thuth at redhat.com>


Thanks for reviewing.


>