hw/ppc/spapr.c | 12 ++++++++---- include/hw/ppc/spapr.h | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-)
-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
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) */
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
© 2016 - 2024 Red Hat, Inc.