[libvirt] [PATCH] qemu: Introduce 16550A serial console model

Andrea Bolognani posted 1 patch 5 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180827121018.11304-1-abologna@redhat.com
Test syntax-check passed
docs/formatdomain.html.in                 | 12 +++++-----
docs/schemas/domaincommon.rng             |  1 +
src/conf/domain_conf.c                    |  1 +
src/conf/domain_conf.h                    |  1 +
src/qemu/qemu_command.c                   | 12 ++++++++++
src/qemu/qemu_domain.c                    | 27 ++++++++++++++++-------
tests/qemuxml2xmloutdata/riscv64-virt.xml |  4 +++-
7 files changed, 44 insertions(+), 14 deletions(-)
[libvirt] [PATCH] qemu: Introduce 16550A serial console model
Posted by Andrea Bolognani 5 years, 8 months ago
None of the existing models is suitable for use with
RISC-V virt guests, and we don't want information about
the serial console to be missing from the XML.

The name is based on comments in qemu/hw/riscv/virt.c:

  RISC-V machine with 16550a UART and VirtIO MMIO

and in qemu/hw/char/serial.c:

  QEMU 16550A UART emulation

along with the output of dmesg in the guest:

  Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
  10000000.uart: ttyS0 at MMIO 0x10000000 (irq = 13,
    base_baud= 230400) is a 16550A

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
CC'ing Rich mostly so that he can double-check the name
choice is sensible.

 docs/formatdomain.html.in                 | 12 +++++-----
 docs/schemas/domaincommon.rng             |  1 +
 src/conf/domain_conf.c                    |  1 +
 src/conf/domain_conf.h                    |  1 +
 src/qemu/qemu_command.c                   | 12 ++++++++++
 src/qemu/qemu_domain.c                    | 27 ++++++++++++++++-------
 tests/qemuxml2xmloutdata/riscv64-virt.xml |  4 +++-
 7 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0cbf570a13..eb619a1656 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7011,7 +7011,8 @@ qemu-kvm -net nic,model=? /dev/null
       is available) and <code>pci-serial</code> (usable whenever PCI support
       is available); <span class="since">since 3.10.0</span>,
       <code>spapr-vio-serial</code> (usable with ppc64/pseries guests),
-      <code>system-serial</code> (usable with aarch64/virt guests) and
+      <code>system-serial</code> (usable with aarch64/virt and,
+      <span class="since">since 4.7.0</span>, riscv/virt guests) and
       <code>sclp-serial</code> (usable with s390 and s390x guests) are
       available as well.
     </p>
@@ -7025,10 +7026,11 @@ qemu-kvm -net nic,model=? /dev/null
       target type); <code>pci-serial</code>
       (usable with the <code>pci-serial</code> target type);
       <code>spapr-vty</code> (usable with the <code>spapr-vio-serial</code>
-      target type); <code>pl011</code> (usable with the
-      <code>system-serial</code> target type); <code>sclpconsole</code> and
-      <code>sclplmconsole</code> (usable with the <code>sclp-serial</code>
-      target type).
+      target type); <code>pl011</code> and,
+      <span class="since">since 4.7.0</span>, <code>16550a</code> (usable
+      with the <code>system-serial</code> target type);
+      <code>sclpconsole</code> and <code>sclplmconsole</code> (usable with
+      the <code>sclp-serial</code> target type).
     </p>
 
     <p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f176538195..3796eb4b5e 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3733,6 +3733,7 @@
           <value>pci-serial</value>
           <value>spapr-vty</value>
           <value>pl011</value>
+          <value>16550a</value>
           <value>sclpconsole</value>
           <value>sclplmconsole</value>
         </choice>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bde9fef914..8af59bb4bb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -493,6 +493,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTargetModel,
               "pl011",
               "sclpconsole",
               "sclplmconsole",
+              "16550a",
 );
 
 VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c0ad072db5..8a3673361a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1133,6 +1133,7 @@ typedef enum {
     VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011,
     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_LAST
 } virDomainChrSerialTargetModel;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index fd9e58fd5d..f63e35444e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9149,6 +9149,7 @@ qemuChrSerialTargetModelToCaps(virDomainChrSerialTargetModel targetModel)
         return QEMU_CAPS_DEVICE_SCLPLMCONSOLE;
     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_NONE:
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
         break;
@@ -9189,6 +9190,16 @@ qemuChrIsPlatformDevice(const virDomainDef *def,
         }
     }
 
+    if (ARCH_IS_RISCV(def->os.arch)) {
+
+        /* 16550a (used by riscv/virt guests) is a platform device */
+        if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
+            chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM &&
+            chr->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A) {
+            return true;
+        }
+    }
+
     /* If we got all the way here and we're still stuck with the default
      * target type for a serial device, it means we have no clue what kind of
      * device we're talking about and we must treat it as a platform device. */
@@ -10579,6 +10590,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
         break;
 
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
         /* Except from _LAST, which is just a guard value and will never
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 886e3fbb72..1cc4cefbd1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4183,6 +4183,7 @@ qemuDomainChrSerialTargetModelToTargetType(int targetModel)
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY:
         return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO;
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
+    case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
         return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM;
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
     case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
@@ -4248,6 +4249,7 @@ qemuDomainChrTargetDefValidate(const virDomainChrDef *chr)
         case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
         case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
         case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
+        case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
 
             expected = qemuDomainChrSerialTargetModelToTargetType(chr->targetModel);
 
@@ -4298,18 +4300,23 @@ qemuDomainChrDefValidate(const virDomainChrDef *dev,
     if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) {
         bool isCompatible = true;
 
+        if (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM) {
+            if (dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011 &&
+                !qemuDomainIsARMVirt(def)) {
+                isCompatible = false;
+            }
+            if (dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A &&
+                !qemuDomainIsRISCVVirt(def)) {
+                isCompatible = false;
+            }
+        }
+
         if (!qemuDomainIsPSeries(def) &&
             (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO ||
              dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY)) {
             isCompatible = false;
         }
 
-        if (!qemuDomainIsARMVirt(def) &&
-            (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM ||
-             dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011)) {
-            isCompatible = false;
-        }
-
         if (!ARCH_IS_S390(def->os.arch) &&
             (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP ||
              dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE ||
@@ -6129,7 +6136,7 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
             chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
         } else if (qemuDomainIsPSeries(def)) {
             chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO;
-        } else if (qemuDomainIsARMVirt(def)) {
+        } else if (qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def)) {
             chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM;
         } else if (ARCH_IS_S390(def->os.arch)) {
             chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP;
@@ -6153,7 +6160,11 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
             chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY;
             break;
         case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
-            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
+            if (qemuDomainIsARMVirt(def)) {
+                chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
+            } else if (qemuDomainIsRISCVVirt(def)) {
+                chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A;
+            }
             break;
         case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
             chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE;
diff --git a/tests/qemuxml2xmloutdata/riscv64-virt.xml b/tests/qemuxml2xmloutdata/riscv64-virt.xml
index 822a59a604..daf9ca4978 100644
--- a/tests/qemuxml2xmloutdata/riscv64-virt.xml
+++ b/tests/qemuxml2xmloutdata/riscv64-virt.xml
@@ -24,7 +24,9 @@
     </disk>
     <controller type='usb' index='0'/>
     <serial type='pty'>
-      <target port='0'/>
+      <target type='system-serial' port='0'>
+        <model name='16550a'/>
+      </target>
     </serial>
     <console type='pty'>
       <target type='serial' port='0'/>
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Introduce 16550A serial console model
Posted by Ján Tomko 5 years, 7 months ago
On Mon, Aug 27, 2018 at 02:10:18PM +0200, Andrea Bolognani wrote:
>None of the existing models is suitable for use with
>RISC-V virt guests, and we don't want information about
>the serial console to be missing from the XML.
>
>The name is based on comments in qemu/hw/riscv/virt.c:
>
>  RISC-V machine with 16550a UART and VirtIO MMIO
>
>and in qemu/hw/char/serial.c:
>
>  QEMU 16550A UART emulation
>
>along with the output of dmesg in the guest:
>
>  Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
>  10000000.uart: ttyS0 at MMIO 0x10000000 (irq = 13,
>    base_baud= 230400) is a 16550A
>
>Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>---
>CC'ing Rich mostly so that he can double-check the name
>choice is sensible.
>
> docs/formatdomain.html.in                 | 12 +++++-----
> docs/schemas/domaincommon.rng             |  1 +
> src/conf/domain_conf.c                    |  1 +
> src/conf/domain_conf.h                    |  1 +
> src/qemu/qemu_command.c                   | 12 ++++++++++
> src/qemu/qemu_domain.c                    | 27 ++++++++++++++++-------
> tests/qemuxml2xmloutdata/riscv64-virt.xml |  4 +++-
> 7 files changed, 44 insertions(+), 14 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

We should consider auto-adding the device to domain XML, if it's always
present in the machine type.

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Introduce 16550A serial console model
Posted by Andrea Bolognani 5 years, 7 months ago
On Tue, 2018-08-28 at 17:49 +0200, Ján Tomko wrote:
> We should consider auto-adding the device to domain XML, if it's always
> present in the machine type.

I'll look into it.

I wonder how many other architectures are affected by the
same issue - at the very least ARM, I think.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Introduce 16550A serial console model
Posted by Richard W.M. Jones 5 years, 8 months ago
On Mon, Aug 27, 2018 at 02:10:18PM +0200, Andrea Bolognani wrote:
> None of the existing models is suitable for use with
> RISC-V virt guests, and we don't want information about
> the serial console to be missing from the XML.
> 
> The name is based on comments in qemu/hw/riscv/virt.c:
> 
>   RISC-V machine with 16550a UART and VirtIO MMIO
> 
> and in qemu/hw/char/serial.c:
> 
>   QEMU 16550A UART emulation
> 
> along with the output of dmesg in the guest:
> 
>   Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
>   10000000.uart: ttyS0 at MMIO 0x10000000 (irq = 13,
>     base_baud= 230400) is a 16550A

FWIW on the real hardware:

[    4.078734] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    4.084078] 10010000.serial: ttySI0 at MMIO 0x10010000 (irq = 43, base_baud = 0) is a sifive-serial
[    4.751449] 10011000.serial: ttySI1 at MMIO 0x10011000 (irq = 44, base_baud = 0) is a sifive-serial

Now about the patch ...

I think this is fine provided it doesn't bake in future libvirt API
that we'll regret.

However the real story is that what real RISC-V hardware will look
like is undecided.  For embedded they're making all the same mistakes
as ARMv7 all over again (despite our clear warnings).  So expect
wildly different implementations, no way to probe at runtime, random
device trees, crazy board descriptions littering the kernel and qemu,
out of tree drivers, etc.  All that crap.

For servers there's a working group supposedly looking into this and
going to produce an SBSA/SBBR-style specification.  Last time I looked
it was a single page with no detail, and I can't actually find the
link to that right now.  In any case it's nowhere near decided.  It
would be nice if they standardized a 16550A UART at a known address,
PCIe, etc. but at the moment I wouldn't make plans based on sanity
prevailing.

TL;DR: Patch is fine as long as we can change things later without
maintaining ABI.

Rich.

> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> CC'ing Rich mostly so that he can double-check the name
> choice is sensible.
> 
>  docs/formatdomain.html.in                 | 12 +++++-----
>  docs/schemas/domaincommon.rng             |  1 +
>  src/conf/domain_conf.c                    |  1 +
>  src/conf/domain_conf.h                    |  1 +
>  src/qemu/qemu_command.c                   | 12 ++++++++++
>  src/qemu/qemu_domain.c                    | 27 ++++++++++++++++-------
>  tests/qemuxml2xmloutdata/riscv64-virt.xml |  4 +++-
>  7 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0cbf570a13..eb619a1656 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7011,7 +7011,8 @@ qemu-kvm -net nic,model=? /dev/null
>        is available) and <code>pci-serial</code> (usable whenever PCI support
>        is available); <span class="since">since 3.10.0</span>,
>        <code>spapr-vio-serial</code> (usable with ppc64/pseries guests),
> -      <code>system-serial</code> (usable with aarch64/virt guests) and
> +      <code>system-serial</code> (usable with aarch64/virt and,
> +      <span class="since">since 4.7.0</span>, riscv/virt guests) and
>        <code>sclp-serial</code> (usable with s390 and s390x guests) are
>        available as well.
>      </p>
> @@ -7025,10 +7026,11 @@ qemu-kvm -net nic,model=? /dev/null
>        target type); <code>pci-serial</code>
>        (usable with the <code>pci-serial</code> target type);
>        <code>spapr-vty</code> (usable with the <code>spapr-vio-serial</code>
> -      target type); <code>pl011</code> (usable with the
> -      <code>system-serial</code> target type); <code>sclpconsole</code> and
> -      <code>sclplmconsole</code> (usable with the <code>sclp-serial</code>
> -      target type).
> +      target type); <code>pl011</code> and,
> +      <span class="since">since 4.7.0</span>, <code>16550a</code> (usable
> +      with the <code>system-serial</code> target type);
> +      <code>sclpconsole</code> and <code>sclplmconsole</code> (usable with
> +      the <code>sclp-serial</code> target type).
>      </p>
>  
>      <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f176538195..3796eb4b5e 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3733,6 +3733,7 @@
>            <value>pci-serial</value>
>            <value>spapr-vty</value>
>            <value>pl011</value>
> +          <value>16550a</value>
>            <value>sclpconsole</value>
>            <value>sclplmconsole</value>
>          </choice>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index bde9fef914..8af59bb4bb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -493,6 +493,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTargetModel,
>                "pl011",
>                "sclpconsole",
>                "sclplmconsole",
> +              "16550a",
>  );
>  
>  VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index c0ad072db5..8a3673361a 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1133,6 +1133,7 @@ typedef enum {
>      VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011,
>      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_LAST
>  } virDomainChrSerialTargetModel;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index fd9e58fd5d..f63e35444e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9149,6 +9149,7 @@ qemuChrSerialTargetModelToCaps(virDomainChrSerialTargetModel targetModel)
>          return QEMU_CAPS_DEVICE_SCLPLMCONSOLE;
>      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_NONE:
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
>          break;
> @@ -9189,6 +9190,16 @@ qemuChrIsPlatformDevice(const virDomainDef *def,
>          }
>      }
>  
> +    if (ARCH_IS_RISCV(def->os.arch)) {
> +
> +        /* 16550a (used by riscv/virt guests) is a platform device */
> +        if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
> +            chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM &&
> +            chr->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A) {
> +            return true;
> +        }
> +    }
> +
>      /* If we got all the way here and we're still stuck with the default
>       * target type for a serial device, it means we have no clue what kind of
>       * device we're talking about and we must treat it as a platform device. */
> @@ -10579,6 +10590,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
>          break;
>  
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE:
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST:
>          /* Except from _LAST, which is just a guard value and will never
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 886e3fbb72..1cc4cefbd1 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4183,6 +4183,7 @@ qemuDomainChrSerialTargetModelToTargetType(int targetModel)
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY:
>          return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO;
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
> +    case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
>          return VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM;
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
>      case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
> @@ -4248,6 +4249,7 @@ qemuDomainChrTargetDefValidate(const virDomainChrDef *chr)
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011:
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE:
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPLMCONSOLE:
> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A:
>  
>              expected = qemuDomainChrSerialTargetModelToTargetType(chr->targetModel);
>  
> @@ -4298,18 +4300,23 @@ qemuDomainChrDefValidate(const virDomainChrDef *dev,
>      if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL) {
>          bool isCompatible = true;
>  
> +        if (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM) {
> +            if (dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011 &&
> +                !qemuDomainIsARMVirt(def)) {
> +                isCompatible = false;
> +            }
> +            if (dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A &&
> +                !qemuDomainIsRISCVVirt(def)) {
> +                isCompatible = false;
> +            }
> +        }
> +
>          if (!qemuDomainIsPSeries(def) &&
>              (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO ||
>               dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY)) {
>              isCompatible = false;
>          }
>  
> -        if (!qemuDomainIsARMVirt(def) &&
> -            (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM ||
> -             dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011)) {
> -            isCompatible = false;
> -        }
> -
>          if (!ARCH_IS_S390(def->os.arch) &&
>              (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP ||
>               dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE ||
> @@ -6129,7 +6136,7 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
>              chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
>          } else if (qemuDomainIsPSeries(def)) {
>              chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO;
> -        } else if (qemuDomainIsARMVirt(def)) {
> +        } else if (qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def)) {
>              chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM;
>          } else if (ARCH_IS_S390(def->os.arch)) {
>              chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP;
> @@ -6153,7 +6160,11 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
>              chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY;
>              break;
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM:
> -            chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
> +            if (qemuDomainIsARMVirt(def)) {
> +                chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011;
> +            } else if (qemuDomainIsRISCVVirt(def)) {
> +                chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_16550A;
> +            }
>              break;
>          case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP:
>              chr->targetModel = VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SCLPCONSOLE;
> diff --git a/tests/qemuxml2xmloutdata/riscv64-virt.xml b/tests/qemuxml2xmloutdata/riscv64-virt.xml
> index 822a59a604..daf9ca4978 100644
> --- a/tests/qemuxml2xmloutdata/riscv64-virt.xml
> +++ b/tests/qemuxml2xmloutdata/riscv64-virt.xml
> @@ -24,7 +24,9 @@
>      </disk>
>      <controller type='usb' index='0'/>
>      <serial type='pty'>
> -      <target port='0'/>
> +      <target type='system-serial' port='0'>
> +        <model name='16550a'/>
> +      </target>
>      </serial>
>      <console type='pty'>
>        <target type='serial' port='0'/>
> -- 
> 2.17.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Introduce 16550A serial console model
Posted by Andrea Bolognani 5 years, 8 months ago
On Mon, 2018-08-27 at 15:31 +0100, Richard W.M. Jones wrote:
> On Mon, Aug 27, 2018 at 02:10:18PM +0200, Andrea Bolognani wrote:
> > None of the existing models is suitable for use with
> > RISC-V virt guests, and we don't want information about
> > the serial console to be missing from the XML.
> > 
> > The name is based on comments in qemu/hw/riscv/virt.c:
> > 
> >   RISC-V machine with 16550a UART and VirtIO MMIO
> > 
> > and in qemu/hw/char/serial.c:
> > 
> >   QEMU 16550A UART emulation
> > 
> > along with the output of dmesg in the guest:
> > 
> >   Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> >   10000000.uart: ttyS0 at MMIO 0x10000000 (irq = 13,
> >     base_baud= 230400) is a 16550A
> 
> FWIW on the real hardware:
> 
> [    4.078734] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [    4.084078] 10010000.serial: ttySI0 at MMIO 0x10010000 (irq = 43, base_baud = 0) is a sifive-serial
> [    4.751449] 10011000.serial: ttySI1 at MMIO 0x10011000 (irq = 44, base_baud = 0) is a sifive-serial

Well, that's a SiFive machine so sifive-serial doesn't look out
of place. I believe the sifive_* machine types in QEMU will also
expose a sifive-serial rather than a 16550a, though I haven't
actually verified that's the case.

> Now about the patch ...
> 
> I think this is fine provided it doesn't bake in future libvirt API
> that we'll regret.

The very idea behind this patch is that spelling out defaults
in the XML leaves the door open to changing them down the line
without affecting existing guests or breaking migration :)

> However the real story is that what real RISC-V hardware will look
> like is undecided.  For embedded they're making all the same mistakes
> as ARMv7 all over again (despite our clear warnings).  So expect
> wildly different implementations, no way to probe at runtime, random
> device trees, crazy board descriptions littering the kernel and qemu,
> out of tree drivers, etc.  All that crap.
> 
> For servers there's a working group supposedly looking into this and
> going to produce an SBSA/SBBR-style specification.  Last time I looked
> it was a single page with no detail, and I can't actually find the
> link to that right now.  In any case it's nowhere near decided.  It
> would be nice if they standardized a 16550A UART at a known address,
> PCIe, etc. but at the moment I wouldn't make plans based on sanity
> prevailing.

Well that's disappointing :( I guess they'll have to get to
a reasonable place the long way around, just like Arm did.

> TL;DR: Patch is fine as long as we can change things later without
> maintaining ABI.

Good to hear! Thanks for the feedback :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list