This reverts the stdvga vs. cirrus selection logic.  Current state is
that "cirrus" is the default and MachineClass->default_display is set to
"std" by some machine types to override that.

The patch makes "std" the default.  Setting default_display to "std" is
dropped.  Machine types which likely depend on cirrus (alpha, mips, old
pc versions) will explicitly set default_display to "cirrus".

With this patch applied all ppc machine types will use "std" as default
display, no matter whenever cirrus-vga is compiled in or not.

Fixes: 29f9cef39e ppc: Include vga cirrus card into the compiling process
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/alpha/dp264.c     | 1 +
 hw/i386/pc_piix.c    | 3 +--
 hw/i386/pc_q35.c     | 1 -
 hw/mips/mips_malta.c | 1 +
 hw/mips/mips_r4k.c   | 1 +
 hw/ppc/spapr.c       | 1 -
 vl.c                 | 4 ++--
 7 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 80b987f7fb..668e6d099f 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -182,6 +182,7 @@ static void clipper_machine_init(MachineClass *mc)
     mc->max_cpus = 4;
     mc->is_default = 1;
     mc->default_cpu_type = ALPHA_CPU_TYPE_NAME("ev67");
+    mc->default_display = "cirrus";
 }
 
 DEFINE_MACHINE("clipper", clipper_machine_init)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dc09466b3e..30ad22e2e3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -424,7 +424,6 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->family = "pc_piix";
     m->desc = "Standard PC (i440FX + PIIX, 1996)";
     m->default_machine_opts = "firmware=bios-256k.bin";
-    m->default_display = "std";
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
 }
 
@@ -566,7 +565,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_2_2_machine_options(m);
     m->hw_version = "2.1.0";
-    m->default_display = NULL;
+    m->default_display = "cirrus";
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
     pcmc->smbios_uuid_encoded = false;
     pcmc->enforce_aligned_dimm = false;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 532241e3f8..8f2c8c8b8b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -303,7 +303,6 @@ static void pc_q35_machine_options(MachineClass *m)
     m->desc = "Standard PC (Q35 + ICH9, 2009)";
     m->units_per_default_bus = 1;
     m->default_machine_opts = "firmware=bios-256k.bin";
-    m->default_display = "std";
     m->no_floppy = 1;
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 3467451482..998971c53a 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1242,6 +1242,7 @@ static void mips_malta_machine_init(MachineClass *mc)
     mc->block_default_type = IF_IDE;
     mc->max_cpus = 16;
     mc->is_default = 1;
+    mc->default_display = "cirrus";
 #ifdef TARGET_MIPS64
     mc->default_cpu_type = MIPS_CPU_TYPE_NAME("20Kc");
 #else
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index d5725d0555..c4c7ee8aa5 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -295,6 +295,7 @@ static void mips_machine_init(MachineClass *mc)
     mc->desc = "mips r4k platform";
     mc->init = mips_r4k_init;
     mc->block_default_type = IF_IDE;
+    mc->default_display = "cirrus";
 #ifdef TARGET_MIPS64
     mc->default_cpu_type = MIPS_CPU_TYPE_NAME("R4000");
 #else
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3f5e1d3ec2..0ab90f3aa8 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3962,7 +3962,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->no_parallel = 1;
     mc->default_boot_order = "";
     mc->default_ram_size = 512 * MiB;
-    mc->default_display = "std";
     mc->kvm_type = spapr_kvm_type;
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
     mc->pci_allow_0_address = true;
diff --git a/vl.c b/vl.c
index 16b913f9d5..e60bf2d6cd 100644
--- a/vl.c
+++ b/vl.c
@@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
     if (default_vga) {
         if (machine_class->default_display) {
             vga_model = machine_class->default_display;
-        } else if (vga_interface_available(VGA_CIRRUS)) {
-            vga_model = "cirrus";
         } else if (vga_interface_available(VGA_STD)) {
             vga_model = "std";
+        } else if (vga_interface_available(VGA_CIRRUS)) {
+            vga_model = "cirrus";
         }
     }
     if (vga_model) {
-- 
2.9.3

Am 2018-07-04 13:28, schrieb Gerd Hoffmann:
> This reverts the stdvga vs. cirrus selection logic.  Current state is
> that "cirrus" is the default and MachineClass->default_display is set 
> to
> "std" by some machine types to override that.
> 
> The patch makes "std" the default.  Setting default_display to "std" is
> dropped.  Machine types which likely depend on cirrus (alpha, mips, old
> pc versions) will explicitly set default_display to "cirrus".
> 
> With this patch applied all ppc machine types will use "std" as default
> display, no matter whenever cirrus-vga is compiled in or not.
> 
> Fixes: 29f9cef39e ppc: Include vga cirrus card into the compiling 
> process
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/alpha/dp264.c     | 1 +
>  hw/i386/pc_piix.c    | 3 +--
>  hw/i386/pc_q35.c     | 1 -
>  hw/mips/mips_malta.c | 1 +
>  hw/mips/mips_r4k.c   | 1 +
>  hw/ppc/spapr.c       | 1 -
>  vl.c                 | 4 ++--
>  7 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 80b987f7fb..668e6d099f 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -182,6 +182,7 @@ static void clipper_machine_init(MachineClass *mc)
>      mc->max_cpus = 4;
>      mc->is_default = 1;
>      mc->default_cpu_type = ALPHA_CPU_TYPE_NAME("ev67");
> +    mc->default_display = "cirrus";
>  }
> 
>  DEFINE_MACHINE("clipper", clipper_machine_init)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index dc09466b3e..30ad22e2e3 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -424,7 +424,6 @@ static void pc_i440fx_machine_options(MachineClass 
> *m)
>      m->family = "pc_piix";
>      m->desc = "Standard PC (i440FX + PIIX, 1996)";
>      m->default_machine_opts = "firmware=bios-256k.bin";
> -    m->default_display = "std";
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>  }
> 
> @@ -566,7 +565,7 @@ static void 
> pc_i440fx_2_1_machine_options(MachineClass *m)
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_2_2_machine_options(m);
>      m->hw_version = "2.1.0";
> -    m->default_display = NULL;
> +    m->default_display = "cirrus";
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
>      pcmc->smbios_uuid_encoded = false;
>      pcmc->enforce_aligned_dimm = false;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 532241e3f8..8f2c8c8b8b 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -303,7 +303,6 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->desc = "Standard PC (Q35 + ICH9, 2009)";
>      m->units_per_default_bus = 1;
>      m->default_machine_opts = "firmware=bios-256k.bin";
> -    m->default_display = "std";
>      m->no_floppy = 1;
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, 
> TYPE_INTEL_IOMMU_DEVICE);
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 3467451482..998971c53a 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1242,6 +1242,7 @@ static void mips_malta_machine_init(MachineClass 
> *mc)
>      mc->block_default_type = IF_IDE;
>      mc->max_cpus = 16;
>      mc->is_default = 1;
> +    mc->default_display = "cirrus";
>  #ifdef TARGET_MIPS64
>      mc->default_cpu_type = MIPS_CPU_TYPE_NAME("20Kc");
>  #else
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index d5725d0555..c4c7ee8aa5 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -295,6 +295,7 @@ static void mips_machine_init(MachineClass *mc)
>      mc->desc = "mips r4k platform";
>      mc->init = mips_r4k_init;
>      mc->block_default_type = IF_IDE;
> +    mc->default_display = "cirrus";
>  #ifdef TARGET_MIPS64
>      mc->default_cpu_type = MIPS_CPU_TYPE_NAME("R4000");
>  #else
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3f5e1d3ec2..0ab90f3aa8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3962,7 +3962,6 @@ static void spapr_machine_class_init(ObjectClass
> *oc, void *data)
>      mc->no_parallel = 1;
>      mc->default_boot_order = "";
>      mc->default_ram_size = 512 * MiB;
> -    mc->default_display = "std";
>      mc->kvm_type = spapr_kvm_type;
>      machine_class_allow_dynamic_sysbus_dev(mc, 
> TYPE_SPAPR_PCI_HOST_BRIDGE);
>      mc->pci_allow_0_address = true;
> diff --git a/vl.c b/vl.c
> index 16b913f9d5..e60bf2d6cd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
>      if (default_vga) {
>          if (machine_class->default_display) {
>              vga_model = machine_class->default_display;
> -        } else if (vga_interface_available(VGA_CIRRUS)) {
> -            vga_model = "cirrus";
>          } else if (vga_interface_available(VGA_STD)) {
>              vga_model = "std";
> +        } else if (vga_interface_available(VGA_CIRRUS)) {
> +            vga_model = "cirrus";
>          }
>      }
>      if (vga_model) {

If that counts:

Reviewed-by: Sebastian Bauer <mail@sebastianbauer.info>

If applied, perhaps setting the default to "std" except for few machines 
is worth to be mentioned in an upcoming ChangeLog, in case there are 
other machines that may require cirrus graphics.

Bye
Sebastian

On Wed, Jul 04, 2018 at 01:28:59PM +0200, Gerd Hoffmann wrote:
> This reverts the stdvga vs. cirrus selection logic.  Current state is
> that "cirrus" is the default and MachineClass->default_display is set to
> "std" by some machine types to override that.
> 
> The patch makes "std" the default.  Setting default_display to "std" is
> dropped.  Machine types which likely depend on cirrus (alpha, mips, old
> pc versions) will explicitly set default_display to "cirrus".
> 
> With this patch applied all ppc machine types will use "std" as default
> display, no matter whenever cirrus-vga is compiled in or not.
> 
> Fixes: 29f9cef39e ppc: Include vga cirrus card into the compiling process
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

So, the current state is that default_display==NULL is
unpredictable and fragile.  The last hunk in this patch changes
the default when default_dispay==NULL, but it's still fragile.

I don't think we must audit all machine-types with
default_display=NULL today.  But:

[...]
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 80b987f7fb..668e6d099f 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -182,6 +182,7 @@ static void clipper_machine_init(MachineClass *mc)
>      mc->max_cpus = 4;
>      mc->is_default = 1;
>      mc->default_cpu_type = ALPHA_CPU_TYPE_NAME("ev67");
> +    mc->default_display = "cirrus";

OK.

>  }
>  
>  DEFINE_MACHINE("clipper", clipper_machine_init)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index dc09466b3e..30ad22e2e3 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -424,7 +424,6 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->family = "pc_piix";
>      m->desc = "Standard PC (i440FX + PIIX, 1996)";
>      m->default_machine_opts = "firmware=bios-256k.bin";
> -    m->default_display = "std";
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);

I wouldn't like to do this.  Long term, it would be a good idea
to have zero machines with default_display==NULL, so let's keep
default_machine="std" on PC?


>  }
>  
> @@ -566,7 +565,7 @@ static void pc_i440fx_2_1_machine_options(MachineClass *m)
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_2_2_machine_options(m);
>      m->hw_version = "2.1.0";
> -    m->default_display = NULL;
> +    m->default_display = "cirrus";

OK.

>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_1);
>      pcmc->smbios_uuid_encoded = false;
>      pcmc->enforce_aligned_dimm = false;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 532241e3f8..8f2c8c8b8b 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -303,7 +303,6 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->desc = "Standard PC (Q35 + ICH9, 2009)";
>      m->units_per_default_bus = 1;
>      m->default_machine_opts = "firmware=bios-256k.bin";
> -    m->default_display = "std";

Same comment as pc_i440fx_machine_options() above.

>      m->no_floppy = 1;
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_AMD_IOMMU_DEVICE);
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_INTEL_IOMMU_DEVICE);
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 3467451482..998971c53a 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1242,6 +1242,7 @@ static void mips_malta_machine_init(MachineClass *mc)
>      mc->block_default_type = IF_IDE;
>      mc->max_cpus = 16;
>      mc->is_default = 1;
> +    mc->default_display = "cirrus";

OK.

>  #ifdef TARGET_MIPS64
>      mc->default_cpu_type = MIPS_CPU_TYPE_NAME("20Kc");
>  #else
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index d5725d0555..c4c7ee8aa5 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -295,6 +295,7 @@ static void mips_machine_init(MachineClass *mc)
>      mc->desc = "mips r4k platform";
>      mc->init = mips_r4k_init;
>      mc->block_default_type = IF_IDE;
> +    mc->default_display = "cirrus";

OK.

>  #ifdef TARGET_MIPS64
>      mc->default_cpu_type = MIPS_CPU_TYPE_NAME("R4000");
>  #else
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3f5e1d3ec2..0ab90f3aa8 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3962,7 +3962,6 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_parallel = 1;
>      mc->default_boot_order = "";
>      mc->default_ram_size = 512 * MiB;
> -    mc->default_display = "std";

Same comment as pc_i440fx_machine_options() above.

>      mc->kvm_type = spapr_kvm_type;
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
>      mc->pci_allow_0_address = true;
> diff --git a/vl.c b/vl.c
> index 16b913f9d5..e60bf2d6cd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
>      if (default_vga) {
>          if (machine_class->default_display) {
>              vga_model = machine_class->default_display;
> -        } else if (vga_interface_available(VGA_CIRRUS)) {
> -            vga_model = "cirrus";
>          } else if (vga_interface_available(VGA_STD)) {
>              vga_model = "std";
> +        } else if (vga_interface_available(VGA_CIRRUS)) {
> +            vga_model = "cirrus";

If we don't make the machines above have default_display=NULL, we
won't need this hunk, right?  In either case, I suggest doing
this change on a separate patch.

-- 
Eduardo

Am 2018-07-04 20:53, schrieb Eduardo Habkost:
>>      mc->kvm_type = spapr_kvm_type;
>>      machine_class_allow_dynamic_sysbus_dev(mc, 
>> TYPE_SPAPR_PCI_HOST_BRIDGE);
>>      mc->pci_allow_0_address = true;
>> diff --git a/vl.c b/vl.c
>> index 16b913f9d5..e60bf2d6cd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
>>      if (default_vga) {
>>          if (machine_class->default_display) {
>>              vga_model = machine_class->default_display;
>> -        } else if (vga_interface_available(VGA_CIRRUS)) {
>> -            vga_model = "cirrus";
>>          } else if (vga_interface_available(VGA_STD)) {
>>              vga_model = "std";
>> +        } else if (vga_interface_available(VGA_CIRRUS)) {
>> +            vga_model = "cirrus";
> 
> If we don't make the machines above have default_display=NULL, we
> won't need this hunk, right?  In either case, I suggest doing
> this change on a separate patch.

An alternative would be to define the default always to VGA_STD if the 
machine does not override it. This is from the observation that the 
majority would chose std. Obviously, having default values always has 
pro and cons. I would not consider it harmful here.

Is there any configuration that includes VGA_CIRRUS but not VGA_STD?

Bye
Sebastian

On Wed, Jul 04, 2018 at 10:21:19PM +0200, Sebastian Bauer wrote:
> Am 2018-07-04 20:53, schrieb Eduardo Habkost:
> > >      mc->kvm_type = spapr_kvm_type;
> > >      machine_class_allow_dynamic_sysbus_dev(mc,
> > > TYPE_SPAPR_PCI_HOST_BRIDGE);
> > >      mc->pci_allow_0_address = true;
> > > diff --git a/vl.c b/vl.c
> > > index 16b913f9d5..e60bf2d6cd 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
> > >      if (default_vga) {
> > >          if (machine_class->default_display) {
> > >              vga_model = machine_class->default_display;
> > > -        } else if (vga_interface_available(VGA_CIRRUS)) {
> > > -            vga_model = "cirrus";
> > >          } else if (vga_interface_available(VGA_STD)) {
> > >              vga_model = "std";
> > > +        } else if (vga_interface_available(VGA_CIRRUS)) {
> > > +            vga_model = "cirrus";
> > 
> > If we don't make the machines above have default_display=NULL, we
> > won't need this hunk, right?  In either case, I suggest doing
> > this change on a separate patch.
> 
> An alternative would be to define the default always to VGA_STD if the
> machine does not override it. This is from the observation that the majority
> would chose std. Obviously, having default values always has pro and cons. I
> would not consider it harmful here.

That would be my preference: not having any machine with
default_display==NULL.  I just don't want to make this a
requirement for fixing the current bug.

> 
> Is there any configuration that includes VGA_CIRRUS but not VGA_STD?

Not sure, I'll check.

-- 
Eduardo

On Wed, Jul 04, 2018 at 05:52:06PM -0300, Eduardo Habkost wrote:
> On Wed, Jul 04, 2018 at 10:21:19PM +0200, Sebastian Bauer wrote:
> > Am 2018-07-04 20:53, schrieb Eduardo Habkost:
> > > >      mc->kvm_type = spapr_kvm_type;
> > > >      machine_class_allow_dynamic_sysbus_dev(mc,
> > > > TYPE_SPAPR_PCI_HOST_BRIDGE);
> > > >      mc->pci_allow_0_address = true;
> > > > diff --git a/vl.c b/vl.c
> > > > index 16b913f9d5..e60bf2d6cd 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
> > > >      if (default_vga) {
> > > >          if (machine_class->default_display) {
> > > >              vga_model = machine_class->default_display;
> > > > -        } else if (vga_interface_available(VGA_CIRRUS)) {
> > > > -            vga_model = "cirrus";
> > > >          } else if (vga_interface_available(VGA_STD)) {
> > > >              vga_model = "std";
> > > > +        } else if (vga_interface_available(VGA_CIRRUS)) {
> > > > +            vga_model = "cirrus";
> > > 
> > > If we don't make the machines above have default_display=NULL, we
> > > won't need this hunk, right?  In either case, I suggest doing
> > > this change on a separate patch.
> > 
> > An alternative would be to define the default always to VGA_STD if the
> > machine does not override it. This is from the observation that the majority
> > would chose std. Obviously, having default values always has pro and cons. I
> > would not consider it harmful here.
> 
> That would be my preference: not having any machine with
> default_display==NULL.  I just don't want to make this a
> requirement for fixing the current bug.
> 
> > 
> > Is there any configuration that includes VGA_CIRRUS but not VGA_STD?
> 
> Not sure, I'll check.

I couldn't find any:

    [VGA_STD] = {
        .opt_name = "std",
        .name = "standard VGA",
        .class_names = { "VGA", "isa-vga" },
    },
    [VGA_CIRRUS] = {
        .opt_name = "cirrus",
        .name = "Cirrus VGA",
        .class_names = { "cirrus-vga", "isa-cirrus-vga" },
    },


"VGA" enabled by CONFIG_VGA_PCI:
hw/display/vga-pci.c:    .name          = "VGA",
hw/display/Makefile.objs:common-obj-$(CONFIG_VGA_PCI) += vga-pci.o

"isa-vga" enabled by CONFIG_VGA_ISA:
hw/display/vga-isa.c:#define TYPE_ISA_VGA "isa-vga"
hw/display/vga-isa.c:    .name          = TYPE_ISA_VGA,
hw/display/Makefile.objs:common-obj-$(CONFIG_VGA_ISA) += vga-isa.o

"cirrus-vga" enabled by CONFIG_VGA_CIRRUS:
hw/display/cirrus_vga.c:#define TYPE_PCI_CIRRUS_VGA "cirrus-vga"
hw/display/cirrus_vga.c:    .name          = TYPE_PCI_CIRRUS_VGA,
hw/display/Makefile.objs:common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o

"isa-cirrus-vga" enabled by CONFIG_VGA_CIRRUS:
hw/display/cirrus_vga.c:#define TYPE_ISA_CIRRUS_VGA "isa-cirrus-vga"
hw/display/cirrus_vga.c:    .name          = TYPE_ISA_CIRRUS_VGA,
hw/display/Makefile.objs:common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o

Build configs:

$ git grep -E 'CONFIG_VGA_(ISA|PCI|CIRRUS)|pci.mak'

default-configs/pci.mak:CONFIG_VGA_PCI=y

* 'include pci.mak' below means VGA_STD is available

default-configs/alpha-softmmu.mak:include pci.mak
default-configs/alpha-softmmu.mak:CONFIG_VGA_CIRRUS=y

* VGA_CIRRUS & VGA_STD.

default-configs/arm-softmmu.mak:include pci.mak

* VGA_STD only.

default-configs/hppa-softmmu.mak:include pci.mak

* VGA_STD only.

default-configs/i386-softmmu.mak:include pci.mak
default-configs/i386-softmmu.mak:CONFIG_VGA_ISA=y
default-configs/i386-softmmu.mak:CONFIG_VGA_CIRRUS=y

* VGA_CIRRUS & VGA_STD.

default-configs/mips-softmmu-common.mak:include pci.mak
default-configs/mips-softmmu-common.mak:CONFIG_VGA_ISA=y
default-configs/mips-softmmu-common.mak:CONFIG_VGA_ISA_MM=y
default-configs/mips-softmmu-common.mak:CONFIG_VGA_CIRRUS=y

* VGA_CIRRUS & VGA_STD.

default-configs/ppc-softmmu.mak:include pci.mak
default-configs/ppc-softmmu.mak:CONFIG_VGA_CIRRUS=y

* VGA_CIRRUS & VGA_STD.

default-configs/ppcemb-softmmu.mak:include pci.mak

* VGA_STD only.

default-configs/sh4-softmmu.mak:include pci.mak

* VGA_STD only.

default-configs/sh4eb-softmmu.mak:include pci.mak

* VGA_STD only.

default-configs/sparc64-softmmu.mak:include pci.mak

* VGA_STD only.

default-configs/x86_64-softmmu.mak:include pci.mak
default-configs/x86_64-softmmu.mak:CONFIG_VGA_ISA=y
default-configs/x86_64-softmmu.mak:CONFIG_VGA_CIRRUS=y

* VGA_CIRRUS & VGA_STD.

roms/config.vga-bochs-display:CONFIG_VGA_PCI=y
roms/config.vga-cirrus:CONFIG_VGA_CIRRUS=y
roms/config.vga-cirrus:CONFIG_VGA_PCI=y
roms/config.vga-isavga:CONFIG_VGA_PCI=n
roms/config.vga-qxl:CONFIG_VGA_PCI=y
roms/config.vga-ramfb:CONFIG_VGA_PCI=n
roms/config.vga-stdvga:CONFIG_VGA_PCI=y
roms/config.vga-virtio:CONFIG_VGA_PCI=y
roms/config.vga-vmware:CONFIG_VGA_PCI=y

* I have no idea what this is, I guess this is not related to the
  QEMU build system at all.  :)

-- 
Eduardo

Am 2018-07-04 23:04, schrieb Eduardo Habkost:
> On Wed, Jul 04, 2018 at 05:52:06PM -0300, Eduardo Habkost wrote:
>> On Wed, Jul 04, 2018 at 10:21:19PM +0200, Sebastian Bauer wrote:
>> > Am 2018-07-04 20:53, schrieb Eduardo Habkost:
>> > > >      mc->kvm_type = spapr_kvm_type;
>> > > >      machine_class_allow_dynamic_sysbus_dev(mc,
>> > > > TYPE_SPAPR_PCI_HOST_BRIDGE);
>> > > >      mc->pci_allow_0_address = true;
>> > > > diff --git a/vl.c b/vl.c
>> > > > index 16b913f9d5..e60bf2d6cd 100644
>> > > > --- a/vl.c
>> > > > +++ b/vl.c
>> > > > @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
>> > > >      if (default_vga) {
>> > > >          if (machine_class->default_display) {
>> > > >              vga_model = machine_class->default_display;
>> > > > -        } else if (vga_interface_available(VGA_CIRRUS)) {
>> > > > -            vga_model = "cirrus";
>> > > >          } else if (vga_interface_available(VGA_STD)) {
>> > > >              vga_model = "std";
>> > > > +        } else if (vga_interface_available(VGA_CIRRUS)) {
>> > > > +            vga_model = "cirrus";
>> > >
>> > > If we don't make the machines above have default_display=NULL, we
>> > > won't need this hunk, right?  In either case, I suggest doing
>> > > this change on a separate patch.
>> >
>> > An alternative would be to define the default always to VGA_STD if the
>> > machine does not override it. This is from the observation that the majority
>> > would chose std. Obviously, having default values always has pro and cons. I
>> > would not consider it harmful here.
>> 
>> That would be my preference: not having any machine with
>> default_display==NULL.  I just don't want to make this a
>> requirement for fixing the current bug.
>> 
>> >
>> > Is there any configuration that includes VGA_CIRRUS but not VGA_STD?
>> 
>> Not sure, I'll check.
> 
> I couldn't find any:

Thanks for checking! In that case, the VGA_CIRRUS branch in the above 
hunk will never be executed (for any current config) after the proposed 
patch. So that branch could be eliminated.

I also checked the code, default values are used also for other fields 
in that struct (e.g., default_cpus). If default values are considered 
fragile, they must be changed as well in the middle term. However, given 
a solid documentation (which is currently missing for these fields) I 
still think that default values are acceptable.

Note that there seems also be the possibility to set != NULL default 
values in the machine_class_init() function.

Bye
Sebastian

On Thu, Jul 05, 2018 at 05:43:45AM +0200, Sebastian Bauer wrote:
> Am 2018-07-04 23:04, schrieb Eduardo Habkost:
> > On Wed, Jul 04, 2018 at 05:52:06PM -0300, Eduardo Habkost wrote:
> > > On Wed, Jul 04, 2018 at 10:21:19PM +0200, Sebastian Bauer wrote:
> > > > Am 2018-07-04 20:53, schrieb Eduardo Habkost:
> > > > > >      mc->kvm_type = spapr_kvm_type;
> > > > > >      machine_class_allow_dynamic_sysbus_dev(mc,
> > > > > > TYPE_SPAPR_PCI_HOST_BRIDGE);
> > > > > >      mc->pci_allow_0_address = true;
> > > > > > diff --git a/vl.c b/vl.c
> > > > > > index 16b913f9d5..e60bf2d6cd 100644
> > > > > > --- a/vl.c
> > > > > > +++ b/vl.c
> > > > > > @@ -4475,10 +4475,10 @@ int main(int argc, char **argv, char **envp)
> > > > > >      if (default_vga) {
> > > > > >          if (machine_class->default_display) {
> > > > > >              vga_model = machine_class->default_display;
> > > > > > -        } else if (vga_interface_available(VGA_CIRRUS)) {
> > > > > > -            vga_model = "cirrus";
> > > > > >          } else if (vga_interface_available(VGA_STD)) {
> > > > > >              vga_model = "std";
> > > > > > +        } else if (vga_interface_available(VGA_CIRRUS)) {
> > > > > > +            vga_model = "cirrus";
> > > > >
> > > > > If we don't make the machines above have default_display=NULL, we
> > > > > won't need this hunk, right?  In either case, I suggest doing
> > > > > this change on a separate patch.
> > > >
> > > > An alternative would be to define the default always to VGA_STD if the
> > > > machine does not override it. This is from the observation that the majority
> > > > would chose std. Obviously, having default values always has pro and cons. I
> > > > would not consider it harmful here.
> > > 
> > > That would be my preference: not having any machine with
> > > default_display==NULL.  I just don't want to make this a
> > > requirement for fixing the current bug.
> > > 
> > > >
> > > > Is there any configuration that includes VGA_CIRRUS but not VGA_STD?
> > > 
> > > Not sure, I'll check.
> > 
> > I couldn't find any:
> 
> Thanks for checking! In that case, the VGA_CIRRUS branch in the above hunk
> will never be executed (for any current config) after the proposed patch. So
> that branch could be eliminated.

I'd prefer to eliminate both vga_interface_available() checks.

> 
> I also checked the code, default values are used also for other fields in
> that struct (e.g., default_cpus). If default values are considered fragile,
> they must be changed as well in the middle term. However, given a solid
> documentation (which is currently missing for these fields) I still think
> that default values are acceptable.
> 
> Note that there seems also be the possibility to set != NULL default values
> in the machine_class_init() function.

I don't mind if the default value is defined at
machine_class_init() or main().  What bothers me is a default
value that depends on vga_interface_available(), because it can
be easily broken by a build config change (as demonstrated by the
bug fixed by this patch).

-- 
Eduardo

On 05/07/18 14:12, Eduardo Habkost wrote:

>> I also checked the code, default values are used also for other fields in
>> that struct (e.g., default_cpus). If default values are considered fragile,
>> they must be changed as well in the middle term. However, given a solid
>> documentation (which is currently missing for these fields) I still think
>> that default values are acceptable.
>>
>> Note that there seems also be the possibility to set != NULL default values
>> in the machine_class_init() function.
> 
> I don't mind if the default value is defined at
> machine_class_init() or main().  What bothers me is a default
> value that depends on vga_interface_available(), because it can
> be easily broken by a build config change (as demonstrated by the
> bug fixed by this patch).

I've already sent David a for-3.0 patch to set stdvga for the PPC Mac 
machines, but should I also do the same for PReP too?

Is it also worth explicitly adding this for sun4u? And what about 
non-ISA/PCI cards such as sun4m's TCX/CG3?


ATB,

Mark.

On Thu, Jul 05, 2018 at 05:24:09PM +0100, Mark Cave-Ayland wrote:
> On 05/07/18 14:12, Eduardo Habkost wrote:
> 
> > > I also checked the code, default values are used also for other fields in
> > > that struct (e.g., default_cpus). If default values are considered fragile,
> > > they must be changed as well in the middle term. However, given a solid
> > > documentation (which is currently missing for these fields) I still think
> > > that default values are acceptable.
> > > 
> > > Note that there seems also be the possibility to set != NULL default values
> > > in the machine_class_init() function.
> > 
> > I don't mind if the default value is defined at
> > machine_class_init() or main().  What bothers me is a default
> > value that depends on vga_interface_available(), because it can
> > be easily broken by a build config change (as demonstrated by the
> > bug fixed by this patch).
> 
> I've already sent David a for-3.0 patch to set stdvga for the PPC Mac
> machines, but should I also do the same for PReP too?
> 
> Is it also worth explicitly adding this for sun4u? And what about
> non-ISA/PCI cards such as sun4m's TCX/CG3?
> 

I think it's worth it.  Having fewer machines with
default_display==NULL sounds like a good idea to me.

Having zero machines with default_display==NULL (so we can
finally delete the vga_interface_available() checks on main())
would be even better.

This is not urgent, though (unless there we plan to change
CONFIG_VGA_* on default-configs again in 3.0).

-- 
Eduardo

On Thu, Jul 05, 2018 at 05:24:09PM +0100, Mark Cave-Ayland wrote:
> On 05/07/18 14:12, Eduardo Habkost wrote:
> 
> > > I also checked the code, default values are used also for other fields in
> > > that struct (e.g., default_cpus). If default values are considered fragile,
> > > they must be changed as well in the middle term. However, given a solid
> > > documentation (which is currently missing for these fields) I still think
> > > that default values are acceptable.
> > > 
> > > Note that there seems also be the possibility to set != NULL default values
> > > in the machine_class_init() function.
> > 
> > I don't mind if the default value is defined at
> > machine_class_init() or main().  What bothers me is a default
> > value that depends on vga_interface_available(), because it can
> > be easily broken by a build config change (as demonstrated by the
> > bug fixed by this patch).
> 
> I've already sent David a for-3.0 patch to set stdvga for the PPC Mac
> machines, but should I also do the same for PReP too?

Makes sense, yes.

> Is it also worth explicitly adding this for sun4u? And what about
> non-ISA/PCI cards such as sun4m's TCX/CG3?

Hmm, sparc has its own mechanism to use tcx as default (IIRC you get tcx
unless -vga cg3 is specified).  Should be possible to switch this over
to use the MachineClass field instead.

cheers,
  Gerd

On 06/07/18 08:14, Gerd Hoffmann wrote:

> On Thu, Jul 05, 2018 at 05:24:09PM +0100, Mark Cave-Ayland wrote:
>> On 05/07/18 14:12, Eduardo Habkost wrote:
>>
>>>> I also checked the code, default values are used also for other fields in
>>>> that struct (e.g., default_cpus). If default values are considered fragile,
>>>> they must be changed as well in the middle term. However, given a solid
>>>> documentation (which is currently missing for these fields) I still think
>>>> that default values are acceptable.
>>>>
>>>> Note that there seems also be the possibility to set != NULL default values
>>>> in the machine_class_init() function.
>>>
>>> I don't mind if the default value is defined at
>>> machine_class_init() or main().  What bothers me is a default
>>> value that depends on vga_interface_available(), because it can
>>> be easily broken by a build config change (as demonstrated by the
>>> bug fixed by this patch).
>>
>> I've already sent David a for-3.0 patch to set stdvga for the PPC Mac
>> machines, but should I also do the same for PReP too?
> 
> Makes sense, yes.

Okay, done. This should ensure that the PPC Mac/PReP machines will 
always work regardless of what the outcome of your other patchset is.

>> Is it also worth explicitly adding this for sun4u? And what about
>> non-ISA/PCI cards such as sun4m's TCX/CG3?
> 
> Hmm, sparc has its own mechanism to use tcx as default (IIRC you get tcx
> unless -vga cg3 is specified).  Should be possible to switch this over
> to use the MachineClass field instead.

Thanks for the pointers - I'll have a look at this again when the 3.1 
development cycle opens.


ATB,

Mark.

> roms/config.vga-bochs-display:CONFIG_VGA_PCI=y
> roms/config.vga-cirrus:CONFIG_VGA_CIRRUS=y
> roms/config.vga-cirrus:CONFIG_VGA_PCI=y
> roms/config.vga-isavga:CONFIG_VGA_PCI=n
> roms/config.vga-qxl:CONFIG_VGA_PCI=y
> roms/config.vga-ramfb:CONFIG_VGA_PCI=n
> roms/config.vga-stdvga:CONFIG_VGA_PCI=y
> roms/config.vga-virtio:CONFIG_VGA_PCI=y
> roms/config.vga-vmware:CONFIG_VGA_PCI=y
> 
> * I have no idea what this is, I guess this is not related to the
>   QEMU build system at all.  :)

seabios config, for vgabios builds (make -C roms vgabios).

cheers,
  Gerd

> > -    m->default_display = "std";
> >      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
> 
> I wouldn't like to do this.  Long term, it would be a good idea
> to have zero machines with default_display==NULL, so let's keep
> default_machine="std" on PC?

Fine with me, I'll send a v2.

> > -        } else if (vga_interface_available(VGA_CIRRUS)) {
> > -            vga_model = "cirrus";
> >          } else if (vga_interface_available(VGA_STD)) {
> >              vga_model = "std";
> > +        } else if (vga_interface_available(VGA_CIRRUS)) {
> > +            vga_model = "cirrus";
> 
> If we don't make the machines above have default_display=NULL, we
> won't need this hunk, right?  In either case, I suggest doing
> this change on a separate patch.

ok.

cheers,
  Gerd