* [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
@ 2015-03-23 17:05 Paolo Bonzini
2015-03-23 17:28 ` Alexander Graf
2015-03-23 18:20 ` Marcel Apfelbaum
0 siblings, 2 replies; 7+ messages in thread
From: Paolo Bonzini @ 2015-03-23 17:05 UTC (permalink / raw)
To: qemu-devel; +Cc: agraf, marcel.a
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;
}
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;
--
2.3.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
2015-03-23 17:05 [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines Paolo Bonzini
@ 2015-03-23 17:28 ` Alexander Graf
2015-03-23 18:20 ` Marcel Apfelbaum
1 sibling, 0 replies; 7+ messages in thread
From: Alexander Graf @ 2015-03-23 17:28 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: marcel.a
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
2015-03-23 17:05 [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines Paolo Bonzini
2015-03-23 17:28 ` Alexander Graf
@ 2015-03-23 18:20 ` Marcel Apfelbaum
2015-03-23 18:21 ` Alexander Graf
1 sibling, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-03-23 18:20 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: agraf, marcel.a
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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
2015-03-23 18:20 ` Marcel Apfelbaum
@ 2015-03-23 18:21 ` Alexander Graf
2015-03-23 20:11 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Alexander Graf @ 2015-03-23 18:21 UTC (permalink / raw)
To: Marcel Apfelbaum, Paolo Bonzini, qemu-devel; +Cc: marcel.a
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
2015-03-23 18:21 ` Alexander Graf
@ 2015-03-23 20:11 ` Paolo Bonzini
2015-03-23 22:00 ` Marcel Apfelbaum
0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2015-03-23 20:11 UTC (permalink / raw)
To: Alexander Graf, Marcel Apfelbaum, qemu-devel; +Cc: marcel.a
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
2015-03-23 20:11 ` Paolo Bonzini
@ 2015-03-23 22:00 ` Marcel Apfelbaum
2015-03-24 7:47 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Marcel Apfelbaum @ 2015-03-23 22:00 UTC (permalink / raw)
To: Paolo Bonzini, Alexander Graf, qemu-devel; +Cc: marcel.a
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines
2015-03-23 22:00 ` Marcel Apfelbaum
@ 2015-03-24 7:47 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2015-03-24 7:47 UTC (permalink / raw)
To: Marcel Apfelbaum, Alexander Graf, qemu-devel; +Cc: marcel.a
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
>>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-24 7:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-23 17:05 [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines Paolo Bonzini
2015-03-23 17:28 ` Alexander Graf
2015-03-23 18:20 ` Marcel Apfelbaum
2015-03-23 18:21 ` Alexander Graf
2015-03-23 20:11 ` Paolo Bonzini
2015-03-23 22:00 ` Marcel Apfelbaum
2015-03-24 7:47 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.