Re: [Qemu-devel] [PATCH 1/4] vga: improve VGA logic

[Jan Kiszka]

On 2012-01-08 22:09, Blue Swirl wrote:
> Improve VGA selection logic, push check for device availabilty to vl.c.
> Create the devices at board level unconditionally.
> 
> Remove now unused pci_try_create*() functions.
> 
> Make PCI VGA devices optional.
> 
> Signed-off-by: Blue Swirl <address@hidden>
> ---
>  hw/alpha_pci.c   |    6 +---
>  hw/boards.h      |    1 -
>  hw/mips_malta.c  |    6 +----
>  hw/pc.c          |    5 ----
>  hw/pci.c         |   20 -----------------
>  hw/pci.h         |    4 ---
>  hw/qdev.c        |    4 +++
>  hw/qdev.h        |    1 +
>  hw/s390-virtio.c |    1 -
>  hw/spapr.c       |    1 -
>  hw/vmware_vga.h  |    8 +-----
>  vl.c             |   62 +++++++++++++++++++++++++++++++++++++++--------------
>  12 files changed, 55 insertions(+), 64 deletions(-)
> 
> diff --git a/hw/alpha_pci.c b/hw/alpha_pci.c
> index e975702..6735577 100644
> --- a/hw/alpha_pci.c
> +++ b/hw/alpha_pci.c
> @@ -121,10 +121,8 @@ void alpha_pci_vga_setup(PCIBus *pci_bus)
>          pci_cirrus_vga_init(pci_bus);
>          return;
>      case VGA_VMWARE:
> -        if (pci_vmsvga_init(pci_bus)) {
> -            return;
> -        }
> -        break;
> +        pci_vmsvga_init(pci_bus);
> +        return;
>      }
>      /* If VGA is enabled at all, and one of the above didn't work, then
>         fallback to Standard VGA.  */
> diff --git a/hw/boards.h b/hw/boards.h
> index 716fd7b..f6d3784 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -22,7 +22,6 @@ typedef struct QEMUMachine {
>      unsigned int no_serial:1,
>          no_parallel:1,
>          use_virtcon:1,
> -        no_vga:1,
>          no_floppy:1,
>          no_cdrom:1,
>          no_sdcard:1;
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index e625ec3..4a4f76d 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -996,11 +996,7 @@ void mips_malta_init (ram_addr_t ram_size,
>      if (cirrus_vga_enabled) {
>          pci_cirrus_vga_init(pci_bus);
>      } else if (vmsvga_enabled) {
> -        if (!pci_vmsvga_init(pci_bus)) {
> -            fprintf(stderr, "Warning: vmware_vga not available,"
> -                    " using standard VGA instead\n");
> -            pci_vga_init(pci_bus);
> -        }
> +        pci_vmsvga_init(pci_bus);
>      } else if (std_vga_enabled) {
>          pci_vga_init(pci_bus);
>      }
> diff --git a/hw/pc.c b/hw/pc.c
> index 85304cf..8cb78d9 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1085,11 +1085,6 @@ DeviceState *pc_vga_init(ISABus *isa_bus,
> PCIBus *pci_bus)
>      } else if (vmsvga_enabled) {
>          if (pci_bus) {
>              dev = pci_vmsvga_init(pci_bus);
> -            if (!dev) {
> -                fprintf(stderr, "Warning: vmware_vga not available,"
> -                        " using standard VGA instead\n");
> -                dev = pci_vga_init(pci_bus);
> -            }
>          } else {
>              fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__);
>          }
> diff --git a/hw/pci.c b/hw/pci.c
> index c3082bc..54400ac 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1572,21 +1572,6 @@ PCIDevice *pci_create_multifunction(PCIBus
> *bus, int devfn, bool multifunction,
>      return DO_UPCAST(PCIDevice, qdev, dev);
>  }
> 
> -PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn,
> -                                        bool multifunction,
> -                                        const char *name)
> -{
> -    DeviceState *dev;
> -
> -    dev = qdev_try_create(&bus->qbus, name);
> -    if (!dev) {
> -        return NULL;
> -    }
> -    qdev_prop_set_uint32(dev, "addr", devfn);
> -    qdev_prop_set_bit(dev, "multifunction", multifunction);
> -    return DO_UPCAST(PCIDevice, qdev, dev);
> -}
> -
>  PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
>                                             bool multifunction,
>                                             const char *name)
> @@ -1606,11 +1591,6 @@ PCIDevice *pci_create_simple(PCIBus *bus, int
> devfn, const char *name)
>      return pci_create_simple_multifunction(bus, devfn, false, name);
>  }
> 
> -PCIDevice *pci_try_create(PCIBus *bus, int devfn, const char *name)
> -{
> -    return pci_try_create_multifunction(bus, devfn, false, name);
> -}
> -
>  static int pci_find_space(PCIDevice *pdev, uint8_t size)
>  {
>      int config_size = pci_config_size(pdev);
> diff --git a/hw/pci.h b/hw/pci.h
> index 625e717..160ba93 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -469,12 +469,8 @@ PCIDevice *pci_create_multifunction(PCIBus *bus,
> int devfn, bool multifunction,
>  PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
>                                             bool multifunction,
>                                             const char *name);
> -PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn,
> -                                        bool multifunction,
> -                                        const char *name);
>  PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
>  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
> -PCIDevice *pci_try_create(PCIBus *bus, int devfn, const char *name);
> 
>  static inline int pci_is_express(const PCIDevice *d)
>  {
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d0cf66d..c424b46 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -80,6 +80,10 @@ static DeviceInfo *qdev_find_info(BusInfo
> *bus_info, const char *name)
>      return NULL;
>  }
> 
> +bool qdev_exists(const char *name)
> +{
> +    return !!qdev_find_info(NULL, name);
> +}
>  static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>                                       Error **errp);
> 
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 2abb767..6b58dd8 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -179,6 +179,7 @@ typedef struct GlobalProperty {
> 
>  DeviceState *qdev_create(BusState *bus, const char *name);
>  DeviceState *qdev_try_create(BusState *bus, const char *name);
> +bool qdev_exists(const char *name);
>  int qdev_device_help(QemuOpts *opts);
>  DeviceState *qdev_device_add(QemuOpts *opts);
>  int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 2210b8a..51123a7 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -317,7 +317,6 @@ static QEMUMachine s390_machine = {
>      .no_serial = 1,
>      .no_parallel = 1,
>      .use_virtcon = 1,
> -    .no_vga = 1,
>      .max_cpus = 255,
>      .is_default = 1,
>  };
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 0e1f80d..906d2e9 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -693,7 +693,6 @@ static QEMUMachine spapr_machine = {
>      .desc = "pSeries Logical Partition (PAPR compliant)",
>      .init = ppc_spapr_init,
>      .max_cpus = MAX_CPUS,
> -    .no_vga = 1,
>      .no_parallel = 1,
>      .use_scsi = 1,
>  };
> diff --git a/hw/vmware_vga.h b/hw/vmware_vga.h
> index db11cbf..000fbdd 100644
> --- a/hw/vmware_vga.h
> +++ b/hw/vmware_vga.h
> @@ -8,12 +8,8 @@ static inline DeviceState *pci_vmsvga_init(PCIBus *bus)
>  {
>      PCIDevice *dev;
> 
> -    dev = pci_try_create(bus, -1, "vmware-svga");
> -    if (!dev || qdev_init(&dev->qdev) < 0) {
> -        return NULL;
> -    } else {
> -        return &dev->qdev;
> -    }
> +    dev = pci_create_simple(bus, -1, "vmware-svga");
> +    return &dev->qdev;
>  }
> 
>  #endif
> diff --git a/vl.c b/vl.c
> index ba55b35..d5868b1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -271,7 +271,6 @@ static int default_serial = 1;
>  static int default_parallel = 1;
>  static int default_virtcon = 1;
>  static int default_monitor = 1;
> -static int default_vga = 1;
>  static int default_floppy = 1;
>  static int default_cdrom = 1;
>  static int default_sdcard = 1;
> @@ -290,11 +289,6 @@ static struct {
>      { .driver = "virtio-serial-pci",    .flag = &default_virtcon   },
>      { .driver = "virtio-serial-s390",   .flag = &default_virtcon   },
>      { .driver = "virtio-serial",        .flag = &default_virtcon   },
> -    { .driver = "VGA",                  .flag = &default_vga       },
> -    { .driver = "cirrus-vga",           .flag = &default_vga       },
> -    { .driver = "vmware-svga",          .flag = &default_vga       },
> -    { .driver = "isa-vga",              .flag = &default_vga       },
> -    { .driver = "qxl-vga",              .flag = &default_vga       },
>  };
> 
>  static void res_free(void)
> @@ -1525,18 +1519,48 @@ static const QEMUOption qemu_options[] = {
>  #include "qemu-options-wrapper.h"
>      { NULL },
>  };
> +
> +static bool vga_available(void)
> +{
> +    return qdev_exists("VGA") || qdev_exists("isa-vga");
> +}
> +
> +static bool cirrus_vga_available(void)
> +{
> +    return qdev_exists("cirrus-vga") || qdev_exists("isa-cirrus-vga");
> +}
> +
> +static bool vmware_vga_available(void)
> +{
> +    return qdev_exists("vmware-svga");
> +}
> +
>  static void select_vgahw (const char *p)
>  {
>      const char *opts;
> 
> -    default_vga = 0;
>      vga_interface_type = VGA_NONE;
>      if (strstart(p, "std", &opts)) {
> -        vga_interface_type = VGA_STD;
> +        if (vga_available()) {
> +            vga_interface_type = VGA_STD;
> +        } else {
> +            fprintf(stderr, "Error: standard VGA not available\n");
> +            exit(0);
> +        }
>      } else if (strstart(p, "cirrus", &opts)) {
> -        vga_interface_type = VGA_CIRRUS;
> +        if (cirrus_vga_available()) {
> +            vga_interface_type = VGA_CIRRUS;
> +        } else {
> +            fprintf(stderr, "Error: Cirrus VGA not available\n");
> +            exit(0);
> +        }
>      } else if (strstart(p, "vmware", &opts)) {
> -        vga_interface_type = VGA_VMWARE;
> +        if (vmware_vga_available()) {
> +            vga_interface_type = VGA_VMWARE;
> +        } else {
> +            fprintf(stderr, "Error: VMWare SVGA not available\n");
> +            exit(0);
> +        }
>      } else if (strstart(p, "xenfb", &opts)) {
>          vga_interface_type = VGA_XENFB;
>      } else if (strstart(p, "qxl", &opts)) {
> @@ -2155,6 +2179,7 @@ int main(int argc, char **argv, char **envp)
>      const char *loadvm = NULL;
>      QEMUMachine *machine;
>      const char *cpu_model;
> +    const char *vga_model = NULL;
>      const char *pid_file = NULL;
>      const char *incoming = NULL;
>  #ifdef CONFIG_VNC
> @@ -2581,7 +2606,7 @@ int main(int argc, char **argv, char **envp)
>                  rtc_utc = 0;
>                  break;
>              case QEMU_OPTION_vga:
> -                select_vgahw (optarg);
> +                vga_model = optarg;
>                  break;
>              case QEMU_OPTION_g:
>                  {
> @@ -2978,7 +3003,6 @@ int main(int argc, char **argv, char **envp)
>                  default_parallel = 0;
>                  default_virtcon = 0;
>                  default_monitor = 0;
> -                default_vga = 0;
>                  default_net = 0;
>                  default_floppy = 0;
>                  default_cdrom = 0;
> @@ -3140,9 +3164,6 @@ int main(int argc, char **argv, char **envp)
>      if (!machine->use_virtcon) {
>          default_virtcon = 0;
>      }
> -    if (machine->no_vga) {
> -        default_vga = 0;
> -    }
>      if (machine->no_floppy) {
>          default_floppy = 0;
>      }
> @@ -3178,8 +3199,6 @@ int main(int argc, char **argv, char **envp)
>          if (default_virtcon)
>              add_device_config(DEV_VIRTCON, "vc:80Cx24C");
>      }
> -    if (default_vga)
> -        vga_interface_type = VGA_CIRRUS;
> 
>      socket_init();
> 
> @@ -3330,6 +3349,15 @@ int main(int argc, char **argv, char **envp)
> 
>      module_call_init(MODULE_INIT_DEVICE);
> 
> +    /* must be after qdev registration but before machine init */
> +    if (vga_model) {
> +        select_vgahw(vga_model);
> +    } else if (cirrus_vga_available()) {
> +        select_vgahw("cirrus");
> +    } else {
> +        select_vgahw("none");
> +    }
> +
>      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func,
> NULL, 0) != 0)
>          exit(0);
> 

Looks good to me.

Reviewed-by: Jan Kiszka <address@hidden>

signature.asc
Description: OpenPGP digital signature