From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39178) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu1Q4-000677-Gs for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:55:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu1Pz-0004O7-87 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:55:48 -0500 Received: from cantor2.suse.de ([195.135.220.15]:35917 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu1Py-0004NC-My for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:55:43 -0500 Message-ID: <54774979.9090701@suse.de> Date: Thu, 27 Nov 2014 16:55:37 +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> <5475B781.5030009@suse.de> <5475E7C8.6040707@linaro.org> <54772F9E.3030701@linaro.org> <54773695.8090800@suse.de> <54773FEE.80002@linaro.org> <54774318.4060607@suse.de> In-Reply-To: <54774318.4060607@suse.de> 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: patches@linaro.org, Kim Phillips , will.deacon@arm.com, stuart.yoder@freescale.com, alex.williamson@redhat.com, kvmarm@lists.cs.columbia.edu On 27.11.14 16:28, Alexander Graf wrote: >=20 >=20 > On 27.11.14 16:14, Eric Auger wrote: >> 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 o= n a machine >>>>>>>>>>> + * init done notifier registered by the machine file. After = its execution >>>>>>>>>>> + * we execute a new notifier that actually starts the inject= ion. 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 *opaqu= e) >>>>>>>>>>> +{ >>>>>>>>>>> + 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_PLATFOR= M)) { >>>>>>>>>>> + 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 inj= ection */ >>>>>>>>>>> +static void vfio_irq_starter_notify(Notifier *notifier, void= *data) >>>>>>>>>>> +{ >>>>>>>>>>> + VfioIrqStarterNotifierParams *p =3D >>>>>>>>>>> + container_of(notifier, VfioIrqStarterNotifierParams,= notifier); >>>>>>>>>>> + DeviceState *dev =3D >>>>>>>>>>> + qdev_find_recursive(sysbus_get_default(), TYPE_PLATF= ORM_BUS_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_firs= t_irq, >>>>>>>>>>> + }; >>>>>>>>>>> + >>>>>>>>>>> + foreach_dynamic_sysbus_device(vfio_irq_starter, &dat= a); >>>>>>>>>>> + } >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +/* registers the machine init done notifier that will start = VFIO IRQ */ >>>>>>>>>>> +void vfio_register_irq_starter(int platform_bus_first_irq) >>>>>>>>>>> +{ >>>>>>>>>>> + VfioIrqStarterNotifierParams *p =3D g_new(VfioIrqStarter= NotifierParams, 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 not= ifier >>>>>>>>>> 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 d= evice per >>>>>>>>>> notifier. >>>>>>>>> >>>>>>>>> Hi Alex, >>>>>>>>> >>>>>>>>> I don't see how to practically follow your request: >>>>>>>>> >>>>>>>>> - at machine init time, VFIO devices are not yet instantiated s= o I >>>>>>>>> cannot call foreach_dynamic_sysbus_device() there - I was defin= itively >>>>>>>>> wrong in my first reply :-(). >>>>>>>>> >>>>>>>>> - I can't register a per VFIO device notifier in the VFIO devic= e >>>>>>>>> finalize function because this latter is called after the platf= orm bus >>>>>>>>> instantiation. So the IRQ binding notifier (registered in platf= orm bus >>>>>>>>> finalize fn) would be called after the IRQ starter notifier. >>>>>>>>> >>>>>>>>> - then to simplify things a bit I could use a qemu_register_res= et in >>>>>>>>> place of a machine init done notifier (would relax the call ord= er >>>>>>>>> constraint) but the problem consists in passing the platform bu= s first >>>>>>>>> irq (all the more so you requested it became part of a const st= ruct) >>>>>>>>> >>>>>>>>> Do I miss something? >>>>>>>> >>>>>>>> So the basic idea is that the device itself calls >>>>>>>> qemu_add_machine_init_done_notifier() in its realize function. T= he >>>>>>>> Notifier struct would be part of the device state which means yo= u can >>>>>>>> cast yourself into the VFIO device state. >>>>>>> >>>>>>> humm, the vfio device is instantiated in the cmd line so after th= e >>>>>>> machine init. This means 1st the platform bus binding notifier is >>>>>>> registered (in platform bus realize) and then VFIO irq starter no= tifiers >>>>>>> are registered (in VFIO realize). Notifiers beeing executed in th= e >>>>>>> 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 notif= iers >>>>>> to this >>>>>> >>>>>> 2) Add an "irq now populated" notifier function callback in a ne= w >>>>>> 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 abso= lute >>>> IRQ number (=3Dirqfd GSI). VFIO device does not have this info. Plat= form >>>> 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 glo= bal >>> 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 VFI= O >> has a handle to the GIC DeviceState (gicdev), which is not curently th= e >> case. so me move the problem to passing the gicdev to vfio ;-) >=20 > That should be easy - make it a link property. In fact, this would be > one of those cases where not generalizing the code would've been a good > idea. >=20 > If device creation would live in the machine file, the machine could > automatically set the link. Maybe you can still get there somehow? You > could add a machine callback in the device allocation function. If this gets too messy, I think doing a machine attribute would work as well here. Check out the way we pass the e500-ccsr object on e500: http://git.qemu.org/?p=3Dqemu.git;a=3Dblob;f=3Dhw/pci-host/ppce500.c;h=3D= 1b4c0f00236e8005c261da527d416fe6a053b353;hb=3DHEAD#l337 http://git.qemu.org/?p=3Dqemu.git;a=3Dblob;f=3Dhw/ppc/e500.c;h=3D2832fc0d= a444d89737768f7c4dcb0638e2625750;hb=3DHEAD#l873 I think doing an actual link would be cleaner, but at least the above gets you to an acceptable state that can still be improved with links later - the basic idea is the same :). Alex