[libvirt PATCH v2 0/3] qemu: support the SeaBIOS/EDK2 debug console
# virsh dumpxml fedora34x86_64 | xmllint -xpath '/domain/devices/console[2]' - <serial type="pty"> <target type="isa-debug"> <model type="isa-debugcon"/> </target> <address type="isa" iobase="0x402"/> </serial> # virsh console --devname console1 fedora34x86_64 Of course you really want to start the guest paused initially to allow time to connect to the console before resuming CPUs, and thus be able to catch early firmware output. In v2: - Use <serial> instead of <console> Daniel P. Berrangé (3): conf: validate serial port model in ABI checks conf: support firmware ISA debug console qemu: add tests for the ISA debug console command line docs/formatdomain.rst | 14 +++--- docs/schemas/domaincommon.rng | 2 + src/conf/domain_conf.c | 26 +++++++++-- src/conf/domain_conf.h | 2 + src/qemu/qemu_command.c | 2 + src/qemu/qemu_domain.c | 4 ++ src/qemu/qemu_domain_address.c | 1 + src/qemu/qemu_validate.c | 5 +++ .../serial-debugcon.x86_64-latest.args | 39 +++++++++++++++++ tests/qemuxml2argvdata/serial-debugcon.xml | 29 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/serial-debugcon.xml | 43 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 13 files changed, 161 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/serial-debugcon.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/serial-debugcon.xml create mode 100644 tests/qemuxml2xmloutdata/serial-debugcon.xml -- 2.34.1
The serial port model cannot be allowed to change across migration as it affects ABI. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 58e696416d..9415ecb13b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21095,6 +21095,14 @@ virDomainSerialDefCheckABIStability(virDomainChrDef *src, return false; } + if (src->targetModel != dst->targetModel) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target serial model %s does not match source %s"), + virDomainChrSerialTargetModelTypeToString(dst->targetModel), + virDomainChrSerialTargetModelTypeToString(src->targetModel)); + return false; + } + if (src->target.port != dst->target.port) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target serial port %d does not match source %d"), -- 2.34.1
On Wed, Feb 02, 2022 at 12:44:43PM +0000, Daniel P. Berrangé wrote:
The serial port model cannot be allowed to change across migration as it affects ABI.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Introduce support for <serial type='pty'> <target type='isa-debug'> <model type='isa-debugcon'/> </target> <address type='isa' iobase='0x402'/> </console> which is used as a way to receive debug messages from the firmware on x86 platforms. Note that the default port is 0x0xe9 since that's the original Bochs debug port. Thus for use with SeaBIOS/OVMF, the iobase port needs to be explicitly set to 0x402. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatdomain.rst | 14 +++++++++----- docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 18 +++++++++++++++--- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 2 ++ src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_domain_address.c | 1 + src/qemu/qemu_validate.c | 5 +++++ 8 files changed, 40 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index e2f99c60a6..8fa5940469 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6418,8 +6418,9 @@ values are, :since:`since 1.0.2` , ``isa-serial`` (usable with x86 guests), ``usb-serial`` (usable whenever USB support is available) and ``pci-serial`` (usable whenever PCI support is available); :since:`since 3.10.0` , ``spapr-vio-serial`` (usable with ppc64/pseries guests), ``system-serial`` -(usable with aarch64/virt and, :since:`since 4.7.0` , riscv/virt guests) and -``sclp-serial`` (usable with s390 and s390x guests) are available as well. +(usable with aarch64/virt and, :since:`since 4.7.0` , riscv/virt guests), +``sclp-serial`` (usable with s390 and s390x guests) are available as well +and :since:`since 8.1.0` ``isa-debug`` (usable with x86 guests). :since:`Since 3.10.0` , the ``target`` element can have an optional ``model`` subelement; valid values for its ``name`` attribute are: ``isa-serial`` (usable @@ -6428,9 +6429,12 @@ with the ``isa-serial`` target type); ``usb-serial`` (usable with the target type); ``spapr-vty`` (usable with the ``spapr-vio-serial`` target type); ``pl011`` and, :since:`since 4.7.0` , ``16550a`` (usable with the ``system-serial`` target type); ``sclpconsole`` and ``sclplmconsole`` (usable -with the ``sclp-serial`` target type). Providing a target model is usually -unnecessary: libvirt will automatically pick one that's suitable for the chosen -target type, and overriding that value is generally not recommended. +with the ``sclp-serial`` target type). ``isa-debugcon`` (usable with the +``isa-debug`` target type); provides a virtual console for receiving debug +messages from the firmware on x86 platforms. :since:`Since: 8.1.0`. +Providing a target model is usually unnecessary: libvirt will automatically +pick one that's suitable for the chosen target type, and overriding that +value is generally not recommended. If any of the attributes is not specified by the user, libvirt will choose a value suitable for most users. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 64a797de46..964b0c9e2f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4401,6 +4401,7 @@ <value>spapr-vio-serial</value> <value>system-serial</value> <value>sclp-serial</value> + <value>isa-debug</value> </choice> </attribute> </define> @@ -4417,6 +4418,7 @@ <value>16550a</value> <value>sclpconsole</value> <value>sclplmconsole</value> + <value>isa-debugcon</value> </choice> </attribute> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9415ecb13b..3266dd0412 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -652,6 +652,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTarget, "spapr-vio-serial", "system-serial", "sclp-serial", + "isa-debug", ); VIR_ENUM_IMPL(virDomainChrChannelTarget, @@ -686,6 +687,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTargetModel, "sclpconsole", "sclplmconsole", "16550a", + "isa-debugcon", ); VIR_ENUM_IMPL(virDomainChrDevice, @@ -4953,6 +4955,7 @@ virDomainDefAddConsoleCompat(virDomainDef *def) case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: /* Nothing to do */ break; @@ -5397,7 +5400,7 @@ virDomainChrIsaSerialDefPostParse(virDomainDef *def) } -static void +static int virDomainChrDefPostParse(virDomainChrDef *chr, const virDomainDef *def) { @@ -5411,6 +5414,14 @@ virDomainChrDefPostParse(virDomainChrDef *chr, chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; } + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && + chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("isa-debug serial type only valid on x86 architecture")); + return -1; + } + if (chr->target.port == -1 && (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL || chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL || @@ -5424,6 +5435,8 @@ virDomainChrDefPostParse(virDomainChrDef *chr, chr->target.port = maxport + 1; } + + return 0; } @@ -5635,8 +5648,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDef *dev, switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_CHR: - virDomainChrDefPostParse(dev->data.chr, def); - ret = 0; + ret = virDomainChrDefPostParse(dev->data.chr, def); break; case VIR_DOMAIN_DEVICE_RNG: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0731007355..b2922e8cff 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1161,6 +1161,7 @@ typedef enum { VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO, VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM, VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP, + VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG, VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST } virDomainChrSerialTargetType; @@ -1204,6 +1205,7 @@ typedef enum { VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE, VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE, VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A, + VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON, VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST } virDomainChrSerialTargetModel; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fc778901d1..c29543396a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9351,6 +9351,7 @@ qemuChrSerialTargetModelToCaps(virDomainChrSerialTargetModel targetModel) case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011: return QEMU_CAPS_DEVICE_PL011; case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A: + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST: break; @@ -10762,6 +10763,7 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def, case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE: + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON: caps = qemuChrSerialTargetModelToCaps(serial->targetModel); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6b915d7535..647bb8839c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5196,6 +5196,9 @@ qemuDomainChrDefPostParse(virDomainChrDef *chr, case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP: chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE; break; + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: + chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON; + break; case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: /* Nothing to do */ @@ -6203,6 +6206,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriver *driver, case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: /* Nothing to do */ diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 3e6eed6ec9..13aad4fc4d 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -995,6 +995,7 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev, case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: return 0; diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 3bf39f8d93..f27e480696 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1817,6 +1817,7 @@ qemuValidateChrSerialTargetTypeToAddressType(int targetType) { switch ((virDomainChrSerialTargetType)targetType) { case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: return VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA; case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: return VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB; @@ -1853,6 +1854,8 @@ qemuValidateChrSerialTargetModelToTargetType(int targetModel) case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE: return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP; + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON: + return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG; case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST: break; @@ -1876,6 +1879,7 @@ qemuValidateDomainChrTargetDef(const virDomainChrDef *chr) case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA_DEBUG: expected = qemuValidateChrSerialTargetTypeToAddressType(chr->targetType); @@ -1915,6 +1919,7 @@ qemuValidateDomainChrTargetDef(const virDomainChrDef *chr) case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE: case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A: + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_DEBUGCON: expected = qemuValidateChrSerialTargetModelToTargetType(chr->targetModel); -- 2.34.1
On Wed, Feb 02, 2022 at 12:44:44PM +0000, Daniel P. Berrangé wrote:
Introduce support for
<serial type='pty'> <target type='isa-debug'> <model type='isa-debugcon'/> </target> <address type='isa' iobase='0x402'/> </console>
which is used as a way to receive debug messages from the firmware on x86 platforms.
Note that the default port is 0x0xe9 since that's the original Bochs debug port. Thus for use with SeaBIOS/OVMF, the iobase port needs to be explicitly set to 0x402.
The default is not set anywhere in the code, so I guess we're relying on the fact that QEMU has that as the current default. Wouldn't it make sense to have it reflected in the XML as well? The previous version used 0x402 as the default, which apparently doesn't match QEMU's own but is what most people will want in practice. Wouldn't that be a better default for us? People who care about Bochs compatibility can always explicitly set the appropriate value themselves. The rest of the changes look good. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 02, 2022 at 06:29:32AM -0800, Andrea Bolognani wrote:
On Wed, Feb 02, 2022 at 12:44:44PM +0000, Daniel P. Berrangé wrote:
Introduce support for
<serial type='pty'> <target type='isa-debug'> <model type='isa-debugcon'/> </target> <address type='isa' iobase='0x402'/> </console>
which is used as a way to receive debug messages from the firmware on x86 platforms.
Note that the default port is 0x0xe9 since that's the original Bochs debug port. Thus for use with SeaBIOS/OVMF, the iobase port needs to be explicitly set to 0x402.
The default is not set anywhere in the code, so I guess we're relying on the fact that QEMU has that as the current default. Wouldn't it make sense to have it reflected in the XML as well?
The previous version used 0x402 as the default, which apparently doesn't match QEMU's own but is what most people will want in practice. Wouldn't that be a better default for us? People who care about Bochs compatibility can always explicitly set the appropriate value themselves.
The previous bversion was a mistake because I incorrectly thought 402 was QEMU's own default. I also mistakenly thought we needed to set it explicitly for ABI compat, but that's not the case because the machine type ensures that. So preferrable to not override QEMU's own defaults in libvirt. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Feb 02, 2022 at 02:32:32PM +0000, Daniel P. Berrangé wrote:
On Wed, Feb 02, 2022 at 06:29:32AM -0800, Andrea Bolognani wrote:
On Wed, Feb 02, 2022 at 12:44:44PM +0000, Daniel P. Berrangé wrote:
Introduce support for
<serial type='pty'> <target type='isa-debug'> <model type='isa-debugcon'/> </target> <address type='isa' iobase='0x402'/> </console>
which is used as a way to receive debug messages from the firmware on x86 platforms.
Note that the default port is 0x0xe9 since that's the original Bochs debug port. Thus for use with SeaBIOS/OVMF, the iobase port needs to be explicitly set to 0x402.
The default is not set anywhere in the code, so I guess we're relying on the fact that QEMU has that as the current default. Wouldn't it make sense to have it reflected in the XML as well?
The previous version used 0x402 as the default, which apparently doesn't match QEMU's own but is what most people will want in practice. Wouldn't that be a better default for us? People who care about Bochs compatibility can always explicitly set the appropriate value themselves.
The previous bversion was a mistake because I incorrectly thought 402 was QEMU's own default. I also mistakenly thought we needed to set it explicitly for ABI compat, but that's not the case because the machine type ensures that.
So preferrable to not override QEMU's own defaults in libvirt.
I think it would still make sense to reflect QEMU's current default in the XML, which would make sure that the same input XML results in the same device even if QEMU decides to change its defaults later on. If you don't agree with that idea you can consider this patch Reviewed-by: Andrea Bolognani <abologna@redhat.com> but please at least reword the last bit of the commit message along the lines of Note that if iobase is not specified the result is going to be hypervisor-specific: QEMU currently defaults to 0xe9, which is the original Bochs debug port, thus for use with... to make it clear that you're talking about QEMU's own default and not libvirt's. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 02, 2022 at 06:46:25AM -0800, Andrea Bolognani wrote:
On Wed, Feb 02, 2022 at 02:32:32PM +0000, Daniel P. Berrangé wrote:
On Wed, Feb 02, 2022 at 06:29:32AM -0800, Andrea Bolognani wrote:
On Wed, Feb 02, 2022 at 12:44:44PM +0000, Daniel P. Berrangé wrote:
Introduce support for
<serial type='pty'> <target type='isa-debug'> <model type='isa-debugcon'/> </target> <address type='isa' iobase='0x402'/> </console>
which is used as a way to receive debug messages from the firmware on x86 platforms.
Note that the default port is 0x0xe9 since that's the original Bochs debug port. Thus for use with SeaBIOS/OVMF, the iobase port needs to be explicitly set to 0x402.
The default is not set anywhere in the code, so I guess we're relying on the fact that QEMU has that as the current default. Wouldn't it make sense to have it reflected in the XML as well?
The previous version used 0x402 as the default, which apparently doesn't match QEMU's own but is what most people will want in practice. Wouldn't that be a better default for us? People who care about Bochs compatibility can always explicitly set the appropriate value themselves.
The previous bversion was a mistake because I incorrectly thought 402 was QEMU's own default. I also mistakenly thought we needed to set it explicitly for ABI compat, but that's not the case because the machine type ensures that.
So preferrable to not override QEMU's own defaults in libvirt.
I think it would still make sense to reflect QEMU's current default in the XML, which would make sure that the same input XML results in the same device even if QEMU decides to change its defaults later on.
We don't do that for any existing ISA devices AFAICT, so I'm not finding a compelling reason to treat isa-debugcon as special. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Feb 02, 2022 at 03:38:59PM +0000, Daniel P. Berrangé wrote:
On Wed, Feb 02, 2022 at 06:46:25AM -0800, Andrea Bolognani wrote:
I think it would still make sense to reflect QEMU's current default in the XML, which would make sure that the same input XML results in the same device even if QEMU decides to change its defaults later on.
We don't do that for any existing ISA devices AFAICT, so I'm not finding a compelling reason to treat isa-debugcon as special.
Go ahead with the patches as they are then. Maybe I will try to implement this idea for all ISA devices at some point :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 02, 2022 at 07:47:37AM -0800, Andrea Bolognani wrote:
On Wed, Feb 02, 2022 at 03:38:59PM +0000, Daniel P. Berrangé wrote:
On Wed, Feb 02, 2022 at 06:46:25AM -0800, Andrea Bolognani wrote:
I think it would still make sense to reflect QEMU's current default in the XML, which would make sure that the same input XML results in the same device even if QEMU decides to change its defaults later on.
We don't do that for any existing ISA devices AFAICT, so I'm not finding a compelling reason to treat isa-debugcon as special.
Go ahead with the patches as they are then. Maybe I will try to implement this idea for all ISA devices at some point :)
FWIW the reason it is important for PCI of course is that PCI is highly dynamic at runtime with ability to hotplug/unplug, which means addresses aren't predictable in the next launch unless you fixate them in libvirt upfront. For ISA being entirely statically defined at cold boot, the only think that can affect the address information (IRQ, IO Port) on next boot is a change in QEMU machine type, which is already solved by versioning them. If the argument is "the machine type could change and we should not let that influence guest future boots" then that means we shouldn't have machine types at all, and be explicitly representing every possible hardware property a machine type implies instead. IMHO the only benefit from exposing the ISA address defaults in the XML is as a means of transparency for users, so they can gain insight into the hardware defaults. Effectively libvirt would be documenting QEMU's machine type in doing that. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/2/22 13:44, Daniel P. Berrangé wrote:
Introduce support for
<serial type='pty'> <target type='isa-debug'> <model type='isa-debugcon'/> </target> <address type='isa' iobase='0x402'/> </console>
which is used as a way to receive debug messages from the firmware on x86 platforms.
Note that the default port is 0x0xe9 since that's the original
nipick: s/0x//
Bochs debug port. Thus for use with SeaBIOS/OVMF, the iobase port needs to be explicitly set to 0x402.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatdomain.rst | 14 +++++++++----- docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 18 +++++++++++++++--- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 2 ++ src/qemu/qemu_domain.c | 4 ++++ src/qemu/qemu_domain_address.c | 1 + src/qemu/qemu_validate.c | 5 +++++ 8 files changed, 40 insertions(+), 8 deletions(-)
Michal
The XML-to-XML test validates that we don't accidentally copy the isa-debug <serial> into a <console>. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .../serial-debugcon.x86_64-latest.args | 39 +++++++++++++++++ tests/qemuxml2argvdata/serial-debugcon.xml | 29 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmloutdata/serial-debugcon.xml | 43 +++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 113 insertions(+) create mode 100644 tests/qemuxml2argvdata/serial-debugcon.x86_64-latest.args create mode 100644 tests/qemuxml2argvdata/serial-debugcon.xml create mode 100644 tests/qemuxml2xmloutdata/serial-debugcon.xml diff --git a/tests/qemuxml2argvdata/serial-debugcon.x86_64-latest.args b/tests/qemuxml2argvdata/serial-debugcon.x86_64-latest.args new file mode 100644 index 0000000000..4e026e7afa --- /dev/null +++ b/tests/qemuxml2argvdata/serial-debugcon.x86_64-latest.args @@ -0,0 +1,39 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram \ +-accel tcg \ +-cpu qemu64 \ +-m 214 \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-device '{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \ +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \ +-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-format","id":"ide0-0-0","bootindex":1}' \ +-chardev pty,id=charserial0 \ +-device '{"driver":"isa-debugcon","chardev":"charserial0","id":"serial0","iobase":1026}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-device '{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/serial-debugcon.xml b/tests/qemuxml2argvdata/serial-debugcon.xml new file mode 100644 index 0000000000..a41be242d2 --- /dev/null +++ b/tests/qemuxml2argvdata/serial-debugcon.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <serial type='pty'> + <target type='isa-debug'> + <model name='isa-debugcon'/> + </target> + <address type='isa' iobase='0x402'/> + </serial> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 96d30f2475..b9d8885912 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1717,6 +1717,7 @@ mymain(void) DO_TEST_CAPS_LATEST("serial-file-log"); DO_TEST_CAPS_LATEST("serial-spiceport"); DO_TEST_CAPS_LATEST("serial-spiceport-nospice"); + DO_TEST_CAPS_LATEST("serial-debugcon"); DO_TEST_CAPS_LATEST("console-compat"); DO_TEST_CAPS_LATEST("console-compat-auto"); diff --git a/tests/qemuxml2xmloutdata/serial-debugcon.xml b/tests/qemuxml2xmloutdata/serial-debugcon.xml new file mode 100644 index 0000000000..6b6126fed1 --- /dev/null +++ b/tests/qemuxml2xmloutdata/serial-debugcon.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <serial type='pty'> + <target type='isa-debug' port='0'> + <model name='isa-debugcon'/> + </target> + <address type='isa' iobase='0x402'/> + </serial> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 935fd955f4..007c9edacd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -497,6 +497,7 @@ mymain(void) cfg->spiceTLS = false; DO_TEST_NOCAPS("serial-spiceport-nospice"); + DO_TEST_NOCAPS("serial-debugcon"); DO_TEST_NOCAPS("console-compat"); DO_TEST_NOCAPS("console-compat2"); DO_TEST_NOCAPS("console-virtio-many"); -- 2.34.1
On Wed, Feb 02, 2022 at 12:44:45PM +0000, Daniel P. Berrangé wrote:
+++ b/tests/qemuxml2argvdata/serial-debugcon.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk>
The <disk> part is irrelevant to the scenario we're testing here and can be safely dropped.
+ <serial type='pty'> + <target type='isa-debug'> + <model name='isa-debugcon'/> + </target>
Turning this into <target type='isa-debug'/> would ensure that <model> is filled in automatically with the correct value. Whether or not you decide do apply the changes suggested above Reviewed-by: Andrea Bolognani <abologna@redhat.com> I think this new feature warrants an entry in the release notes. Feel free to post that as a follow-up :) -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Prívozník