Re: [Qemu-ppc] [Qemu-devel][PATCH v5 4/4] spapr: Add support for -vga option

[Li Zhang]

On Fri, Jul 6, 2012 at 9:50 PM, Alexander Graf <address@hidden> wrote:
>
> On 02.07.2012, at 07:25, address@hidden wrote:
>
>> From: Li Zhang <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Li Zhang <address@hidden>
>> ---
>> 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
>



-- 

Best Regards
-Li