| Message ID | CAD8of+pgPfM3sUdvetGHxjU3vwsDiWGnFD8Qa2EqszmiMcgTig@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Andreas, This patch is to enable vga which works well on our internal tree. Would you please help review it? Any suggestion is appreciated. Thanks a lot. :) On Mon, Jun 18, 2012 at 5:34 PM, Li Zhang <zhlcindy@gmail.com> wrote: > 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 | 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" > +#include "usb.h" > > #include "exec-memory.h" > > @@ -82,6 +84,7 @@ > #define PHANDLE_XICP 0x00001111 > > sPAPREnvironment *spapr; > +static int spapr_has_graphics; > > qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num, > enum xics_irq_type type) > @@ -222,6 +225,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))); > > @@ -457,7 +463,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))); > > @@ -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; > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void ppc_spapr_init(ram_addr_t ram_size, > const char *boot_device, > @@ -663,6 +695,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 = 1; > + } > + > machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); > if (machine_opts) > usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > @@ -670,6 +707,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, > if (usb_on) { > 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 >= " > -- > 1.7.7.6
On 18.06.2012, at 11:34, Li Zhang wrote: > 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 | 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" > +#include "usb.h" > > #include "exec-memory.h" > > @@ -82,6 +84,7 @@ > #define PHANDLE_XICP 0x00001111 > > sPAPREnvironment *spapr; > +static int spapr_has_graphics; > > qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num, > enum xics_irq_type type) > @@ -222,6 +225,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))); > > @@ -457,7 +463,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))); > > @@ -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 */ Ben, mind to push a working SLOF, we we can just enable all of it in one go and don't have to commit #if 0'ed code? Rest looks reasonable to me. Alex
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 <benh@kernel.crashing.org> > Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> > --- > 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. > +#include "usb.h" > > #include "exec-memory.h" > > @@ -82,6 +84,7 @@ > #define PHANDLE_XICP 0x00001111 > > sPAPREnvironment *spapr; > +static int spapr_has_graphics; bool please. > @@ -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? > /* pSeries LPAR / sPAPR hardware init */ > static void ppc_spapr_init(ram_addr_t ram_size, > const char *boot_device, > @@ -663,6 +695,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 = 1; true for bool Functionally apart from the #if 0 it looks okay to me. Andreas > + } > + > machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); > if (machine_opts) > usb_on = qemu_opt_get_bool(machine_opts, "usb", true); > @@ -670,6 +707,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, > if (usb_on) { > 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 >= "
On Wed, Jun 27, 2012 at 9:47 PM, Andreas Färber <afaerber@suse.de> 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 <benh@kernel.crashing.org> >> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >> --- >> 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); >> +#include "usb.h" >> >> #include "exec-memory.h" >> >> @@ -82,6 +84,7 @@ >> #define PHANDLE_XICP 0x00001111 >> >> sPAPREnvironment *spapr; >> +static int spapr_has_graphics; > > bool please. > >> @@ -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. :) > >> /* pSeries LPAR / sPAPR hardware init */ >> static void ppc_spapr_init(ram_addr_t ram_size, >> const char *boot_device, >> @@ -663,6 +695,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 = 1; > > true for bool > OK. > Functionally apart from the #if 0 it looks okay to me. > > Andreas > >> + } >> + >> machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); >> if (machine_opts) >> usb_on = qemu_opt_get_bool(machine_opts, "usb", true); >> @@ -670,6 +707,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, >> if (usb_on) { >> 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 >= " > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 27.06.2012 15:55, schrieb Li Zhang: > On Wed, Jun 27, 2012 at 9:47 PM, Andreas Färber <afaerber@suse.de> 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 <benh@kernel.crashing.org> >>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>> --- >>> 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? >>> @@ -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... Andreas
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 <afaerber@suse.de> 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 <benh@kernel.crashing.org> >>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>>> --- >>>> 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
On Wed, 2012-06-27 at 14:08 +0200, Alexander Graf wrote: > Ben, mind to push a working SLOF, we we can just enable all of it in > one go and don't have to commit #if 0'ed code? > > Rest looks reasonable to me. Sure, SLOF was gated by the memop patch but we can push it now. However I'd rather leave that #if 0 for now for cirrus... There are several issues with cirrus at the moment. Off the top of my head: - The Xorg driver has bugs with physical addresses > 32-bit (even when compiled 64-bit). I had a patch floating around, I need to refresh it and get it merged. - There is another issue with the way it accesses legacy VGA space which should be routed by the kernel to a bit of shmem but isn't, we need to add support for a VGA hole to qemu (if we didn't already, I don't remember now, I'll check that later). - cirrusfb in the kernel will explode due to various bugs in it (basically assumes that on powerpc it's a prep machine and does horrible things with resources). Even with that fixed we still have a problem with color expansion - color expansion works by constantly replacing the memory region in qemu that covers the frame buffer, flipping it between a direct mapping of the fb vs. emulated registers. This triggers races & problems in our kvm implementation. We need to fix them but we haven't yet. So overall, let's keep cirrus off in that code path. It's still possible to manually enable it with -device for development/debugging purposes. Cheers, Ben.
On 27.06.2012, at 23:39, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-27 at 14:08 +0200, Alexander Graf wrote: >> Ben, mind to push a working SLOF, we we can just enable all of it in >> one go and don't have to commit #if 0'ed code? >> >> Rest looks reasonable to me. > > Sure, SLOF was gated by the memop patch but we can push it now. However > I'd rather leave that #if 0 for now for cirrus... > > There are several issues with cirrus at the moment. Off the top of my > head: > > - The Xorg driver has bugs with physical addresses > 32-bit (even when > compiled 64-bit). I had a patch floating around, I need to refresh it > and get it merged. > > - There is another issue with the way it accesses legacy VGA space > which should be routed by the kernel to a bit of shmem but isn't, we > need to add support for a VGA hole to qemu (if we didn't already, I > don't remember now, I'll check that later). > > - cirrusfb in the kernel will explode due to various bugs in it > (basically assumes that on powerpc it's a prep machine and does horrible > things with resources). Even with that fixed we still have a problem > with color expansion > > - color expansion works by constantly replacing the memory region in > qemu that covers the frame buffer, flipping it between a direct mapping > of the fb vs. emulated registers. This triggers races & problems in our > kvm implementation. We need to fix them but we haven't yet. > > So overall, let's keep cirrus off in that code path. It's still possible > to manually enable it with -device for development/debugging purposes. It shouldn't be an #if 0 but an if (cirrus) hw_abort() then though. Otherwise we'd just silently ignore the option. Alex
On Wed, 2012-06-27 at 16:32 +0200, Andreas Färber wrote: > >> 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... Right. However, to ease the pain with some things like libvirt, maybe we should make those choices "fallback" to std rather than not instanciate graphics at all ? Cheers, Ben.
On 27.06.2012, at 23:48, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-27 at 16:32 +0200, Andreas Färber wrote: >>>> 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... > > Right. However, to ease the pain with some things like libvirt, maybe we > should make those choices "fallback" to std rather than not instanciate > graphics at all ? No, I'd rather just make the default be -vga std for -M pseries. It already is for the Mac PPC machines. libvirt shouldn't pass in -vga cirrus to get the default on any particular machine. Alex
On Wed, 2012-06-27 at 23:42 +0200, Alexander Graf wrote: > > It shouldn't be an #if 0 but an if (cirrus) hw_abort() then though. > Otherwise we'd just silently ignore the option. You guys love aborts too much. For a high level option like -vga I'd rather just fallback to "std" (like afaik some other machines do in unsupported cases) and have something going, maybe with a warning. Cheers, Ben.
On 28.06.2012, at 00:05, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-27 at 23:42 +0200, Alexander Graf wrote: >> >> It shouldn't be an #if 0 but an if (cirrus) hw_abort() then though. >> Otherwise we'd just silently ignore the option. > > You guys love aborts too much. For a high level option like -vga I'd > rather just fallback to "std" (like afaik some other machines do in > unsupported cases) and have something going, maybe with a warning. If you pass in nothing, yes, default to std. If you explicitly say "I want a cirrus adapter", we either give the user a cirrus adapter or we tell him that we can't give him what he's asking for. Otherwise what's the point in having the option? And if libvirt gets it wrong (again), then we better fix libvirt rather than put a hack into QEMU. Alex
On 06/27/2012 05:14 PM, Alexander Graf wrote: > > On 28.06.2012, at 00:05, Benjamin Herrenschmidt wrote: > >> On Wed, 2012-06-27 at 23:42 +0200, Alexander Graf wrote: >>> >>> It shouldn't be an #if 0 but an if (cirrus) hw_abort() then though. >>> Otherwise we'd just silently ignore the option. >> >> You guys love aborts too much. For a high level option like -vga I'd >> rather just fallback to "std" (like afaik some other machines do in >> unsupported cases) and have something going, maybe with a warning. > > If you pass in nothing, yes, default to std. If you explicitly say "I want a cirrus adapter", we either give the user a cirrus adapter or we tell him that we can't give him what he's asking for. Otherwise what's the point in having the option? And if libvirt gets it wrong (again), then we better fix libvirt rather than put a hack into QEMU. Ack. If a user asks for something and we can't make it work, we should fail. Regards, Anthony Liguori > > Alex >
On Wed, Jun 27, 2012 at 10:37 PM, Alexander Graf <agraf@suse.de> 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 <afaerber@suse.de> 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 <benh@kernel.crashing.org> >>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> >>>>> --- >>>>> 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 >
On 06/28/2012 09:34 AM, Li Zhang wrote: > On Wed, Jun 27, 2012 at 10:37 PM, Alexander Graf<agraf@suse.de> 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<afaerber@suse.de> 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<benh@kernel.crashing.org> >>>>>> Signed-off-by: Li Zhang<zhlcindy@linux.vnet.ibm.com> >>>>>> --- >>>>>> 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. > > 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 :). > >>>>>> @@ -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. Alex
On Thu, Jun 28, 2012 at 7:03 PM, Alexander Graf <agraf@suse.de> wrote: > On 06/28/2012 09:34 AM, Li Zhang wrote: >> >> On Wed, Jun 27, 2012 at 10:37 PM, Alexander Graf<agraf@suse.de> 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<afaerber@suse.de> >>>>> 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<benh@kernel.crashing.org> >>>>>>> Signed-off-by: Li Zhang<zhlcindy@linux.vnet.ibm.com> >>>>>>> --- >>>>>>> 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 >
On Wed, 2012-06-27 at 17:25 -0500, Anthony Liguori wrote:
> If a user asks for something and we can't make it work, we should fail.
Note that QEMU's cirrus works fine with the new kernel cirrusdrmfb, so
I say we should allow it (needs to be able to the powerpc device .mak
as well tho.
It shouldn't be primary because old RHEL's iirc used to have cirrusfb
enabled and that would blow due to guest kernel bugs among others.
I haven't had a chance to try whatever userspace DDX the Xorg folks have
come up with to use on top of cirrusdrmfb, so that might need a bit of
fixing but there's no reason not to allow -vga cirrus at this point.
Note to Matthew: cirrusdrmfb is a LOT SLOWER than offb for a similar
SW only dumb framebuffer, probably has to do with the way it does the
"dirty" stuff, not sure ...
Why not draw directly into the emulated vram ?
Cheers,
Ben.
On Mon, 2012-07-23 at 16:40 +1000, Benjamin Herrenschmidt wrote: > > Note to Matthew: cirrusdrmfb is a LOT SLOWER than offb for a similar > SW only dumb framebuffer, probably has to do with the way it does the > "dirty" stuff, not sure ... > > Why not draw directly into the emulated vram ? More note to Matthew ... any objection to doing something similar for qemu "bochs" style framebuffer ? (ia -vga std). This is the preferred one on ppc, does 32-bpp fine and is generally simpler. Cheers, Ben.
Dave Airlie wrote the dirty handling code, so Cc:ing him. I suspect transitioning offb over to a similar design would be fine by him. On Mon, Jul 23, 2012 at 04:40:21PM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2012-06-27 at 17:25 -0500, Anthony Liguori wrote: > > > If a user asks for something and we can't make it work, we should fail. > > Note that QEMU's cirrus works fine with the new kernel cirrusdrmfb, so > I say we should allow it (needs to be able to the powerpc device .mak > as well tho. > > It shouldn't be primary because old RHEL's iirc used to have cirrusfb > enabled and that would blow due to guest kernel bugs among others. > > I haven't had a chance to try whatever userspace DDX the Xorg folks have > come up with to use on top of cirrusdrmfb, so that might need a bit of > fixing but there's no reason not to allow -vga cirrus at this point. > > Note to Matthew: cirrusdrmfb is a LOT SLOWER than offb for a similar > SW only dumb framebuffer, probably has to do with the way it does the > "dirty" stuff, not sure ... > > Why not draw directly into the emulated vram ? > > Cheers, > Ben. > > >
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" +#include "usb.h" #include "exec-memory.h" @@ -82,6 +84,7 @@ #define PHANDLE_XICP 0x00001111 sPAPREnvironment *spapr; +static int spapr_has_graphics; qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num, enum xics_irq_type type) @@ -222,6 +225,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))); @@ -457,7 +463,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))); @@ -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; +} + /* pSeries LPAR / sPAPR hardware init */ static void ppc_spapr_init(ram_addr_t ram_size, const char *boot_device, @@ -663,6 +695,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 = 1; + } + machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0); if (machine_opts) usb_on = qemu_opt_get_bool(machine_opts, "usb", true); @@ -670,6 +707,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, if (usb_on) { 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 >= "