[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]