[PATCH] pseries: allow setting stdout-path even on machines with a VGA

[PATCH] pseries: allow setting stdout-path even on machines with a VGA

[Paolo Bonzini]

-machine graphics=off is the usual way to tell the firmware or the OS that the
user wants a serial console.  The pseries machine however does not support
this, and never adds the stdout-path node to the device tree if a VGA device
is provided.  This is in addition to the other magic behavior of VGA devices,
which is to add a keyboard and mouse to the default USB bus.

Split spapr->has_graphics in two variables so that the two behaviors can be
separated: the USB devices remains the same, but the stdout-path is added
even with "-device VGA -machine graphics=off".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ppc/spapr.c         | 12 ++++++++----
 include/hw/ppc/spapr.h |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 22569305d2..05eacf29b8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1066,7 +1066,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
             _FDT(fdt_setprop_string(fdt, chosen, "qemu,boot-device", boot_device));
         }
 
-        if (!spapr->has_graphics && stdout_path) {
+        if (!spapr->want_stdout_path && stdout_path) {
             /*
              * "linux,stdout-path" and "stdout" properties are
              * deprecated by linux kernel. New platforms should only
@@ -2711,6 +2711,7 @@ static void spapr_machine_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
     PCIHostState *phb;
+    bool has_vga;
     int i;
     MemoryRegion *sysmem = get_system_memory();
     long load_limit, fw_size;
@@ -2949,9 +2950,12 @@ static void spapr_machine_init(MachineState *machine)
     }
 
     /* Graphics */
-    if (spapr_vga_init(phb->bus, &error_fatal)) {
-        spapr->has_graphics = true;
+    has_vga = spapr_vga_init(phb->bus, &error_fatal);
+    if (has_vga) {
+        spapr->want_stdout_path = !machine->enable_graphics;
         machine->usb |= defaults_enabled() && !machine->usb_disabled;
+    } else {
+        spapr->want_stdout_path = true;
     }
 
     if (machine->usb) {
@@ -2961,7 +2965,7 @@ static void spapr_machine_init(MachineState *machine)
             pci_create_simple(phb->bus, -1, "nec-usb-xhci");
         }
 
-        if (spapr->has_graphics) {
+        if (has_vga) {
             USBBus *usb_bus = usb_bus_find(-1);
 
             usb_create_simple(usb_bus, "usb-kbd");
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 14b01c3f59..072dda2c72 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -194,7 +194,7 @@ struct SpaprMachineState {
     Vof *vof;
     uint64_t rtc_offset; /* Now used only during incoming migration */
     struct PPCTimebase tb;
-    bool has_graphics;
+    bool want_stdout_path;
     uint32_t vsmt;       /* Virtual SMT mode (KVM's "core stride") */
 
     /* Nested HV support (TCG only) */
-- 
2.35.1

Re: [PATCH] pseries: allow setting stdout-path even on machines with a VGA

[Daniel Henrique Barboza]

On 5/5/22 06:26, Paolo Bonzini wrote:
> -machine graphics=off is the usual way to tell the firmware or the OS that the
> user wants a serial console.  The pseries machine however does not support
> this, and never adds the stdout-path node to the device tree if a VGA device
> is provided.  This is in addition to the other magic behavior of VGA devices,
> which is to add a keyboard and mouse to the default USB bus.
> 
> Split spapr->has_graphics in two variables so that the two behaviors can be
> separated: the USB devices remains the same, but the stdout-path is added
> even with "-device VGA -machine graphics=off".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/ppc/spapr.c         | 12 ++++++++----
>   include/hw/ppc/spapr.h |  2 +-
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 22569305d2..05eacf29b8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1066,7 +1066,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
>               _FDT(fdt_setprop_string(fdt, chosen, "qemu,boot-device", boot_device));
>           }
>   
> -        if (!spapr->has_graphics && stdout_path) {
> +        if (!spapr->want_stdout_path && stdout_path) {

hmmmmm I don't think this is correct. You defined 'spapr->want_stdout_path' in
spapr_machine_init() to be true if (1) there is no VGA or (2) graphics=off. But
then you're writing the DT at this point by checking !spapr->want_stdout_path.

I guess this is doing the opposite of what you intended. In fact, with this patch
as is, with this test case:


$ ./qemu-system-ppc64 -machine pseries --nodefaults \
-chardev pty,id=charserial0 \
-device spapr-vty,chardev=charserial0,id=serial0,reg=0x70000000 \
-machine dumpdtb=fdt.dtb \
-device VGA -machine graphics=off
char device redirected to /dev/pts/6 (label charserial0)
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ccf-assist=on
qemu-system-ppc64: info: dtb dumped to fdt.dtb. Exiting.

$ dtc -q -I dtb -O dts fdt.dtb | grep stdout
$


'stdout-path' isn't being written using -machine graphics=off.


Removing the negation seems to do what you want:

-        if (!spapr->want_stdout_path && stdout_path) {
+        if (spapr->want_stdout_path && stdout_path) {



$ ./qemu-system-ppc64 -machine pseries --nodefaults \
-chardev pty,id=charserial0 \
-device spapr-vty,chardev=charserial0,id=serial0,reg=0x70000000 \
-machine dumpdtb=fdt.dtb \
-device VGA -machine graphics=off
char device redirected to /dev/pts/6 (label charserial0)
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-cfpc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-sbbc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ibs=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature, cap-ccf-assist=on
qemu-system-ppc64: info: dtb dumped to fdt.dtb. Exiting.
$ dtc -q -I dtb -O dts fdt.dtb | grep stdout
		stdout-path = "/vdevice/vty@70000000";
		linux,stdout-path = "/vdevice/vty@70000000";



Thanks,


Daniel


>               /*
>                * "linux,stdout-path" and "stdout" properties are
>                * deprecated by linux kernel. New platforms should only
> @@ -2711,6 +2711,7 @@ static void spapr_machine_init(MachineState *machine)
>       const char *kernel_filename = machine->kernel_filename;
>       const char *initrd_filename = machine->initrd_filename;
>       PCIHostState *phb;
> +    bool has_vga;
>       int i;
>       MemoryRegion *sysmem = get_system_memory();
>       long load_limit, fw_size;
> @@ -2949,9 +2950,12 @@ static void spapr_machine_init(MachineState *machine)
>       }
>   
>       /* Graphics */
> -    if (spapr_vga_init(phb->bus, &error_fatal)) {
> -        spapr->has_graphics = true;
> +    has_vga = spapr_vga_init(phb->bus, &error_fatal);
> +    if (has_vga) {
> +        spapr->want_stdout_path = !machine->enable_graphics;
>           machine->usb |= defaults_enabled() && !machine->usb_disabled;
> +    } else {
> +        spapr->want_stdout_path = true;
>       }
>   
>       if (machine->usb) {
> @@ -2961,7 +2965,7 @@ static void spapr_machine_init(MachineState *machine)
>               pci_create_simple(phb->bus, -1, "nec-usb-xhci");
>           }
>   
> -        if (spapr->has_graphics) {
> +        if (has_vga) {
>               USBBus *usb_bus = usb_bus_find(-1);
>   
>               usb_create_simple(usb_bus, "usb-kbd");
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 14b01c3f59..072dda2c72 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -194,7 +194,7 @@ struct SpaprMachineState {
>       Vof *vof;
>       uint64_t rtc_offset; /* Now used only during incoming migration */
>       struct PPCTimebase tb;
> -    bool has_graphics;
> +    bool want_stdout_path;
>       uint32_t vsmt;       /* Virtual SMT mode (KVM's "core stride") */
>   
>       /* Nested HV support (TCG only) */

Re: [PATCH] pseries: allow setting stdout-path even on machines with a VGA

[Paolo Bonzini]

On 5/6/22 00:10, Daniel Henrique Barboza wrote:
> 
> Removing the negation seems to do what you want:
> 
> -        if (!spapr->want_stdout_path && stdout_path) {
> +        if (spapr->want_stdout_path && stdout_path) {

Yes, of course.  Sorry!

Paolo