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

[Li Zhang]

On Thu, Jun 28, 2012 at 7:03 PM, Alexander Graf <address@hidden> wrote:
> On 06/28/2012 09:34 AM, Li Zhang wrote:
>>
>> 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?
>
>
> Yes, please :). Just create a new patch that splits the definition out into
> a new header file, includes the new header file from pc.h and then include
> only that new header file in spapr.c.
>
OK, I will do that.
>
>>
>> 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.
>
>
> If you want to get bonus points, check if we can maybe replace other pc.h
> users with this new include as well :).
>
I will check if we can replace other users.
>
>>
>>>>>>> @@ -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.
>
>
> SPICE consists roughly of 2 parts:
>
>  1) guest interface
>  2) protocol
>
> The guest interface is essentially the QXL graphics adapter. The protocol is
> what is called SPICE. You can use the QXL device without using the SPICE
> protocol. I'm not sure if it works the other way around, but I'd assume so
> too.
>
I see, thanks for your explaination. :)
>
> Alex
>



-- 

Best Regards
-Li