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

[Li Zhang]

On Wed, Jun 27, 2012 at 10:37 PM, Alexander Graf <address@hidden> wrote:
>
> 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?
>
Hi Alex,
There is no file called vga-pci.h. Is it ok to create one new file?

I saw that hw/vga-pci.c still includes pc.h.
And all the files which called pci_vga_init()  include pc.h, such as
ppc_newworld.c.

>
>>
>>>>> @@ -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.

Sorry, I am not looking at QXL. :)
Know little about SPICE.

>
>
> Alex
>



-- 

Best Regards
-Li