From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UImRX-0004R9-3w for qemu-devel@nongnu.org; Thu, 21 Mar 2013 16:50:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UImRU-0002Lv-20 for qemu-devel@nongnu.org; Thu, 21 Mar 2013 16:50:35 -0400 Date: Thu, 21 Mar 2013 15:50:06 -0500 From: Scott Wood In-Reply-To: (from agraf@suse.de on Thu Mar 21 03:41:19 2013) Message-ID: <1363899006.31522.24@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC ppc-next PATCH 6/6] kvm/openpic: in-kernel mpic support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org On 03/21/2013 03:41:19 AM, Alexander Graf wrote: >=20 > On 14.02.2013, at 07:32, Scott Wood wrote: >=20 > > +DeviceState *kvm_openpic_create(BusState *bus, int model) > > +{ > > + KVMState *s =3D kvm_state; > > + DeviceState *dev; > > + struct kvm_create_device cd =3D {0}; > > + int ret; > > + > > + if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) { > > + return NULL; > > + } > > + > > + switch (model) { > > + case OPENPIC_MODEL_FSL_MPIC_20: > > + cd.type =3D KVM_DEV_TYPE_FSL_MPIC_20; > > + break; > > + > > + case OPENPIC_MODEL_FSL_MPIC_42: > > + cd.type =3D KVM_DEV_TYPE_FSL_MPIC_42; > > + break; > > + > > + default: > > + qemu_log_mask(LOG_UNIMP, "%s: unknown openpic model %d\n", > > + __func__, model); > > + return NULL; > > + } > > + > > + ret =3D kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd); > > + if (ret < 0) { > > + fprintf(stderr, "%s: can't create device %d: %s\n", =20 > __func__, cd.type, > > + strerror(errno)); > > + return NULL; > > + } >=20 > Can't all the stuff above here just simply go into the qdev init =20 > function? Not if you want platform code to be able to fall back to a QEMU mpic if =20 an in-kernel mpic is unavailable. > > /* MPIC */ > > mpic =3D g_new(qemu_irq, 256); > > - dev =3D qdev_create(NULL, "openpic"); > > - qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus); > > - qdev_prop_set_uint32(dev, "model", params->mpic_version); > > + > > + if (kvm_irqchip_wanted()) { > > + dev =3D kvm_openpic_create(NULL, params->mpic_version); >=20 > This really should be just a >=20 > dev =3D qdev_create(NULL, kvm_irqchip_wanted() ? "kvm-openpic" : =20 > "openpic"); >=20 > The logic whether an in-kernel irqchip is available belongs into the =20 > default setting of kvm_irqchip_wanted. That is exactly what I was trying to avoid by introducing =20 kvm_irqchip_wanted. We're no longer testing some vague generic irqchip =20 capability, but the presence of a specific type of device (and version =20 thereof). How would the code that sets kvm_irqchip_wanted know what to =20 test for? > If the host kvm version can't handle an in-kernel MPIC, it should =20 > simply default to false. If it supports one, it defaults to true. OK, I misread the existing code and thought that the in-kernel irqchip =20 would never be used unless explicitly requested. > Whenever the user defines something explicitly with -machine, that =20 > wins. Then we'd need kvm_irqchip_wanted to be a tristate -- on, off, or =20 unspecified. At that point it might be better to drop it entirely and =20 just open-code the option check. -Scott=