[PATCH v2] hw/arm/sbsa-ref: add GIC node into DT

[PATCH v2] hw/arm/sbsa-ref: add GIC node into DT

[Marcin Juszkiewicz]

Let add GIC information into DeviceTree as part of SBSA-REF versioning.

Trusted Firmware will read it and provide to next firmware level.

Bumps platform version to 0.1 one so we can check is node is present.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 792371fdce..9204e8605f 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -29,6 +29,7 @@
 #include "exec/hwaddr.h"
 #include "kvm_arm.h"
 #include "hw/arm/boot.h"
+#include "hw/arm/fdt.h"
 #include "hw/arm/smmuv3.h"
 #include "hw/block/flash.h"
 #include "hw/boards.h"
@@ -168,6 +169,20 @@ static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
     return arm_cpu_mp_affinity(idx, clustersz);
 }
 
+static void sbsa_fdt_add_gic_node(SBSAMachineState *sms)
+{
+    char *nodename;
+
+    nodename = g_strdup_printf("/intc");
+    qemu_fdt_add_subnode(sms->fdt, nodename);
+    qemu_fdt_setprop_sized_cells(sms->fdt, nodename, "reg",
+                                 2, sbsa_ref_memmap[SBSA_GIC_DIST].base,
+                                 2, sbsa_ref_memmap[SBSA_GIC_DIST].size,
+                                 2, sbsa_ref_memmap[SBSA_GIC_REDIST].base,
+                                 2, sbsa_ref_memmap[SBSA_GIC_REDIST].size);
+
+    g_free(nodename);
+}
 /*
  * Firmware on this machine only uses ACPI table to load OS, these limited
  * device tree nodes are just to let firmware know the info which varies from
@@ -204,7 +219,7 @@ static void create_fdt(SBSAMachineState *sms)
      *                        fw compatibility.
      */
     qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
-    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);
+    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 1);
 
     if (ms->numa_state->have_numa_distance) {
         int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t);
@@ -260,6 +275,8 @@ static void create_fdt(SBSAMachineState *sms)
 
         g_free(nodename);
     }
+
+    sbsa_fdt_add_gic_node(sms);
 }
 
 #define SBSA_FLASH_SECTOR_SIZE (256 * KiB)
-- 
2.40.1

Re: [PATCH v2] hw/arm/sbsa-ref: add GIC node into DT

[Leif Lindholm]

On 2023-05-17 11:53, Marcin Juszkiewicz wrote:
> Let add GIC information into DeviceTree as part of SBSA-REF versioning.
> 
> Trusted Firmware will read it and provide to next firmware level.
> 
> Bumps platform version to 0.1 one so we can check is node is present.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

> ---
>   hw/arm/sbsa-ref.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 792371fdce..9204e8605f 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -29,6 +29,7 @@
>   #include "exec/hwaddr.h"
>   #include "kvm_arm.h"
>   #include "hw/arm/boot.h"
> +#include "hw/arm/fdt.h"
>   #include "hw/arm/smmuv3.h"
>   #include "hw/block/flash.h"
>   #include "hw/boards.h"
> @@ -168,6 +169,20 @@ static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
>       return arm_cpu_mp_affinity(idx, clustersz);
>   }
>   
> +static void sbsa_fdt_add_gic_node(SBSAMachineState *sms)
> +{
> +    char *nodename;
> +
> +    nodename = g_strdup_printf("/intc");
> +    qemu_fdt_add_subnode(sms->fdt, nodename);
> +    qemu_fdt_setprop_sized_cells(sms->fdt, nodename, "reg",
> +                                 2, sbsa_ref_memmap[SBSA_GIC_DIST].base,
> +                                 2, sbsa_ref_memmap[SBSA_GIC_DIST].size,
> +                                 2, sbsa_ref_memmap[SBSA_GIC_REDIST].base,
> +                                 2, sbsa_ref_memmap[SBSA_GIC_REDIST].size);
> +
> +    g_free(nodename);
> +}
>   /*
>    * Firmware on this machine only uses ACPI table to load OS, these limited
>    * device tree nodes are just to let firmware know the info which varies from
> @@ -204,7 +219,7 @@ static void create_fdt(SBSAMachineState *sms)
>        *                        fw compatibility.
>        */
>       qemu_fdt_setprop_cell(fdt, "/", "machine-version-major", 0);
> -    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 0);
> +    qemu_fdt_setprop_cell(fdt, "/", "machine-version-minor", 1);
>   
>       if (ms->numa_state->have_numa_distance) {
>           int size = nb_numa_nodes * nb_numa_nodes * 3 * sizeof(uint32_t);
> @@ -260,6 +275,8 @@ static void create_fdt(SBSAMachineState *sms)
>   
>           g_free(nodename);
>       }
> +
> +    sbsa_fdt_add_gic_node(sms);
>   }
>   
>   #define SBSA_FLASH_SECTOR_SIZE (256 * KiB)

Re: [PATCH v2] hw/arm/sbsa-ref: add GIC node into DT

[Peter Maydell]

On Wed, 17 May 2023 at 11:53, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> Let add GIC information into DeviceTree as part of SBSA-REF versioning.
>
> Trusted Firmware will read it and provide to next firmware level.
>
> Bumps platform version to 0.1 one so we can check is node is present.
>
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

Are we documenting anywhere what the format/requirements/versions
of this board-to-firmware interface are ?

thanks
-- PMM

[PATCH 1/2] docs: sbsa: correct graphics card name

[Marcin Juszkiewicz]

We moved from VGA to Bochs to have PCIe card.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 docs/system/arm/sbsa.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
index b499d7e927..fea4992df2 100644
--- a/docs/system/arm/sbsa.rst
+++ b/docs/system/arm/sbsa.rst
@@ -27,6 +27,6 @@ The sbsa-ref board supports:
   - System bus EHCI controller
   - CDROM and hard disc on AHCI bus
   - E1000E ethernet card on PCIe bus
-  - VGA display adaptor on PCIe bus
+  - Bochs display adaptor on PCIe bus
   - A generic SBSA watchdog device
 
-- 
2.40.1

[PATCH 2/2] docs: sbsa: document platform version changes

[Marcin Juszkiewicz]

We plan to start using sbsa-ref platform version fields
in DT so firmware does not have to use hardcoded values.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 docs/system/arm/sbsa.rst | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
index fea4992df2..fb17744e95 100644
--- a/docs/system/arm/sbsa.rst
+++ b/docs/system/arm/sbsa.rst
@@ -6,16 +6,35 @@ any real hardware the ``sbsa-ref`` board intends to look like real
 hardware. The `Server Base System Architecture
 <https://developer.arm.com/documentation/den0029/latest>`_ defines a
 minimum base line of hardware support and importantly how the firmware
-reports that to any operating system. It is a static system that
-reports a very minimal DT to the firmware for non-discoverable
-information about components affected by the qemu command line (i.e.
-cpus and memory). As a result it must have a firmware specifically
-built to expect a certain hardware layout (as you would in a real
-machine).
+reports that to any operating system.
 
 It is intended to be a machine for developing firmware and testing
 standards compliance with operating systems.
 
+Platform versions
+"""""""""""""""""
+
+QEMU 7.1 brought support for "platform version major/minor" fields in
+DeviceTree.
+
+Version 0.0
+'''''''''''
+
+It is a static system that reports a very minimal DT to the firmware for
+non-discoverable information about components affected by the qemu
+command line (i.e. cpus and memory). As a result it must have a firmware
+specifically built to expect a certain hardware layout (as you would in
+a real machine).
+
+Version 0.1
+'''''''''''
+
+Additional data are provided in DT to the firmware:
+  - address and size of GIC Distributor
+  - address and size of GIC Redistributor
+
+Simple "/intc/reg" field is used.
+
 Supported devices
 """""""""""""""""
 
-- 
2.40.1

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Thomas Huth]

On 23/05/2023 17.56, Marcin Juszkiewicz wrote:
> We moved from VGA to Bochs to have PCIe card.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   docs/system/arm/sbsa.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
> index b499d7e927..fea4992df2 100644
> --- a/docs/system/arm/sbsa.rst
> +++ b/docs/system/arm/sbsa.rst
> @@ -27,6 +27,6 @@ The sbsa-ref board supports:
>     - System bus EHCI controller
>     - CDROM and hard disc on AHCI bus
>     - E1000E ethernet card on PCIe bus
> -  - VGA display adaptor on PCIe bus
> +  - Bochs display adaptor on PCIe bus
>     - A generic SBSA watchdog device
>   

While you're at it, I'd suggest to replace "adaptor" with "adapter" which 
seems to be way more common in the QEMU sources:

$ grep -r adaptor * | wc -l
5
$ grep -r adapter * | wc -l
385

With that changed:
Reviewed-by: Thomas Huth <thuth@redhat.com>


PS: An idea for another patch: I think the "config SBSA_REF" in 
hw/arm/Kconfig should select BOCHS_DISPLAY now, since this board seems to 
always require this card now (is there a reason why it can't be disabled 
with "-vga none" or "-nodefaults"?)

[PATCH v2 1/3] docs: sbsa: correct graphics card name

[Marcin Juszkiewicz]

We moved from VGA to Bochs to have PCIe card.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 docs/system/arm/sbsa.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
index b499d7e927..016776aed8 100644
--- a/docs/system/arm/sbsa.rst
+++ b/docs/system/arm/sbsa.rst
@@ -27,6 +27,6 @@ The sbsa-ref board supports:
   - System bus EHCI controller
   - CDROM and hard disc on AHCI bus
   - E1000E ethernet card on PCIe bus
-  - VGA display adaptor on PCIe bus
+  - Bochs display adapter on PCIe bus
   - A generic SBSA watchdog device
 
-- 
2.40.1

[PATCH v2 2/3] docs: sbsa: document platform version changes

[Marcin Juszkiewicz]

We plan to start using sbsa-ref platform version fields
in DT so firmware does not have to use hardcoded values.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 docs/system/arm/sbsa.rst | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
index 016776aed8..922a29700d 100644
--- a/docs/system/arm/sbsa.rst
+++ b/docs/system/arm/sbsa.rst
@@ -6,16 +6,35 @@ any real hardware the ``sbsa-ref`` board intends to look like real
 hardware. The `Server Base System Architecture
 <https://developer.arm.com/documentation/den0029/latest>`_ defines a
 minimum base line of hardware support and importantly how the firmware
-reports that to any operating system. It is a static system that
-reports a very minimal DT to the firmware for non-discoverable
-information about components affected by the qemu command line (i.e.
-cpus and memory). As a result it must have a firmware specifically
-built to expect a certain hardware layout (as you would in a real
-machine).
+reports that to any operating system.
 
 It is intended to be a machine for developing firmware and testing
 standards compliance with operating systems.
 
+Platform versions
+"""""""""""""""""
+
+QEMU 7.1 brought support for "platform version major/minor" fields in
+DeviceTree.
+
+Version 0.0
+'''''''''''
+
+It is a static system that reports a very minimal DT to the firmware for
+non-discoverable information about components affected by the qemu
+command line (i.e. cpus and memory). As a result it must have a firmware
+specifically built to expect a certain hardware layout (as you would in
+a real machine).
+
+Version 0.1
+'''''''''''
+
+Additional data are provided in DT to the firmware:
+  - address and size of GIC Distributor
+  - address and size of GIC Redistributor
+
+Simple "/intc/reg" field is used.
+
 Supported devices
 """""""""""""""""
 
-- 
2.40.1

[PATCH v2 3/3] hw/arm/Kconfig: sbsa-ref uses Bochs display

[Marcin Juszkiewicz]

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 0f42c556d7..4484de67e8 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -249,6 +249,7 @@ config SBSA_REF
     select PL061 # GPIO
     select USB_EHCI_SYSBUS
     select WDT_SBSA
+    select BOCHS_DISPLAY
 
 config SABRELITE
     bool
-- 
2.40.1

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Marcin Juszkiewicz]

W dniu 23.05.2023 o 19:11, Thomas Huth pisze:
> On 23/05/2023 17.56, Marcin Juszkiewicz wrote:
>> We moved from VGA to Bochs to have PCIe card.
>>
>> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
>> ---
>>   docs/system/arm/sbsa.rst | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
>> index b499d7e927..fea4992df2 100644
>> --- a/docs/system/arm/sbsa.rst
>> +++ b/docs/system/arm/sbsa.rst
>> @@ -27,6 +27,6 @@ The sbsa-ref board supports:
>>     - System bus EHCI controller
>>     - CDROM and hard disc on AHCI bus
>>     - E1000E ethernet card on PCIe bus
>> -  - VGA display adaptor on PCIe bus
>> +  - Bochs display adaptor on PCIe bus
>>     - A generic SBSA watchdog device
> 
> While you're at it, I'd suggest to replace "adaptor" with "adapter" 
> which seems to be way more common in the QEMU sources:
> 
> $ grep -r adaptor * | wc -l
> 5
> $ grep -r adapter * | wc -l
> 385
> 
> With that changed:
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks. changed.


> PS: An idea for another patch: I think the "config SBSA_REF" in 
> hw/arm/Kconfig should select BOCHS_DISPLAY now, since this board seems 
> to always require this card now 

Thanks, patch sent.

> (is there a reason why it can't be disabled with "-vga none" or "-nodefaults"?)

That's something I need to check how it should be done. Should it also 
drop default_nic?

Re: [PATCH v2 1/3] docs: sbsa: correct graphics card name

[Thomas Huth]

On 23/05/2023 19.28, Marcin Juszkiewicz wrote:
> We moved from VGA to Bochs to have PCIe card.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   docs/system/arm/sbsa.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/system/arm/sbsa.rst b/docs/system/arm/sbsa.rst
> index b499d7e927..016776aed8 100644
> --- a/docs/system/arm/sbsa.rst
> +++ b/docs/system/arm/sbsa.rst
> @@ -27,6 +27,6 @@ The sbsa-ref board supports:
>     - System bus EHCI controller
>     - CDROM and hard disc on AHCI bus
>     - E1000E ethernet card on PCIe bus
> -  - VGA display adaptor on PCIe bus
> +  - Bochs display adapter on PCIe bus
>     - A generic SBSA watchdog device
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Thomas Huth]

On 23/05/2023 19.30, Marcin Juszkiewicz wrote:
...
>> (is there a reason why it can't be disabled with "-vga none" or 
>> "-nodefaults"?)
> 
> That's something I need to check how it should be done.

Other boards set mc->default_display in their ...class_init
function and then use pci_vga_init() (or vga_interface_type)
to instantiate their default display adapter ... however, that
does not seem to support the bochs adapter yet (see
vga_interfaces[] in softmmu/vl.c).

Not sure whether it's worth the effort to extend vga_interfaces[]
in vl.c, but you could at least check whether vga_interface_type
is VGA_NONE and skip the creation of the bochs adapter in that
case?

> Should it also drop default_nic?

Seems like sbsa-ref already uses nd_table[], so "-net none" should
already work. For "configure --without-default-devices" builds, we
still need a patch like this on top, though:

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -596,6 +596,7 @@ static void create_pcie(SBSAMachineState *sms)
      hwaddr size_mmio_high = sbsa_ref_memmap[SBSA_PCIE_MMIO_HIGH].size;
      hwaddr base_pio = sbsa_ref_memmap[SBSA_PCIE_PIO].base;
      int irq = sbsa_ref_irqmap[SBSA_PCIE];
+    MachineClass *mc = MACHINE_GET_CLASS(sms);
      MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
      MemoryRegion *ecam_alias, *ecam_reg;
      DeviceState *dev;
@@ -641,7 +642,7 @@ static void create_pcie(SBSAMachineState *sms)
              NICInfo *nd = &nd_table[i];
  
              if (!nd->model) {
-                nd->model = g_strdup("e1000e");
+                nd->model = g_strdup(mc->default_nic);
              }
  
              pci_nic_init_nofail(nd, pci->bus, nd->model, NULL);
@@ -858,6 +859,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
      mc->minimum_page_bits = 12;
      mc->block_default_type = IF_IDE;
      mc->no_cdrom = 1;
+    mc->default_nic = "e1000e";
      mc->default_ram_size = 1 * GiB;
      mc->default_ram_id = "sbsa-ref.ram";
      mc->default_cpus = 4;

(I'm doing that default_nic change for a lot of other boards
currently, so I can send a proper patch for this later)

  Thomas

Re: [PATCH v2 3/3] hw/arm/Kconfig: sbsa-ref uses Bochs display

[Thomas Huth]

On 23/05/2023 19.28, Marcin Juszkiewicz wrote:
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 0f42c556d7..4484de67e8 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -249,6 +249,7 @@ config SBSA_REF
>       select PL061 # GPIO
>       select USB_EHCI_SYSBUS
>       select WDT_SBSA
> +    select BOCHS_DISPLAY
>   
>   config SABRELITE
>       bool

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 1/3] hw/arm/sbsa-ref: honor "-vga none" argument

[Marcin Juszkiewicz]

In case someone wants to run without graphics card.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 9c3e670ec6..c540b2f1ba 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -649,7 +649,9 @@ static void create_pcie(SBSAMachineState *sms)
         }
     }
 
-    pci_create_simple(pci->bus, -1, "bochs-display");
+    if (vga_interface_type != VGA_NONE) {
+        pci_create_simple(pci->bus, -1, "bochs-display");
+    }
 
     create_smmu(sms, pci->bus);
 }
-- 
2.40.1

[PATCH 2/3] hw/arm/sbsa-ref: add gfx card only if we have pci

[Marcin Juszkiewicz]

Creation of network card is guarded with check do we
have pci bus. Do the same with graphics card.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index c540b2f1ba..9a3d77d6b6 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -647,10 +647,10 @@ static void create_pcie(SBSAMachineState *sms)
 
             pci_nic_init_nofail(nd, pci->bus, nd->model, NULL);
         }
-    }
 
-    if (vga_interface_type != VGA_NONE) {
-        pci_create_simple(pci->bus, -1, "bochs-display");
+        if (vga_interface_type != VGA_NONE) {
+            pci_create_simple(pci->bus, -1, "bochs-display");
+        }
     }
 
     create_smmu(sms, pci->bus);
-- 
2.40.1

[PATCH 3/3] hw/arm/sbsa-ref: use MachineClass->default_display

[Marcin Juszkiewicz]

Mark the default graphica via the new MachineClass->default_display
setting so that the machine-defaults code in vl.c can decide whether the
default graphics is usable or not (for example when compiling with the
"--without-default-devices" configure switch).

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 9a3d77d6b6..30ce7f7db4 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -649,7 +649,7 @@ static void create_pcie(SBSAMachineState *sms)
         }
 
         if (vga_interface_type != VGA_NONE) {
-            pci_create_simple(pci->bus, -1, "bochs-display");
+            pci_create_simple(pci->bus, -1, mc->default_display);
         }
     }
 
@@ -865,6 +865,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "sbsa-ref.ram";
     mc->default_cpus = 4;
+    mc->default_display = "bochs-display";
     mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
     mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
     mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
-- 
2.40.1

Re: [PATCH 1/3] hw/arm/sbsa-ref: honor "-vga none" argument

[Thomas Huth]

On 24/05/2023 10.39, Marcin Juszkiewicz wrote:
> In case someone wants to run without graphics card.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 9c3e670ec6..c540b2f1ba 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -649,7 +649,9 @@ static void create_pcie(SBSAMachineState *sms)
>           }
>       }
>   
> -    pci_create_simple(pci->bus, -1, "bochs-display");
> +    if (vga_interface_type != VGA_NONE) {
> +        pci_create_simple(pci->bus, -1, "bochs-display");
> +    }
>   
>       create_smmu(sms, pci->bus);
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/3] hw/arm/sbsa-ref: add gfx card only if we have pci

[Thomas Huth]

On 24/05/2023 10.39, Marcin Juszkiewicz wrote:
> Creation of network card is guarded with check do we
> have pci bus. Do the same with graphics card.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index c540b2f1ba..9a3d77d6b6 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -647,10 +647,10 @@ static void create_pcie(SBSAMachineState *sms)
>   
>               pci_nic_init_nofail(nd, pci->bus, nd->model, NULL);
>           }
> -    }
>   
> -    if (vga_interface_type != VGA_NONE) {
> -        pci_create_simple(pci->bus, -1, "bochs-display");
> +        if (vga_interface_type != VGA_NONE) {
> +            pci_create_simple(pci->bus, -1, "bochs-display");
> +        }
>       }
>   
>       create_smmu(sms, pci->bus);

I wonder whether pci->bus can ever be NULL in this function?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/3] hw/arm/sbsa-ref: use MachineClass->default_display

[Thomas Huth]

On 24/05/2023 10.39, Marcin Juszkiewicz wrote:
> Mark the default graphica via the new MachineClass->default_display
> setting so that the machine-defaults code in vl.c can decide whether the
> default graphics is usable or not (for example when compiling with the
> "--without-default-devices" configure switch).
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 9a3d77d6b6..30ce7f7db4 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -649,7 +649,7 @@ static void create_pcie(SBSAMachineState *sms)
>           }
>   
>           if (vga_interface_type != VGA_NONE) {
> -            pci_create_simple(pci->bus, -1, "bochs-display");
> +            pci_create_simple(pci->bus, -1, mc->default_display);

Based-on: <20230524082037.1620952-1-thuth@redhat.com>
(otherwise you don't have the "mc" variable available here)

>           }
>       }
>   
> @@ -865,6 +865,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
>       mc->default_ram_size = 1 * GiB;
>       mc->default_ram_id = "sbsa-ref.ram";
>       mc->default_cpus = 4;
> +    mc->default_display = "bochs-display";

I guess this might not work as expected yet...

The code in vl.c looks through the vga_interfaces[] array and warns if it 
cannot find the default_display there... and since there is no entry for the 
bochs-display in that array, you likely now always get this warning message, 
even if it is available?

So I think you either have to drop this patch, or you've got to add an entry 
for the bochs-display to vga_interfaces[], too.

  Thomas

Re: [PATCH 3/3] hw/arm/sbsa-ref: use MachineClass->default_display

[Thomas Huth]

On 24/05/2023 10.39, Marcin Juszkiewicz wrote:
> Mark the default graphica via the new MachineClass->default_display
> setting so that the machine-defaults code in vl.c can decide whether the
> default graphics is usable or not (for example when compiling with the
> "--without-default-devices" configure switch).
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 9a3d77d6b6..30ce7f7db4 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -649,7 +649,7 @@ static void create_pcie(SBSAMachineState *sms)
>           }
>   
>           if (vga_interface_type != VGA_NONE) {
> -            pci_create_simple(pci->bus, -1, "bochs-display");
> +            pci_create_simple(pci->bus, -1, mc->default_display);

Based-on: <20230524082037.1620952-1-thuth@redhat.com>
(otherwise you don't have the "mc" variable available here)

>           }
>       }
>   
> @@ -865,6 +865,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
>       mc->default_ram_size = 1 * GiB;
>       mc->default_ram_id = "sbsa-ref.ram";
>       mc->default_cpus = 4;
> +    mc->default_display = "bochs-display";
>       mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
>       mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
>       mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;

I guess this might not work as expected yet...

The code in vl.c looks through the vga_interfaces[] array and warns if it 
cannot find the default_display there... and since there is no entry for the 
bochs-display in that array, you likely now always get this warning message, 
even if it is available?

So I think you either have to drop this patch, or you've got to add an entry 
for the bochs-display to vga_interfaces[], too.

  Thomas

Re: [PATCH 1/3] hw/arm/sbsa-ref: honor "-vga none" argument

[Thomas Huth]

On 24/05/2023 10.39, Marcin Juszkiewicz wrote:
> In case someone wants to run without graphics card.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 9c3e670ec6..c540b2f1ba 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -649,7 +649,9 @@ static void create_pcie(SBSAMachineState *sms)
>           }
>       }
>   
> -    pci_create_simple(pci->bus, -1, "bochs-display");
> +    if (vga_interface_type != VGA_NONE) {
> +        pci_create_simple(pci->bus, -1, "bochs-display");
> +    }
>   
>       create_smmu(sms, pci->bus);
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/3] hw/arm/sbsa-ref: add gfx card only if we have pci

[Thomas Huth]

On 24/05/2023 10.39, Marcin Juszkiewicz wrote:
> Creation of network card is guarded with check do we
> have pci bus. Do the same with graphics card.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
>   hw/arm/sbsa-ref.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index c540b2f1ba..9a3d77d6b6 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -647,10 +647,10 @@ static void create_pcie(SBSAMachineState *sms)
>   
>               pci_nic_init_nofail(nd, pci->bus, nd->model, NULL);
>           }
> -    }
>   
> -    if (vga_interface_type != VGA_NONE) {
> -        pci_create_simple(pci->bus, -1, "bochs-display");
> +        if (vga_interface_type != VGA_NONE) {
> +            pci_create_simple(pci->bus, -1, "bochs-display");
> +        }
>       }

I wonder whether pci->bus can ever be NULL in this function?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 1/5] hw/arm: Use MachineClass->default_nic in the sbsa-ref machine

[Marcin Juszkiewicz]

From: Thomas Huth <thuth@redhat.com>

Mark the default NIC via the new MachineClass->default_nic setting
so that the machine-defaults code in vl.c can decide whether the
default NIC is usable or not (for example when compiling with the
"--without-default-devices" configure switch).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/sbsa-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 792371fdce..9c3e670ec6 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -596,6 +596,7 @@ static void create_pcie(SBSAMachineState *sms)
     hwaddr size_mmio_high = sbsa_ref_memmap[SBSA_PCIE_MMIO_HIGH].size;
     hwaddr base_pio = sbsa_ref_memmap[SBSA_PCIE_PIO].base;
     int irq = sbsa_ref_irqmap[SBSA_PCIE];
+    MachineClass *mc = MACHINE_GET_CLASS(sms);
     MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
     MemoryRegion *ecam_alias, *ecam_reg;
     DeviceState *dev;
@@ -641,7 +642,7 @@ static void create_pcie(SBSAMachineState *sms)
             NICInfo *nd = &nd_table[i];
 
             if (!nd->model) {
-                nd->model = g_strdup("e1000e");
+                nd->model = g_strdup(mc->default_nic);
             }
 
             pci_nic_init_nofail(nd, pci->bus, nd->model, NULL);
@@ -858,6 +859,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
     mc->minimum_page_bits = 12;
     mc->block_default_type = IF_IDE;
     mc->no_cdrom = 1;
+    mc->default_nic = "e1000e";
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "sbsa-ref.ram";
     mc->default_cpus = 4;
-- 
2.40.1

[PATCH v2 2/5] Add Bochs to list of vga_interfaces

[Marcin Juszkiewicz]

arm/sbsa-ref uses Bochs-display graphics card and without it being
present in vga_interfaces "-vga none" argument handling cannot be added.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 include/sysemu/sysemu.h | 2 +-
 softmmu/vl.c            | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 25be2a692e..9713a1b470 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -29,7 +29,7 @@ extern int autostart;
 
 typedef enum {
     VGA_NONE, VGA_STD, VGA_CIRRUS, VGA_VMWARE, VGA_XENFB, VGA_QXL,
-    VGA_TCX, VGA_CG3, VGA_DEVICE, VGA_VIRTIO,
+    VGA_TCX, VGA_CG3, VGA_DEVICE, VGA_VIRTIO, VGA_BOCHS,
     VGA_TYPE_MAX,
 } VGAInterfaceType;
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index b0b96f67fa..07e6030875 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -216,6 +216,7 @@ static struct {
     { .driver = "ati-vga",              .flag = &default_vga       },
     { .driver = "vhost-user-vga",       .flag = &default_vga       },
     { .driver = "virtio-vga-gl",        .flag = &default_vga       },
+    { .driver = "bochs-display",        .flag = &default_vga       },
 };
 
 static QemuOptsList qemu_rtc_opts = {
@@ -935,6 +936,11 @@ static const VGAInterfaceInfo vga_interfaces[VGA_TYPE_MAX] = {
         .name = "CG3 framebuffer",
         .class_names = { "cgthree" },
     },
+    [VGA_BOCHS] = {
+        .opt_name = "bochs-display",
+        .name = "Bochs framebuffer",
+        .class_names = { "bochs-display" },
+    },
 #ifdef CONFIG_XEN_BACKEND
     [VGA_XENFB] = {
         .opt_name = "xenfb",
-- 
2.40.1

[PATCH v2 3/5] hw/arm/sbsa-ref: honor "-vga none" argument

[Marcin Juszkiewicz]

In case someone wants to run without graphics card.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 9c3e670ec6..c540b2f1ba 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -649,7 +649,9 @@ static void create_pcie(SBSAMachineState *sms)
         }
     }
 
-    pci_create_simple(pci->bus, -1, "bochs-display");
+    if (vga_interface_type != VGA_NONE) {
+        pci_create_simple(pci->bus, -1, "bochs-display");
+    }
 
     create_smmu(sms, pci->bus);
 }
-- 
2.40.1

[PATCH v2 4/5] hw/arm/sbsa-ref: add gfx card only if we have pci

[Marcin Juszkiewicz]

Creation of network card is guarded with check do we
have pci bus. Do the same with graphics card.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index c540b2f1ba..9a3d77d6b6 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -647,10 +647,10 @@ static void create_pcie(SBSAMachineState *sms)
 
             pci_nic_init_nofail(nd, pci->bus, nd->model, NULL);
         }
-    }
 
-    if (vga_interface_type != VGA_NONE) {
-        pci_create_simple(pci->bus, -1, "bochs-display");
+        if (vga_interface_type != VGA_NONE) {
+            pci_create_simple(pci->bus, -1, "bochs-display");
+        }
     }
 
     create_smmu(sms, pci->bus);
-- 
2.40.1

[PATCH v2 5/5] hw/arm/sbsa-ref: use MachineClass->default_display

[Marcin Juszkiewicz]

Mark the default graphica via the new MachineClass->default_display
setting so that the machine-defaults code in vl.c can decide whether the
default graphics is usable or not (for example when compiling with the
"--without-default-devices" configure switch).

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
 hw/arm/sbsa-ref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 9a3d77d6b6..30ce7f7db4 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -649,7 +649,7 @@ static void create_pcie(SBSAMachineState *sms)
         }
 
         if (vga_interface_type != VGA_NONE) {
-            pci_create_simple(pci->bus, -1, "bochs-display");
+            pci_create_simple(pci->bus, -1, mc->default_display);
         }
     }
 
@@ -865,6 +865,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "sbsa-ref.ram";
     mc->default_cpus = 4;
+    mc->default_display = "bochs-display";
     mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
     mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
     mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
-- 
2.40.1

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Peter Maydell]

On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote:
>
> On 23/05/2023 19.30, Marcin Juszkiewicz wrote:
> ...
> >> (is there a reason why it can't be disabled with "-vga none" or
> >> "-nodefaults"?)
> >
> > That's something I need to check how it should be done.
>
> Other boards set mc->default_display in their ...class_init
> function and then use pci_vga_init() (or vga_interface_type)
> to instantiate their default display adapter ... however, that
> does not seem to support the bochs adapter yet (see
> vga_interfaces[] in softmmu/vl.c).
>
> Not sure whether it's worth the effort to extend vga_interfaces[]
> in vl.c

Isn't that a legacy-command-line-option thing? I feel like
the recommended way to select a different graphics card
these days would be to use -device my-pci-vga-card ...

thanks
-- PMM

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Thomas Huth]

On 25/05/2023 12.05, Peter Maydell wrote:
> On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 23/05/2023 19.30, Marcin Juszkiewicz wrote:
>> ...
>>>> (is there a reason why it can't be disabled with "-vga none" or
>>>> "-nodefaults"?)
>>>
>>> That's something I need to check how it should be done.
>>
>> Other boards set mc->default_display in their ...class_init
>> function and then use pci_vga_init() (or vga_interface_type)
>> to instantiate their default display adapter ... however, that
>> does not seem to support the bochs adapter yet (see
>> vga_interfaces[] in softmmu/vl.c).
>>
>> Not sure whether it's worth the effort to extend vga_interfaces[]
>> in vl.c
> 
> Isn't that a legacy-command-line-option thing? I feel like
> the recommended way to select a different graphics card
> these days would be to use -device my-pci-vga-card ...

"-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the 
graphics card to be always available, so if you add a "-device 
something-vga-card" on the command line, you'd get two graphic cards on your 
machine, even if you use -nodefaults.

So there needs to be at least some logic dealing with vga_interface_type if 
we want to be able to select a different graphics card for this machine. 
Then why not go the full way and use pci_vga_init() here, too? ... that's 
certainly the least confusing way for the users.

  Thomas

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Peter Maydell]

On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote:
>
> On 25/05/2023 12.05, Peter Maydell wrote:
> > On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> On 23/05/2023 19.30, Marcin Juszkiewicz wrote:
> >> ...
> >>>> (is there a reason why it can't be disabled with "-vga none" or
> >>>> "-nodefaults"?)
> >>>
> >>> That's something I need to check how it should be done.
> >>
> >> Other boards set mc->default_display in their ...class_init
> >> function and then use pci_vga_init() (or vga_interface_type)
> >> to instantiate their default display adapter ... however, that
> >> does not seem to support the bochs adapter yet (see
> >> vga_interfaces[] in softmmu/vl.c).
> >>
> >> Not sure whether it's worth the effort to extend vga_interfaces[]
> >> in vl.c
> >
> > Isn't that a legacy-command-line-option thing? I feel like
> > the recommended way to select a different graphics card
> > these days would be to use -device my-pci-vga-card ...
>
> "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the
> graphics card to be always available, so if you add a "-device
> something-vga-card" on the command line, you'd get two graphic cards on your
> machine, even if you use -nodefaults.

At least some boards do "only create the default graphics
type if vga_interface_type != VGA_NONE".

Mostly what I would like here is consistency. But also, we
haven't added a new item to the '-vga' option list since
2014, which makes me strongly suspect it's legacy and we
should only be keeping it for backwards compatibility.

> So there needs to be at least some logic dealing with vga_interface_type if
> we want to be able to select a different graphics card for this machine.
> Then why not go the full way and use pci_vga_init() here, too? ... that's
> certainly the least confusing way for the users.

Is it? From an Arm perspective, having "-vga" do anything
at all is pretty confusing: it's a rather PC-centric option name.
(Also pretty noticeable for the Sparc TCX/CG3 framebuffers,
which are not VGA in any way.)

thanks
-- PMM

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Thomas Huth]

On 25/05/2023 12.44, Peter Maydell wrote:
> On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 25/05/2023 12.05, Peter Maydell wrote:
>>> On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 23/05/2023 19.30, Marcin Juszkiewicz wrote:
>>>> ...
>>>>>> (is there a reason why it can't be disabled with "-vga none" or
>>>>>> "-nodefaults"?)
>>>>>
>>>>> That's something I need to check how it should be done.
>>>>
>>>> Other boards set mc->default_display in their ...class_init
>>>> function and then use pci_vga_init() (or vga_interface_type)
>>>> to instantiate their default display adapter ... however, that
>>>> does not seem to support the bochs adapter yet (see
>>>> vga_interfaces[] in softmmu/vl.c).
>>>>
>>>> Not sure whether it's worth the effort to extend vga_interfaces[]
>>>> in vl.c
>>>
>>> Isn't that a legacy-command-line-option thing? I feel like
>>> the recommended way to select a different graphics card
>>> these days would be to use -device my-pci-vga-card ...
>>
>> "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the
>> graphics card to be always available, so if you add a "-device
>> something-vga-card" on the command line, you'd get two graphic cards on your
>> machine, even if you use -nodefaults.
> 
> At least some boards do "only create the default graphics
> type if vga_interface_type != VGA_NONE".
> 
> Mostly what I would like here is consistency. But also, we
> haven't added a new item to the '-vga' option list since
> 2014, which makes me strongly suspect it's legacy and we
> should only be keeping it for backwards compatibility.
> 
>> So there needs to be at least some logic dealing with vga_interface_type if
>> we want to be able to select a different graphics card for this machine.
>> Then why not go the full way and use pci_vga_init() here, too? ... that's
>> certainly the least confusing way for the users.
> 
> Is it? From an Arm perspective, having "-vga" do anything
> at all is pretty confusing: it's a rather PC-centric option name.
> (Also pretty noticeable for the Sparc TCX/CG3 framebuffers,
> which are not VGA in any way.)

Ok, if this is rather an oddity on arm, then it's maybe better to do the 
"vga_interface_type != VGA_NONE" check instead.

  Thomas

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Mark Cave-Ayland]

On 25/05/2023 11:44, Peter Maydell wrote:

> On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 25/05/2023 12.05, Peter Maydell wrote:
>>> On Tue, 23 May 2023 at 19:41, Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 23/05/2023 19.30, Marcin Juszkiewicz wrote:
>>>> ...
>>>>>> (is there a reason why it can't be disabled with "-vga none" or
>>>>>> "-nodefaults"?)
>>>>>
>>>>> That's something I need to check how it should be done.
>>>>
>>>> Other boards set mc->default_display in their ...class_init
>>>> function and then use pci_vga_init() (or vga_interface_type)
>>>> to instantiate their default display adapter ... however, that
>>>> does not seem to support the bochs adapter yet (see
>>>> vga_interfaces[] in softmmu/vl.c).
>>>>
>>>> Not sure whether it's worth the effort to extend vga_interfaces[]
>>>> in vl.c
>>>
>>> Isn't that a legacy-command-line-option thing? I feel like
>>> the recommended way to select a different graphics card
>>> these days would be to use -device my-pci-vga-card ...
>>
>> "-vga" is kind of legacy, indeed, but currently the sbsa-ref hard-codes the
>> graphics card to be always available, so if you add a "-device
>> something-vga-card" on the command line, you'd get two graphic cards on your
>> machine, even if you use -nodefaults.
> 
> At least some boards do "only create the default graphics
> type if vga_interface_type != VGA_NONE".
> 
> Mostly what I would like here is consistency. But also, we
> haven't added a new item to the '-vga' option list since
> 2014, which makes me strongly suspect it's legacy and we
> should only be keeping it for backwards compatibility.
> 
>> So there needs to be at least some logic dealing with vga_interface_type if
>> we want to be able to select a different graphics card for this machine.
>> Then why not go the full way and use pci_vga_init() here, too? ... that's
>> certainly the least confusing way for the users.
> 
> Is it? From an Arm perspective, having "-vga" do anything
> at all is pretty confusing: it's a rather PC-centric option name.
> (Also pretty noticeable for the Sparc TCX/CG3 framebuffers,
> which are not VGA in any way.)

Right. From the SPARC perspective it was added to allow the user to select either the 
TCX (default) or CG3 framebuffers from the command line.

However I guess that shouldn't be needed anymore now that mc->default_display exists. 
Presumably there is now some kind of -global sun4m.default_display=cg3 command line 
option that could set the machine default_display property value instead?


ATB,

Mark.

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Peter Maydell]

On Thu, 25 May 2023 at 12:06, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 25/05/2023 11:44, Peter Maydell wrote:
>
> > On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote:
> >
> >> So there needs to be at least some logic dealing with vga_interface_type if
> >> we want to be able to select a different graphics card for this machine.
> >> Then why not go the full way and use pci_vga_init() here, too? ... that's
> >> certainly the least confusing way for the users.
> >
> > Is it? From an Arm perspective, having "-vga" do anything
> > at all is pretty confusing: it's a rather PC-centric option name.
> > (Also pretty noticeable for the Sparc TCX/CG3 framebuffers,
> > which are not VGA in any way.)
>
> Right. From the SPARC perspective it was added to allow the user to select either the
> TCX (default) or CG3 framebuffers from the command line.
>
> However I guess that shouldn't be needed anymore now that mc->default_display exists.
> Presumably there is now some kind of -global sun4m.default_display=cg3 command line
> option that could set the machine default_display property value instead?

Maybe. Handling builtin default devices remains kind of awkward.
But for this Arm board they're all just PCI cards, so the
only thing we really need is a way to say "don't create that
default device"...

-- PMM

Re: [PATCH 1/2] docs: sbsa: correct graphics card name

[Thomas Huth]

On 25/05/2023 13.39, Peter Maydell wrote:
> On Thu, 25 May 2023 at 12:06, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 25/05/2023 11:44, Peter Maydell wrote:
>>
>>> On Thu, 25 May 2023 at 11:32, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> So there needs to be at least some logic dealing with vga_interface_type if
>>>> we want to be able to select a different graphics card for this machine.
>>>> Then why not go the full way and use pci_vga_init() here, too? ... that's
>>>> certainly the least confusing way for the users.
>>>
>>> Is it? From an Arm perspective, having "-vga" do anything
>>> at all is pretty confusing: it's a rather PC-centric option name.
>>> (Also pretty noticeable for the Sparc TCX/CG3 framebuffers,
>>> which are not VGA in any way.)
>>
>> Right. From the SPARC perspective it was added to allow the user to select either the
>> TCX (default) or CG3 framebuffers from the command line.
>>
>> However I guess that shouldn't be needed anymore now that mc->default_display exists.
>> Presumably there is now some kind of -global sun4m.default_display=cg3 command line
>> option that could set the machine default_display property value instead?
> 
> Maybe. Handling builtin default devices remains kind of awkward.
> But for this Arm board they're all just PCI cards, so the
> only thing we really need is a way to say "don't create that
> default device"...

I wonder whether we could deprecate and finally remove "-vga" ... there is 
also the "graphics" machine property that is used by some boards instead, so 
maybe we could use that as a replacement for "-vga none" everywhere (and use 
"-device xxx" as replacement for "vga xxx" of course). Thoughts?

  Thomas