From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtdss-0000Ua-ER for qemu-devel@nongnu.org; Wed, 26 Nov 2014 09:48:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xtdsm-00034M-ER for qemu-devel@nongnu.org; Wed, 26 Nov 2014 09:47:58 -0500 Received: from mail-wg0-f49.google.com ([74.125.82.49]:47266) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtdsm-000346-37 for qemu-devel@nongnu.org; Wed, 26 Nov 2014 09:47:52 -0500 Received: by mail-wg0-f49.google.com with SMTP id x12so3888530wgg.8 for ; Wed, 26 Nov 2014 06:47:51 -0800 (PST) Message-ID: <5475E7C8.6040707@linaro.org> Date: Wed, 26 Nov 2014 15:46:32 +0100 From: Eric Auger 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> <5475B781.5030009@suse.de> In-Reply-To: <5475B781.5030009@suse.de> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Alexander Graf , 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 11/26/2014 12:20 PM, Alexander Graf wrote: > > > 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 notifier: >>>>>> + * this is needed since at finalize time, the device IRQ are not yet >>>>>> + * bound to the platform bus IRQ. It is assumed here dynamic instantiation >>>>>> + * always is used. Binding to the platform bus IRQ happens on a machine >>>>>> + * init done notifier registered by the machine file. After its execution >>>>>> + * we execute a new notifier that actually starts the injection. When using >>>>>> + * irqfd, programming the injection consists in associating eventfds 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 = opaque; >>>>>> + VFIOPlatformDevice *vdev; >>>>>> + VFIODevice *vbasedev; >>>>>> + uint64_t irq_number; >>>>>> + PlatformBusDevice *pbus = p->pbus; >>>>>> + int platform_bus_first_irq = p->platform_bus_first_irq; >>>>>> + >>>>>> + if (object_dynamic_cast(OBJECT(sbdev), TYPE_VFIO_PLATFORM)) { >>>>>> + vdev = VFIO_PLATFORM_DEVICE(sbdev); >>>>>> + vbasedev = &vdev->vbasedev; >>>>>> + for (i = 0; i < vbasedev->num_irqs; i++) { >>>>>> + irq_number = 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 = >>>>>> + container_of(notifier, VfioIrqStarterNotifierParams, notifier); >>>>>> + DeviceState *dev = >>>>>> + qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE); >>>>>> + PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(dev); >>>>>> + >>>>>> + if (pbus->done_gathering) { >>>>>> + VfioIrqStartParams data = { >>>>>> + .pbus = pbus, >>>>>> + .platform_bus_first_irq = p->platform_bus_first_irq, >>>>>> + }; >>>>>> + >>>>>> + foreach_dynamic_sysbus_device(vfio_irq_starter, &data); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +/* registers the machine init done notifier that will start VFIO IRQ */ >>>>>> +void vfio_register_irq_starter(int platform_bus_first_irq) >>>>>> +{ >>>>>> + VfioIrqStarterNotifierParams *p = g_new(VfioIrqStarterNotifierParams, 1); >>>>>> + >>>>>> + p->platform_bus_first_irq = platform_bus_first_irq; >>>>>> + p->notifier.notify = 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 directly >>>>> 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 definitively >>>> 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 bus >>>> instantiation. So the IRQ binding notifier (registered in platform bus >>>> 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 first >>>> 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. >> >> 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 notifiers >> 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. Hi Alex, ok I work on 2) Thanks for your guidance Eric > > > Alex >