From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55624) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu0ns-0007RW-7A for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:16:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu0nm-00069A-8C for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:16:20 -0500 Received: from mail-wg0-f49.google.com ([74.125.82.49]:36819) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu0nl-000692-V3 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:16:14 -0500 Received: by mail-wg0-f49.google.com with SMTP id x12so6698679wgg.22 for ; Thu, 27 Nov 2014 07:16:13 -0800 (PST) Message-ID: <54773FEE.80002@linaro.org> Date: Thu, 27 Nov 2014 16:14:54 +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> <5475E7C8.6040707@linaro.org> <54772F9E.3030701@linaro.org> <54773695.8090800@suse.de> In-Reply-To: <54773695.8090800@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/27/2014 03:35 PM, Alexander Graf wrote: > > > On 27.11.14 15:05, Eric Auger wrote: >> On 11/26/2014 03:46 PM, Eric Auger wrote: >>> 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) >> >> Hi Alex, >> >> I believe I understand your proposal but the issue is to pass the >> platform bus first_irq parameter which is needed to compute the absolute >> IRQ number (=irqfd GSI). VFIO device does not have this info. Platform >> bus doesn't have it either. Only machine file has the info. > > Well, the GIC should have this info as well. That's why I was trying to > point out that you want to ask the GIC about the absolute IRQ number on > its own number space. > > You need to make the connection with the GIC anyway, no? So you need to > somehow get awareness of the GIC device. Or are you hijacking the global > GSI number space? Hi Alex, Well OK I believe I understand your idea: in vfio device, loop on all gic gpios using qdev_get_gpio_in(gicdev, i) and identify i that matches the qemu_irq I want to kick off. That would be feasible if VFIO has a handle to the GIC DeviceState (gicdev), which is not curently the case. so me move the problem to passing the gicdev to vfio ;-) VFIO being mostly generic we could only do that in the derived VFIO device (the famous calxeda xgmac device) or some intermediate vfio arm device - let's be crazy!? ;-) - . GIC derives from std sysbus device (no kind of generic interrupt controller device I could recognize) when parsing the qom tree stuff so I don't see any other solution to retrieve the intc handle after machine creation. I can try that. In that case do you agree with adding/removing sysbus binding_done notifiers in platform bus and drop callback in platform bus class. I would call all registered notifiers at the end of platform_bus_init_notify. Thanks Best Regards Eric > >> >> The "irq now populated" notifier function callback would be called in >> platform bus platform_bus_init_notify or link_sysbus_device I guess, >> already executed in a machine-init-done notifier. The callback would >> need to be called with sbdev and first_irq param to fulfill its task >> (check of VFIO type, IRQFD setup). So I need to pass first_irq to >> platform_bus. Do you agree? Can I add an API? >> >> Besides there would be a single callback per platform bus. Wouldn't it >> be worth to add an infrastructure to add/remove misc "binding_done" >> notifiers and call all registered functions in link_sysbus_device? > > Usually the "realize" function is good enough for 99% of the devices out > there. We're just special because we do lazy binding of IRQs on the > platform bus :). > > > > Alex >