Re: [Qemu-devel] [PATCH 2/2] spapr: Add support for -vga option

[Alexander Graf]

On 27.06.2012, at 16:32, Andreas Färber wrote:

> Am 27.06.2012 15:55, schrieb Li Zhang:
>> On Wed, Jun 27, 2012 at 9:47 PM, Andreas Färber <address@hidden> wrote:
>>> Am 18.06.2012 11:34, schrieb Li Zhang:
>>>> 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 |   43 ++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 files changed, 42 insertions(+), 1 deletions(-)
>>>> 
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index 8d158d7..c7b6e9d 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>>>> @@ -45,6 +45,8 @@
>>>> #include "kvm.h"
>>>> #include "kvm_ppc.h"
>>>> #include "pci.h"
>>>> +#include "pc.h"
>>> 
>>> This seems wrong for sPAPR.
>>> 
>> pci_vga_init()  is defined in pc.h which is called in the following.
>> 
>> +    } else if (std_vga_enabled) {
>> +       pci_vga_init(pci_bus);
> 
> Then we should move the declaration to a better place instead. :)
> 
> We seriously shouldn't expect pc.h to build on random targets.
> Not sure what the function does, maybe it can be avoided by QOM? Alex?

hw/vga-pci.c:DeviceState *pci_vga_init(PCIBus *bus)

So why not just extract the definition out into vga-pci.h and include that 
instead?


> 
>>>> @@ -510,6 +518,30 @@ static void spapr_cpu_reset(void *opaque)
>>>>     cpu_reset(CPU(cpu));
>>>> }
>>>> 
>>>> +static int spapr_vga_init(PCIBus *pci_bus)
>>>> +{
>>>> +    /* Default is nothing */
>>>> +#if 0 /* Enable this once we merge a SLOF which works with Cirrus */
>>>> +    if (cirrus_vga_enabled) {
>>>> +        pci_cirrus_vga_init(pci_bus);
>>>> +    } else
>>>> +#endif
>>>> +    if (vmsvga_enabled) {
>>>> +        fprintf(stderr, "Warning: vmware_vga not available,"
>>>> +                " using standard VGA instead\n");
>>>> +        pci_vga_init(pci_bus);
>>>> +#ifdef CONFIG_SPICE
>>>> +    } else if (qxl_enabled) {
>>>> +        pci_create_simple(pci_bus, -1, "qxl-vga");
>>>> +#endif
>>>> +    } else if (std_vga_enabled) {
>>>> +        pci_vga_init(pci_bus);
>>>> +    } else {
>>>> +        return 0;
>>>> +    }
>>>> +    return 1;
>>>> +}
>>>> +
>>> 
>>> Did you test whether all those paths actually work with ppc? SPICE
>>> didn't support ppc host last time I checked. Does it work on x86 host?
>> Currently, I test -vga std, it works well.
>> SPICE and curris are not supported on pcc. :)
> 
> Please elaborate on this: ppc host or guest? If they don't work with
> sPAPR ppc guests there's little point in including the code here...

There was a project going to get SPICE host support rolling with -M pseries. I 
don't think they were actually looking at QXL yet though.


Alex