From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaAOY-0006xf-0g for qemu-devel@nongnu.org; Mon, 23 Mar 2015 18:00:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaAOU-0004bA-No for qemu-devel@nongnu.org; Mon, 23 Mar 2015 18:00:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56598) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaAOU-0004aZ-H2 for qemu-devel@nongnu.org; Mon, 23 Mar 2015 18:00:22 -0400 Message-ID: <55108CF2.7010508@redhat.com> Date: Tue, 24 Mar 2015 00:00:18 +0200 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1427130328-3629-1-git-send-email-pbonzini@redhat.com> <55105971.3000708@redhat.com> <551059A0.9060602@suse.de> <5510736B.8000107@redhat.com> In-Reply-To: <5510736B.8000107@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.3] powerpc: fix -machine usb=no for newworld and pseries machines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Alexander Graf , qemu-devel@nongnu.org Cc: marcel.a@redhat.com 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 >>>> --- >>>> 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 >