| Message ID | 1427130328-3629-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/23/2015 06:05 PM, Paolo Bonzini wrote: > Capture the explicit setting of "usb=no" into a separate bool, and > use it to skip the update of machine->usb in the board init function. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, applied to ppc-next (for 2.3). Alex
On 03/23/2015 07:05 PM, Paolo Bonzini wrote: > Capture the explicit setting of "usb=no" into a separate bool, and > use it to skip the update of machine->usb in the board init function. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/core/machine.c | 1 + > hw/ppc/mac_newworld.c | 2 +- > hw/ppc/spapr.c | 2 +- > include/hw/boards.h | 1 + > 4 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index cb1185a..25c45e6 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -223,6 +223,7 @@ static void machine_set_usb(Object *obj, bool value, Error **errp) > MachineState *ms = MACHINE(obj); > > ms->usb = value; > + ms->usb_disabled = !value; Maybe is too late now, but I really not like this pollution of MachineState with 'usb_disabled'. (Imagine we have this kind of fields for lots of objects and lots of corner cases...) I know it comes to solve a bug, but we talked about it in another mail thread and this change in semantics was approved. Let me explain *why* I don't like it. 1. We add an "usb_disabled" field to a base class (actually object) of all the machines and the only place it is interesting is for 2 machines on ppc. 2. Even for these 2 machines, the scenario of defaults=on and usb=off is not practical. I hope I helped, Thanks, Marcel P.S. If you still want it there, maybe move it to a ppc base class? > } > > static char *machine_get_firmware(Object *obj, Error **errp) > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c > index e0397bc..a365bf9 100644 > --- a/hw/ppc/mac_newworld.c > +++ b/hw/ppc/mac_newworld.c > @@ -371,7 +371,7 @@ static void ppc_core99_init(MachineState *machine) > /* 970 gets a U3 bus */ > pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io()); > machine_arch = ARCH_MAC99_U3; > - machine->usb |= defaults_enabled(); > + machine->usb |= defaults_enabled() && !machine->usb_disabled; > } else { > pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io()); > machine_arch = ARCH_MAC99; > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0487f52..dd5e101 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1563,7 +1563,7 @@ static void ppc_spapr_init(MachineState *machine) > /* Graphics */ > if (spapr_vga_init(phb->bus)) { > spapr->has_graphics = true; > - machine->usb |= defaults_enabled(); > + machine->usb |= defaults_enabled() && !machine->usb_disabled; > } > > if (machine->usb) { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 1feea2b..be6a0ed 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -141,6 +141,7 @@ struct MachineState { > bool dump_guest_core; > bool mem_merge; > bool usb; > + bool usb_disabled; > char *firmware; > bool iommu; > bool suppress_vmdesc; >
On 03/23/2015 07:20 PM, Marcel Apfelbaum wrote: > On 03/23/2015 07:05 PM, Paolo Bonzini wrote: >> Capture the explicit setting of "usb=no" into a separate bool, and >> use it to skip the update of machine->usb in the board init function. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/core/machine.c | 1 + >> hw/ppc/mac_newworld.c | 2 +- >> hw/ppc/spapr.c | 2 +- >> include/hw/boards.h | 1 + >> 4 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index cb1185a..25c45e6 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -223,6 +223,7 @@ static void machine_set_usb(Object *obj, bool >> value, Error **errp) >> MachineState *ms = MACHINE(obj); >> >> ms->usb = value; >> + ms->usb_disabled = !value; > Maybe is too late now, but I really not like this pollution of > MachineState > with 'usb_disabled'. (Imagine we have this kind of fields for lots of > objects and lots > of corner cases...) > I know it comes to solve a bug, but we talked about it in another mail > thread and > this change in semantics was approved. > > Let me explain *why* I don't like it. > 1. We add an "usb_disabled" field to a base class (actually object) > of all the machines and the only place it is interesting is > for 2 machines on ppc. > 2. Even for these 2 machines, the scenario of defaults=on and usb=off > is not practical. I'm personally fine either way, but I assumed that Paolo had a good reason for writing the patch? Alex
On 23/03/2015 19:21, Alexander Graf wrote: > On 03/23/2015 07:20 PM, Marcel Apfelbaum wrote: >> On 03/23/2015 07:05 PM, Paolo Bonzini wrote: >>> Capture the explicit setting of "usb=no" into a separate bool, and >>> use it to skip the update of machine->usb in the board init function. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> hw/core/machine.c | 1 + >>> hw/ppc/mac_newworld.c | 2 +- >>> hw/ppc/spapr.c | 2 +- >>> include/hw/boards.h | 1 + >>> 4 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>> index cb1185a..25c45e6 100644 >>> --- a/hw/core/machine.c >>> +++ b/hw/core/machine.c >>> @@ -223,6 +223,7 @@ static void machine_set_usb(Object *obj, bool >>> value, Error **errp) >>> MachineState *ms = MACHINE(obj); >>> >>> ms->usb = value; >>> + ms->usb_disabled = !value; >> Maybe is too late now, but I really not like this pollution of >> MachineState >> with 'usb_disabled'. (Imagine we have this kind of fields for lots of >> objects and lots >> of corner cases...) >> I know it comes to solve a bug, but we talked about it in another mail >> thread and >> this change in semantics was approved. >> >> Let me explain *why* I don't like it. >> 1. We add an "usb_disabled" field to a base class (actually object) >> of all the machines and the only place it is interesting is >> for 2 machines on ppc. So we do for kernel_irqchip_requested/allowed. Both approaches could be replaced by a tri-state on/off/auto. >> 2. Even for these 2 machines, the scenario of defaults=on and usb=off >> is not practical. Why? For example you could add a virtio-input device instead of a USB keyboard and mouse. > I'm personally fine either way, but I assumed that Paolo had a good > reason for writing the patch? Actually, two. One good reason is that no matter how you look at it, it's at least surprising and at worst a bug that "-machine usb=no" includes a default USB controller. The second good reason is that it's a guest ABI change for the versioned pSeries machines, and as such it breaks migration. Paolo
On 03/23/2015 10:11 PM, Paolo Bonzini wrote: > > > On 23/03/2015 19:21, Alexander Graf wrote: >> On 03/23/2015 07:20 PM, Marcel Apfelbaum wrote: >>> On 03/23/2015 07:05 PM, Paolo Bonzini wrote: >>>> Capture the explicit setting of "usb=no" into a separate bool, and >>>> use it to skip the update of machine->usb in the board init function. >>>> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>>> --- >>>> hw/core/machine.c | 1 + >>>> hw/ppc/mac_newworld.c | 2 +- >>>> hw/ppc/spapr.c | 2 +- >>>> include/hw/boards.h | 1 + >>>> 4 files changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/core/machine.c b/hw/core/machine.c >>>> index cb1185a..25c45e6 100644 >>>> --- a/hw/core/machine.c >>>> +++ b/hw/core/machine.c >>>> @@ -223,6 +223,7 @@ static void machine_set_usb(Object *obj, bool >>>> value, Error **errp) >>>> MachineState *ms = MACHINE(obj); >>>> >>>> ms->usb = value; >>>> + ms->usb_disabled = !value; >>> Maybe is too late now, but I really not like this pollution of >>> MachineState >>> with 'usb_disabled'. (Imagine we have this kind of fields for lots of >>> objects and lots >>> of corner cases...) >>> I know it comes to solve a bug, but we talked about it in another mail >>> thread and >>> this change in semantics was approved. >>> >>> Let me explain *why* I don't like it. >>> 1. We add an "usb_disabled" field to a base class (actually object) >>> of all the machines and the only place it is interesting is >>> for 2 machines on ppc. > > So we do for kernel_irqchip_requested/allowed. Both approaches could be > replaced by a tri-state on/off/auto. Personally I prefer this one, but out of the scope of this patch. > >>> 2. Even for these 2 machines, the scenario of defaults=on and usb=off >>> is not practical. > > Why? For example you could add a virtio-input device instead of a USB > keyboard and mouse. You got me there :) From what I understood for those boards there is no need for this combination but I don't know them enough (OK.. at all). > >> I'm personally fine either way, but I assumed that Paolo had a good >> reason for writing the patch? > > Actually, two. > > One good reason is that no matter how you look at it, it's at least > surprising and at worst a bug that "-machine usb=no" includes a default > USB controller. > > The second good reason is that it's a guest ABI change for the versioned > pSeries machines, and as such it breaks migration. Always migration wins. Bottom line, of course I don't have anything against fixing this bug, my problem was only with the way we add those fields (usb_disabled), maybe a three state QOM property (and variable behind it) is a solution, but not for now of course. I also didn't like the required/allowed fields and I added them anyway... Thanks, Marcel > > Paolo >
On 23/03/2015 23:00, Marcel Apfelbaum wrote: >>>> I know it comes to solve a bug, but we talked about it in another mail >>>> thread and this change in semantics was approved. I forgot to reply to this---my understanding is that it was okay for the sake of your patch series, but it would be fixed before 2.3. >>>> Let me explain *why* I don't like it. >>>> 1. We add an "usb_disabled" field to a base class (actually object) >>>> of all the machines and the only place it is interesting is >>>> for 2 machines on ppc. >> >> So we do for kernel_irqchip_requested/allowed. Both approaches could be >> replaced by a tri-state on/off/auto. > Personally I prefer this one, but out of the scope of this patch. Yes, that was my rationale as well. >>>> 2. Even for these 2 machines, the scenario of defaults=on and usb=off >>>> is not practical. >> >> Why? For example you could add a virtio-input device instead of a USB >> keyboard and mouse. > You got me there :) > From what I understood for those boards there is no need for this > combination but I don't know them enough (OK.. at all). Well, you can always find a reason. USB is a good default, but it doesn't have to be the only one. You might even be okay with USB, but prefer a different host controller. > Bottom line, of course I don't have anything against fixing this bug, > my problem was only with the way we add those fields (usb_disabled), > maybe a three state QOM property (and variable behind it) is a > solution, but not for now of course. I think the QOM property should not be tristate, only the variable. Another possibility is backing "xyz" with a bool xyz, but adding a bool xyz_set. Then irqchip_required = irqchip_set && irqchip, and irqchip_allowed = !irqchip_set || irqchip. Paolo > I also didn't like the required/allowed fields and I added them anyway... > > Thanks, > Marcel > > >> >> Paolo >> > > >
diff --git a/hw/core/machine.c b/hw/core/machine.c index cb1185a..25c45e6 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -223,6 +223,7 @@ static void machine_set_usb(Object *obj, bool value, Error **errp) MachineState *ms = MACHINE(obj); ms->usb = value; + ms->usb_disabled = !value; } static char *machine_get_firmware(Object *obj, Error **errp) diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c index e0397bc..a365bf9 100644 --- a/hw/ppc/mac_newworld.c +++ b/hw/ppc/mac_newworld.c @@ -371,7 +371,7 @@ static void ppc_core99_init(MachineState *machine) /* 970 gets a U3 bus */ pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io()); machine_arch = ARCH_MAC99_U3; - machine->usb |= defaults_enabled(); + machine->usb |= defaults_enabled() && !machine->usb_disabled; } else { pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io()); machine_arch = ARCH_MAC99; diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0487f52..dd5e101 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1563,7 +1563,7 @@ static void ppc_spapr_init(MachineState *machine) /* Graphics */ if (spapr_vga_init(phb->bus)) { spapr->has_graphics = true; - machine->usb |= defaults_enabled(); + machine->usb |= defaults_enabled() && !machine->usb_disabled; } if (machine->usb) { diff --git a/include/hw/boards.h b/include/hw/boards.h index 1feea2b..be6a0ed 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -141,6 +141,7 @@ struct MachineState { bool dump_guest_core; bool mem_merge; bool usb; + bool usb_disabled; char *firmware; bool iommu; bool suppress_vmdesc;
Capture the explicit setting of "usb=no" into a separate bool, and use it to skip the update of machine->usb in the board init function. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/core/machine.c | 1 + hw/ppc/mac_newworld.c | 2 +- hw/ppc/spapr.c | 2 +- include/hw/boards.h | 1 + 4 files changed, 4 insertions(+), 2 deletions(-)