From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtaeJ-0004QI-AC for qemu-devel@nongnu.org; Wed, 26 Nov 2014 06:20:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtaeD-0004Yw-0G for qemu-devel@nongnu.org; Wed, 26 Nov 2014 06:20:42 -0500 Received: from cantor2.suse.de ([195.135.220.15]:60926 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtaeC-0004X8-ED for qemu-devel@nongnu.org; Wed, 26 Nov 2014 06:20:36 -0500 Message-ID: <5475B781.5030009@suse.de> Date: Wed, 26 Nov 2014 12:20:33 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1414764350-5140-1-git-send-email-eric.auger@linaro.org> <1414764350-5140-10-git-send-email-eric.auger@linaro.org> <5459FC19.1050200@suse.de> <5475A150.8030305@linaro.org> <5475AA53.1030509@suse.de> <5475AFED.6090800@linaro.org> In-Reply-To: <5475AFED.6090800@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v7 09/16] hw/vfio/platform: add vfio-platform support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Auger , eric.auger@st.com, christoffer.dall@linaro.org, qemu-devel@nongnu.org, pbonzini@redhat.com, kim.phillips@freescale.com, a.rigo@virtualopensystems.com, manish.jaggi@caviumnetworks.com, joel.schopp@amd.com Cc: peter.maydell@linaro.org, patches@linaro.org, Kim Phillips , will.deacon@arm.com, stuart.yoder@freescale.com, Bharat.Bhushan@freescale.com, alex.williamson@redhat.com, a.motakis@virtualopensystems.com, kvmarm@lists.cs.columbia.edu On 26.11.14 11:48, Eric Auger wrote: > On 11/26/2014 11:24 AM, Alexander Graf wrote: >> >> >> On 26.11.14 10:45, Eric Auger wrote: >>> On 11/05/2014 11:29 AM, Alexander Graf wrote: >>>> >>>> >>>> On 31.10.14 15:05, Eric Auger wrote: >>>>> Minimal VFIO platform implementation supporting >>>>> - register space user mapping, >>>>> - IRQ assignment based on eventfds handled on qemu side. >>>>> >>>>> irqfd kernel acceleration comes in a subsequent patch. >>>>> >>>>> Signed-off-by: Kim Phillips >>>>> Signed-off-by: Eric Auger >> >> [...] >> >>>>> +/* >>>>> + * Mechanics to program/start irq injection on machine init done n= otifier: >>>>> + * this is needed since at finalize time, the device IRQ are not y= et >>>>> + * bound to the platform bus IRQ. It is assumed here dynamic insta= ntiation >>>>> + * always is used. Binding to the platform bus IRQ happens on a ma= chine >>>>> + * init done notifier registered by the machine file. After its ex= ecution >>>>> + * we execute a new notifier that actually starts the injection. W= hen using >>>>> + * irqfd, programming the injection consists in associating eventf= ds to >>>>> + * GSI number,ie. virtual IRQ number >>>>> + */ >>>>> + >>>>> +typedef struct VfioIrqStarterNotifierParams { >>>>> + unsigned int platform_bus_first_irq; >>>>> + Notifier notifier; >>>>> +} VfioIrqStarterNotifierParams; >>>>> + >>>>> +typedef struct VfioIrqStartParams { >>>>> + PlatformBusDevice *pbus; >>>>> + int platform_bus_first_irq; >>>>> +} VfioIrqStartParams; >>>>> + >>>>> +/* Start injection of IRQ for a specific VFIO device */ >>>>> +static int vfio_irq_starter(SysBusDevice *sbdev, void *opaque) >>>>> +{ >>>>> + int i; >>>>> + VfioIrqStartParams *p =3D opaque; >>>>> + VFIOPlatformDevice *vdev; >>>>> + VFIODevice *vbasedev; >>>>> + uint64_t irq_number; >>>>> + PlatformBusDevice *pbus =3D p->pbus; >>>>> + int platform_bus_first_irq =3D p->platform_bus_first_irq; >>>>> + >>>>> + if (object_dynamic_cast(OBJECT(sbdev), TYPE_VFIO_PLATFORM)) { >>>>> + vdev =3D VFIO_PLATFORM_DEVICE(sbdev); >>>>> + vbasedev =3D &vdev->vbasedev; >>>>> + for (i =3D 0; i < vbasedev->num_irqs; i++) { >>>>> + irq_number =3D platform_bus_get_irqn(pbus, sbdev, i) >>>>> + + platform_bus_first_irq; >>>>> + vfio_start_irq_injection(sbdev, i, irq_number); >>>>> + } >>>>> + } >>>>> + return 0; >>>>> +} >>>>> + >>>>> +/* loop on all VFIO platform devices and start their IRQ injection= */ >>>>> +static void vfio_irq_starter_notify(Notifier *notifier, void *data= ) >>>>> +{ >>>>> + VfioIrqStarterNotifierParams *p =3D >>>>> + container_of(notifier, VfioIrqStarterNotifierParams, notif= ier); >>>>> + DeviceState *dev =3D >>>>> + qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BU= S_DEVICE); >>>>> + PlatformBusDevice *pbus =3D PLATFORM_BUS_DEVICE(dev); >>>>> + >>>>> + if (pbus->done_gathering) { >>>>> + VfioIrqStartParams data =3D { >>>>> + .pbus =3D pbus, >>>>> + .platform_bus_first_irq =3D p->platform_bus_first_irq, >>>>> + }; >>>>> + >>>>> + foreach_dynamic_sysbus_device(vfio_irq_starter, &data); >>>>> + } >>>>> +} >>>>> + >>>>> +/* registers the machine init done notifier that will start VFIO I= RQ */ >>>>> +void vfio_register_irq_starter(int platform_bus_first_irq) >>>>> +{ >>>>> + VfioIrqStarterNotifierParams *p =3D g_new(VfioIrqStarterNotifi= erParams, 1); >>>>> + >>>>> + p->platform_bus_first_irq =3D platform_bus_first_irq; >>>>> + p->notifier.notify =3D vfio_irq_starter_notify; >>>>> + qemu_add_machine_init_done_notifier(&p->notifier); >>>> >>>> Could you add a notifier for each device instead? Then the notifier >>>> would be part of the vfio device struct and not some dangling random >>>> pointer :). >>>> >>>> Of course instead of foreach_dynamic_sysbus_device() you would direc= tly >>>> know the device you're dealing with and only handle a single device = per >>>> notifier. >>> >>> Hi Alex, >>> >>> I don't see how to practically follow your request: >>> >>> - at machine init time, VFIO devices are not yet instantiated so I >>> cannot call foreach_dynamic_sysbus_device() there - I was definitivel= y >>> wrong in my first reply :-(). >>> >>> - I can't register a per VFIO device notifier in the VFIO device >>> finalize function because this latter is called after the platform bu= s >>> instantiation. So the IRQ binding notifier (registered in platform bu= s >>> finalize fn) would be called after the IRQ starter notifier. >>> >>> - then to simplify things a bit I could use a qemu_register_reset in >>> place of a machine init done notifier (would relax the call order >>> constraint) but the problem consists in passing the platform bus firs= t >>> irq (all the more so you requested it became part of a const struct) >>> >>> Do I miss something? >> >> So the basic idea is that the device itself calls >> qemu_add_machine_init_done_notifier() in its realize function. The >> Notifier struct would be part of the device state which means you can >> cast yourself into the VFIO device state. >=20 > humm, the vfio device is instantiated in the cmd line so after the > machine init. This means 1st the platform bus binding notifier is > registered (in platform bus realize) and then VFIO irq starter notifier= s > are registered (in VFIO realize). Notifiers beeing executed in the > reverse order of their registration, this would fail. Am I wrong? Bleks. Ok, I see 2 ways out of this: 1) Create a TailNotifier and convert the machine_init_done notifiers to this 2) Add an "irq now populated" notifier function callback in a new PlatformBusDeviceClass struct that you use to describe the PlatformBusDevice class. Call all children's notifiers from the machine_init notifier in the platform bus. The more I think about it, the more I prefer option 2 I think. Alex