| Message ID | 1420550957-22337-6-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/01/2015 14:29, Marcel Apfelbaum wrote: > @@ -1484,9 +1484,10 @@ static void ppc_spapr_init(MachineState *machine) > /* Graphics */ > if (spapr_vga_init(phb->bus)) { > spapr->has_graphics = true; > + machine->usb |= defaults_enabled(); > } Could the solution be to do this in instance_init? Then you would have patches 2, 4, 5, 6, 3 (patch 1 would not be needed anymore). Paolo
On 01/06/2015 10:45 PM, Paolo Bonzini wrote: > > > On 06/01/2015 14:29, Marcel Apfelbaum wrote: >> @@ -1484,9 +1484,10 @@ static void ppc_spapr_init(MachineState *machine) >> /* Graphics */ >> if (spapr_vga_init(phb->bus)) { >> spapr->has_graphics = true; >> + machine->usb |= defaults_enabled(); >> } > > Could the solution be to do this in instance_init? Then you would have > patches 2, 4, 5, 6, 3 (patch 1 would not be needed anymore). Hi Paolo, Thanks for the review. While I agree it will be better if we place this in instance_init, setting the machine_usb to defaults_enabled() there would be problematic since it depends on - papr_vga_init(phb->bus) for sparpr and - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99. (The env itself is set in machine_init) Both of those conditions may be available only at machine_init time, and I am not sure how it would affect those machines. This is why I prefer it this way, Thanks, Marcel > > Paolo >
On 07/01/2015 12:03, Marcel Apfelbaum wrote: > > While I agree it will be better if we place this in instance_init, > setting the machine_usb to defaults_enabled() there would be problematic > since it depends on > - papr_vga_init(phb->bus) for sparpr and That's effectively vga_interface_type == VGA_DEVICE || vga_interface_type == VGA_STD. > - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99. > (The env itself is set in machine_init) Alex, why is auto-USB disabled for 6xx? Can it use vga_interface_type like spapr does? Paolo > Both of those conditions may be available only at machine_init time, > and I am not sure how it would affect those machines.
On 07.01.15 12:07, Paolo Bonzini wrote: > > > On 07/01/2015 12:03, Marcel Apfelbaum wrote: >> >> While I agree it will be better if we place this in instance_init, >> setting the machine_usb to defaults_enabled() there would be problematic >> since it depends on >> - papr_vga_init(phb->bus) for sparpr and > > That's effectively vga_interface_type == VGA_DEVICE || > vga_interface_type == VGA_STD. > >> - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99. >> (The env itself is set in machine_init) > > Alex, why is auto-USB disabled for 6xx? Can it use vga_interface_type > like spapr does? That one's a nasty hack. We basically have 2 different machine types that we expose as a single type to the user: mac99. In reality there's a 64bit mac99 and a 32bit mac99. 32bit mac99 can expose keyboard and mouse via a special apple bus. That driver doesn't work with 64bit Linux guests though, so there we need USB. Thinking about it, maybe the best way forward would be to create 2 machine types out of these. Have a mac99 (32bit) and a mac99-g5 target where the g5 target defaults to -cpu G5 and USB enabled. All of this is pretty frankenstein btw. What we would really want for a G5 guest is something built around U3 or U4, not the U1 that -M mac99 exposes. Alex
On 07/01/2015 12:15, Alexander Graf wrote: > > > On 07.01.15 12:07, Paolo Bonzini wrote: >> >> >> On 07/01/2015 12:03, Marcel Apfelbaum wrote: >>> >>> While I agree it will be better if we place this in instance_init, >>> setting the machine_usb to defaults_enabled() there would be problematic >>> since it depends on >>> - papr_vga_init(phb->bus) for sparpr and >> >> That's effectively vga_interface_type == VGA_DEVICE || >> vga_interface_type == VGA_STD. >> >>> - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99. >>> (The env itself is set in machine_init) >> >> Alex, why is auto-USB disabled for 6xx? Can it use vga_interface_type >> like spapr does? > > That one's a nasty hack. We basically have 2 different machine types > that we expose as a single type to the user: mac99. In reality there's a > 64bit mac99 and a 32bit mac99. > > 32bit mac99 can expose keyboard and mouse via a special apple bus. That > driver doesn't work with 64bit Linux guests though, so there we need USB. > > Thinking about it, maybe the best way forward would be to create 2 > machine types out of these. Have a mac99 (32bit) and a mac99-g5 target > where the g5 target defaults to -cpu G5 and USB enabled. > > All of this is pretty frankenstein btw. What we would really want for a > G5 guest is something built around U3 or U4, not the U1 that -M mac99 > exposes. Hmm, then I guess let's apply these patches and fix things up later? Paolo
On 07.01.15 12:22, Paolo Bonzini wrote: > > > On 07/01/2015 12:15, Alexander Graf wrote: >> >> >> On 07.01.15 12:07, Paolo Bonzini wrote: >>> >>> >>> On 07/01/2015 12:03, Marcel Apfelbaum wrote: >>>> >>>> While I agree it will be better if we place this in instance_init, >>>> setting the machine_usb to defaults_enabled() there would be problematic >>>> since it depends on >>>> - papr_vga_init(phb->bus) for sparpr and >>> >>> That's effectively vga_interface_type == VGA_DEVICE || >>> vga_interface_type == VGA_STD. >>> >>>> - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99. >>>> (The env itself is set in machine_init) >>> >>> Alex, why is auto-USB disabled for 6xx? Can it use vga_interface_type >>> like spapr does? >> >> That one's a nasty hack. We basically have 2 different machine types >> that we expose as a single type to the user: mac99. In reality there's a >> 64bit mac99 and a 32bit mac99. >> >> 32bit mac99 can expose keyboard and mouse via a special apple bus. That >> driver doesn't work with 64bit Linux guests though, so there we need USB. >> >> Thinking about it, maybe the best way forward would be to create 2 >> machine types out of these. Have a mac99 (32bit) and a mac99-g5 target >> where the g5 target defaults to -cpu G5 and USB enabled. >> >> All of this is pretty frankenstein btw. What we would really want for a >> G5 guest is something built around U3 or U4, not the U1 that -M mac99 >> exposes. > > Hmm, then I guess let's apply these patches and fix things up later? Certainly works for me ;). Alex
On 07/01/2015 12:27, Alexander Graf wrote: > > Hmm, then I guess let's apply these patches and fix things up later? > > Certainly works for me ;). Send a pull request then. :) Paolo
On 07.01.15 12:32, Paolo Bonzini wrote: > > > On 07/01/2015 12:27, Alexander Graf wrote: >>> Hmm, then I guess let's apply these patches and fix things up later? >> >> Certainly works for me ;). > > Send a pull request then. :) Ok, applied the patch set to ppc-next. Now let's see how much of my autotest setup fell apart over the holidays ;). Alex
On 01/07/2015 01:15 PM, Alexander Graf wrote: > > > On 07.01.15 12:07, Paolo Bonzini wrote: >> >> >> On 07/01/2015 12:03, Marcel Apfelbaum wrote: >>> >>> While I agree it will be better if we place this in instance_init, >>> setting the machine_usb to defaults_enabled() there would be problematic >>> since it depends on >>> - papr_vga_init(phb->bus) for sparpr and >> >> That's effectively vga_interface_type == VGA_DEVICE || >> vga_interface_type == VGA_STD. That means moving select_vgahw (vl.c) that sets vga_interface_type much much earlier in main, before the current machine is created. And it depends itself on other stuff... >> >>> - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99. >>> (The env itself is set in machine_init) >> >> Alex, why is auto-USB disabled for 6xx? Can it use vga_interface_type >> like spapr does? > > That one's a nasty hack. We basically have 2 different machine types > that we expose as a single type to the user: mac99. In reality there's a > 64bit mac99 and a 32bit mac99. > > 32bit mac99 can expose keyboard and mouse via a special apple bus. That > driver doesn't work with 64bit Linux guests though, so there we need USB. > > Thinking about it, maybe the best way forward would be to create 2 > machine types out of these. Have a mac99 (32bit) and a mac99-g5 target > where the g5 target defaults to -cpu G5 and USB enabled. > > All of this is pretty frankenstein btw. What we would really want for a > G5 guest is something built around U3 or U4, not the U1 that -M mac99 > exposes. Given my (lack of) expertise on ppc, I shouldn't throw myself yet in the above adventure. Since I will not be able (soon) to get in the stuff Alex mentioned and the implications moving the setting of vga_interface_type earlier in main fall far beyond this series target (fix a bug/simpligu -usb on the way), I suggest putting the above on todo list. I plan to do the same (access machine's properties instead of quering QemuOpts) for all other machine properties because I am sure we have other hidden bugs there. Thanks, Marcel > > > Alex >
On 01/07/2015 01:37 PM, Alexander Graf wrote: > > > On 07.01.15 12:32, Paolo Bonzini wrote: >> >> >> On 07/01/2015 12:27, Alexander Graf wrote: >>>> Hmm, then I guess let's apply these patches and fix things up later? >>> >>> Certainly works for me ;). >> >> Send a pull request then. :) > > Ok, applied the patch set to ppc-next. Now let's see how much of my > autotest setup fell apart over the holidays ;). > > > Alex > Thanks a lot for taking the time to discuss this! Marcel
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 72c3102..53c4116 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1484,9 +1484,10 @@ static void ppc_spapr_init(MachineState *machine) /* Graphics */ if (spapr_vga_init(phb->bus)) { spapr->has_graphics = true; + machine->usb |= defaults_enabled(); } - if ((spapr->has_graphics && defaults_enabled()) || usb_enabled()) { + if (machine->usb) { pci_create_simple(phb->bus, -1, "pci-ohci"); if (spapr->has_graphics) { usbdevice_create("keyboard");
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- hw/ppc/spapr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)