On Mon, Oct 17, 2011 at 7:17 AM, Jan Kiszka <jan.kis...@web.de> wrote:
> On 2011-10-16 23:21, Blue Swirl wrote:
>> Improve VGA selection logic, push check for device availabilty to vl.c.
>> Make PCI VGA devices optional.
>>
>> Signed-off-by: Blue Swirl <blauwir...@gmail.com>
>> ---
>> hw/cirrus_vga.c | 5 -----
>> hw/pc.c | 6 +-----
>> hw/pc.h | 33 +++++++++++++++++++++++++++------
>> hw/pci.c | 18 ++++++++++++++++++
>> hw/pci.h | 4 ++++
>> hw/qdev.c | 5 +++++
>> hw/qdev.h | 1 +
>> hw/vga-pci.c | 6 ------
>> vl.c | 33 +++++++++++++++++++++++++++------
>> 9 files changed, 83 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>> index c7e365b..a11444c 100644
>> --- a/hw/cirrus_vga.c
>> +++ b/hw/cirrus_vga.c
>> @@ -2955,11 +2955,6 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>> return 0;
>> }
>>
>> -void pci_cirrus_vga_init(PCIBus *bus)
>> -{
>> - pci_create_simple(bus, -1, "cirrus-vga");
>> -}
>> -
>> static PCIDeviceInfo cirrus_vga_info = {
>> .qdev.name = "cirrus-vga",
>> .qdev.desc = "Cirrus CLGD 54xx VGA",
>> diff --git a/hw/pc.c b/hw/pc.c
>> index f0802b7..057eb9c 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1080,11 +1080,7 @@ void pc_vga_init(PCIBus *pci_bus)
>> }
>> } else if (vmsvga_enabled) {
>> if (pci_bus) {
>> - 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 {
>> fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__);
>> }
>> diff --git a/hw/pc.h b/hw/pc.h
>> index b8ad9a3..6c951e8 100644
>> --- a/hw/pc.h
>> +++ b/hw/pc.h
>> @@ -9,6 +9,7 @@
>> #include "net.h"
>> #include "memory.h"
>> #include "ioapic.h"
>> +#include "pci.h"
>>
>> /* PC-style peripherals (also used by other machines). */
>>
>> @@ -203,26 +204,46 @@ enum vga_retrace_method {
>>
>> extern enum vga_retrace_method vga_retrace_method;
>>
>> -static inline int isa_vga_init(void)
>> +static inline bool isa_vga_init(void)
>> {
>> ISADevice *dev;
>>
>> dev = isa_try_create("isa-vga");
>> if (!dev) {
>> - fprintf(stderr, "Warning: isa-vga not available\n");
>> - return 0;
>> + return false;
>> }
>> qdev_init_nofail(&dev->qdev);
>> - return 1;
>> + return true;
>> +}
>> +
>> +/* vga-pci.c */
>> +static inline bool pci_vga_init(PCIBus *bus)
>> +{
>> + PCIDevice *dev;
>> +
>> + dev = pci_try_create_simple(bus, -1, "VGA");
>> + if (!dev) {
>> + return false;
>> + }
>> + return true;
>> }
>>
>> -int pci_vga_init(PCIBus *bus);
>> int isa_vga_mm_init(target_phys_addr_t vram_base,
>> target_phys_addr_t ctrl_base, int it_shift,
>> MemoryRegion *address_space);
>>
>> /* cirrus_vga.c */
>> -void pci_cirrus_vga_init(PCIBus *bus);
>> +static inline bool pci_cirrus_vga_init(PCIBus *bus)
>> +{
>> + PCIDevice *dev;
>> +
>> + dev = pci_try_create_simple(bus, -1, "cirrus-vga");
>> + if (!dev) {
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> void isa_cirrus_vga_init(MemoryRegion *address_space);
>>
>> /* ne2000.c */
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 749e8d8..46c01ac 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1687,6 +1687,19 @@ PCIDevice
>> *pci_create_simple_multifunction(PCIBus *bus, int devfn,
>> return dev;
>> }
>>
>> +PCIDevice *pci_try_create_simple_multifunction(PCIBus *bus, int devfn,
>> + bool multifunction,
>> + const char *name)
>> +{
>> + PCIDevice *dev = pci_try_create_multifunction(bus, devfn, multifunction,
>> + name);
>> + if (!dev) {
>> + return NULL;
>> + }
>> + qdev_init_nofail(&dev->qdev);
>> + return dev;
>> +}
>> +
>> PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
>> {
>> return pci_create_multifunction(bus, devfn, false, name);
>> @@ -1702,6 +1715,11 @@ PCIDevice *pci_try_create(PCIBus *bus, int
>> devfn, const char *name)
>> return pci_try_create_multifunction(bus, devfn, false, name);
>> }
>>
>> +PCIDevice *pci_try_create_simple(PCIBus *bus, int devfn, const char *name)
>> +{
>> + return pci_try_create_simple_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 86a81c8..aa2e040 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -473,9 +473,13 @@ PCIDevice *pci_create_simple_multifunction(PCIBus
>> *bus, int devfn,
>> PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn,
>> bool multifunction,
>> const char *name);
>> +PCIDevice *pci_try_create_simple_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);
>> +PCIDevice *pci_try_create_simple(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 a223d41..dc0aa1c 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -80,6 +80,11 @@ 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 DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
>> {
>> DeviceState *dev;
>> diff --git a/hw/qdev.h b/hw/qdev.h
>> index aa7ae36..a53ccb1 100644
>> --- a/hw/qdev.h
>> +++ b/hw/qdev.h
>> @@ -123,6 +123,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/vga-pci.c b/hw/vga-pci.c
>> index 14bfadb..68fddd9 100644
>> --- a/hw/vga-pci.c
>> +++ b/hw/vga-pci.c
>> @@ -70,12 +70,6 @@ static int pci_vga_initfn(PCIDevice *dev)
>> return 0;
>> }
>>
>> -int pci_vga_init(PCIBus *bus)
>> -{
>> - pci_create_simple(bus, -1, "VGA");
>> - return 0;
>> -}
>> -
>> static PCIDeviceInfo vga_info = {
>> .qdev.name = "VGA",
>> .qdev.size = sizeof(PCIVGAState),
>> diff --git a/vl.c b/vl.c
>> index 2dce3ae..15fb0d1 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1654,11 +1654,26 @@ static void select_vgahw (const char *p)
>> default_vga = 0;
>> vga_interface_type = VGA_NONE;
>> if (strstart(p, "std", &opts)) {
>> - vga_interface_type = VGA_STD;
>> + if (qdev_exists("VGA") || qdev_exists("isa-vga")) {
>> + 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 (qdev_exists("cirrus-vga") || qdev_exists("isa-cirrus-vga")) {
>> + 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 (qdev_exists("vmware-svga")) {
>> + 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)) {
>> @@ -2277,6 +2292,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
>> @@ -2694,7 +2710,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:
>> {
>> @@ -3252,8 +3268,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();
>>
>> @@ -3398,6 +3412,13 @@ 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 (default_vga) {
>> + select_vgahw("cirrus");
>> + }
>
> Nice change, except maybe for this detail: If the default model is
> disabled, we will bail out, no? Would it make sense to handle this case
> gracefully as well?
Yes, !default_vga should be same as -vga none.
We could also drop some try_create stuff now that the checks are done
at higher level, though those are still useful for things like
vmmouse.
