Patchwork [Qemu-ppc,v5,4/4] spapr: Add support for -vga option

login
register
mail settings
Submitter zhlcindy@gmail.com
Date July 2, 2012, 5:25 a.m.
Message ID <0be04c2530ab90a35e26d34cd2fdea49fa392716.1341204647.git.zhlcindy@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/168491/
State New
Headers show

Comments

zhlcindy@gmail.com - July 2, 2012, 5:25 a.m.
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

Also instanciate the USB keyboard and mouse when that option is used
(you can still use -device to create individual devices without all
the defaults)

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 hw/spapr.c |   29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)
Alexander Graf - July 6, 2012, 1:50 p.m.
On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:

> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> 
> Also instanciate the USB keyboard and mouse when that option is used
> (you can still use -device to create individual devices without all
> the defaults)
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> ---
> hw/spapr.c |   29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 973de1b..3648cad 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -45,6 +45,8 @@
> #include "kvm.h"
> #include "kvm_ppc.h"
> #include "pci.h"
> +#include "vga-pci.h"
> +#include "usb.h"
> 
> #include "exec-memory.h"
> 
> @@ -82,6 +84,7 @@
> #define PHANDLE_XICP            0x00001111
> 
> sPAPREnvironment *spapr;
> +bool spapr_has_graphics;

Globals are a big no-no :). Please move this into sPAPREnvironment.

> 
> qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
>                             enum xics_irq_type type)
> @@ -257,6 +260,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>         _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
>     }
>     _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));

Are you sure we want to set these in -nographic or -vga none mode as well?

> 
>     _FDT((fdt_end_node(fdt)));
> 
> @@ -503,7 +509,9 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>         }
>     }
> 
> -    spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
> +    if (!spapr_has_graphics) {
> +        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
> +    }
> 
>     _FDT((fdt_pack(fdt)));
> 
> @@ -556,6 +564,16 @@ static void spapr_cpu_reset(void *opaque)
>     cpu_reset(CPU(cpu));
> }
> 
> +static int spapr_vga_init(PCIBus *pci_bus)
> +{
> +    if (std_vga_enabled) {
> +        pci_vga_init(pci_bus);
> +    } else {
> +        return 0;
> +    }
> +    return 1;

This still silently ignores unsupported -vga options, right? Please error out properly if the user passes in -vga cirrus or xql. We need to let him know that what he selected is not available.

> +}
> +
> /* pSeries LPAR / sPAPR hardware init */
> static void ppc_spapr_init(ram_addr_t ram_size,
>                            const char *boot_device,
> @@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>         spapr_vscsi_create(spapr->vio_bus);
>     }
> 
> +    /* Graphics */
> +    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
> +        spapr_has_graphics = true;
> +    }

How about

spapr_has_graphics = spapr_vga_init(...);

If that gets you above 80 characters, just shove the parameter to spapr_vga_init into a variable.


Alex
Andreas Färber - July 6, 2012, 1:58 p.m.
Am 06.07.2012 15:50, schrieb Alexander Graf:
> 
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
> 
>> @@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr_vscsi_create(spapr->vio_bus);
>>     }
>>
>> +    /* Graphics */
>> +    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
>> +        spapr_has_graphics = true;
>> +    }
> 
> How about
> 
> spapr_has_graphics = spapr_vga_init(...);
> 
> If that gets you above 80 characters, just shove the parameter to spapr_vga_init into a variable.

Further, that expression could use
PCIHostState *phb = PCI_HOST_BRIDGE(QLIST_FIRST(&spapr->phbs));
spapr_vga_init(phb->bus)
once introduced through the pci_host series. :)

Andreas
zhlcindy@gmail.com - July 8, 2012, 3:08 p.m.
On Fri, Jul 6, 2012 at 9:50 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>
>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> Also instanciate the USB keyboard and mouse when that option is used
>> (you can still use -device to create individual devices without all
>> the defaults)
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> ---
>> hw/spapr.c |   29 ++++++++++++++++++++++++++++-
>> 1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index 973de1b..3648cad 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -45,6 +45,8 @@
>> #include "kvm.h"
>> #include "kvm_ppc.h"
>> #include "pci.h"
>> +#include "vga-pci.h"
>> +#include "usb.h"
>>
>> #include "exec-memory.h"
>>
>> @@ -82,6 +84,7 @@
>> #define PHANDLE_XICP            0x00001111
>>
>> sPAPREnvironment *spapr;
>> +bool spapr_has_graphics;
>
> Globals are a big no-no :). Please move this into sPAPREnvironment.
>
>>
>> qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
>>                             enum xics_irq_type type)
>> @@ -257,6 +260,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>         _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
>>     }
>>     _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
>> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
>> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
>
> Are you sure we want to set these in -nographic or -vga none mode as well?

I am not sure about this.

Ben, would you please give more information about this?

Thanks.

>
>>
>>     _FDT((fdt_end_node(fdt)));
>>
>> @@ -503,7 +509,9 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>         }
>>     }
>>
>> -    spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
>> +    if (!spapr_has_graphics) {
>> +        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
>> +    }
>>
>>     _FDT((fdt_pack(fdt)));
>>
>> @@ -556,6 +564,16 @@ static void spapr_cpu_reset(void *opaque)
>>     cpu_reset(CPU(cpu));
>> }
>>
>> +static int spapr_vga_init(PCIBus *pci_bus)
>> +{
>> +    if (std_vga_enabled) {
>> +        pci_vga_init(pci_bus);
>> +    } else {
>> +        return 0;
>> +    }
>> +    return 1;
>
> This still silently ignores unsupported -vga options, right? Please error out properly if the user passes in -vga cirrus or xql. We need to let him know that what he selected is not available.
Yes, right. It seems that it's not good.
OK, I will do that.
>
>> +}
>> +
>> /* pSeries LPAR / sPAPR hardware init */
>> static void ppc_spapr_init(ram_addr_t ram_size,
>>                            const char *boot_device,
>> @@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr_vscsi_create(spapr->vio_bus);
>>     }
>>
>> +    /* Graphics */
>> +    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
>> +        spapr_has_graphics = true;
>> +    }
>
> How about
>
> spapr_has_graphics = spapr_vga_init(...);
>
> If that gets you above 80 characters, just shove the parameter to spapr_vga_init into a variable.
OK. I see.
Thanks.
>
>
> Alex
>
Benjamin Herrenschmidt - July 8, 2012, 9:54 p.m.
On Sun, 2012-07-08 at 23:08 +0800, Li Zhang wrote:
> > Are you sure we want to set these in -nographic or -vga none mode as
> well?
> 
> I am not sure about this.
> 
> Ben, would you please give more information about this?

Doesn't matter much. They are only useful when there is a vga adapter,
they don't hurt if there isn't and it shouldn't depend on the presence
of -vga since once can add an adapter with -device, so it becomes really
tricky to figure out whether to expose them or not, so I made it
unconditional.

Cheers,
Ben.

Patch

diff --git a/hw/spapr.c b/hw/spapr.c
index 973de1b..3648cad 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -45,6 +45,8 @@ 
 #include "kvm.h"
 #include "kvm_ppc.h"
 #include "pci.h"
+#include "vga-pci.h"
+#include "usb.h"
 
 #include "exec-memory.h"
 
@@ -82,6 +84,7 @@ 
 #define PHANDLE_XICP            0x00001111
 
 sPAPREnvironment *spapr;
+bool spapr_has_graphics;
 
 qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
                             enum xics_irq_type type)
@@ -257,6 +260,9 @@  static void *spapr_create_fdt_skel(const char *cpu_model,
         _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
     }
     _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
+    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
+    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
+    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
 
     _FDT((fdt_end_node(fdt)));
 
@@ -503,7 +509,9 @@  static void spapr_finalize_fdt(sPAPREnvironment *spapr,
         }
     }
 
-    spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
+    if (!spapr_has_graphics) {
+        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
+    }
 
     _FDT((fdt_pack(fdt)));
 
@@ -556,6 +564,16 @@  static void spapr_cpu_reset(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
+static int spapr_vga_init(PCIBus *pci_bus)
+{
+    if (std_vga_enabled) {
+        pci_vga_init(pci_bus);
+    } else {
+        return 0;
+    }
+    return 1;
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(ram_addr_t ram_size,
                            const char *boot_device,
@@ -712,6 +730,11 @@  static void ppc_spapr_init(ram_addr_t ram_size,
         spapr_vscsi_create(spapr->vio_bus);
     }
 
+    /* Graphics */
+    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
+        spapr_has_graphics = true;
+    }
+
     machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
     if (machine_opts) {
         add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
@@ -720,6 +743,10 @@  static void ppc_spapr_init(ram_addr_t ram_size,
     if (add_usb) {
         pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
                           -1, "pci-ohci");
+        if (spapr_has_graphics) {
+            usbdevice_create("keyboard");
+            usbdevice_create("mouse");
+        }
     }
     if (rma_size < (MIN_RMA_SLOF << 20)) {
         fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "