From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60009) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu0zm-00046a-Bp for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:28:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xu0zc-0002Ct-Q0 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:28:38 -0500 Received: from cantor2.suse.de ([195.135.220.15]:35282 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xu0zc-0002CR-GO for qemu-devel@nongnu.org; Thu, 27 Nov 2014 10:28:28 -0500 Message-ID: <54774318.4060607@suse.de> Date: Thu, 27 Nov 2014 16:28:24 +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> In-Reply-To: <54773FEE.80002@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 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 d= one 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 i= ts execution >>>>>>>>>> + * we execute a new notifier that actually starts the injecti= on. When using >>>>>>>>>> + * irqfd, programming the injection consists in associating e= ventfds 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 inje= ction */ >>>>>>>>>> +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_PLATFO= RM_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_first= _irq, >>>>>>>>>> + }; >>>>>>>>>> + >>>>>>>>>> + foreach_dynamic_sysbus_device(vfio_irq_starter, &data= ); >>>>>>>>>> + } >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> +/* registers the machine init done notifier that will start V= FIO IRQ */ >>>>>>>>>> +void vfio_register_irq_starter(int platform_bus_first_irq) >>>>>>>>>> +{ >>>>>>>>>> + VfioIrqStarterNotifierParams *p =3D g_new(VfioIrqStarterN= otifierParams, 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 noti= fier >>>>>>>>> would be part of the vfio device struct and not some dangling r= andom >>>>>>>>> pointer :). >>>>>>>>> >>>>>>>>> Of course instead of foreach_dynamic_sysbus_device() you would = directly >>>>>>>>> know the device you're dealing with and only handle a single de= vice 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 defini= tively >>>>>>>> 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 platfo= rm bus >>>>>>>> instantiation. So the IRQ binding notifier (registered in platfo= rm bus >>>>>>>> finalize fn) would be called after the IRQ starter notifier. >>>>>>>> >>>>>>>> - then to simplify things a bit I could use a qemu_register_rese= t in >>>>>>>> place of a machine init done notifier (would relax the call orde= r >>>>>>>> constraint) but the problem consists in passing the platform bus= first >>>>>>>> irq (all the more so you requested it became part of a const str= uct) >>>>>>>> >>>>>>>> Do I miss something? >>>>>>> >>>>>>> So the basic idea is that the device itself calls >>>>>>> qemu_add_machine_init_done_notifier() in its realize function. Th= e >>>>>>> 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 not= ifiers >>>>>> 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 notifi= ers >>>>> 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 absol= ute >>> IRQ number (=3Dirqfd GSI). VFIO device does not have this info. Platf= orm >>> 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 t= o >> point out that you want to ask the GIC about the absolute IRQ number o= n >> its own number space. >> >> You need to make the connection with the GIC anyway, no? So you need t= o >> somehow get awareness of the GIC device. Or are you hijacking the glob= al >> GSI number space? >=20 > Hi Alex, >=20 > 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 ;-) 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. 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. I would also do the lookup the other way around. The GPIO / IRQ number mapping is reasonably local to the GIC device, so I'd rather call a GIC function to find the ID: kvm_gic_get_irq_gsi(s->gic_link, qdev_get_gpio_in(s, i)); > 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 (n= o > kind of generic interrupt controller device I could recognize) when > parsing the qom tree stuff so I don't see any other solution to retriev= e > the intc handle after machine creation. >=20 > 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 bu= s > class. I would call all registered notifiers at the end of > platform_bus_init_notify. Not sure I understand what you're asking for :). Alex