From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXNrs-0000XC-Tw for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:24:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXNro-0006tJ-Kw for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:23:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46818) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXNro-0006tD-DF for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:23:56 -0400 Date: Wed, 10 Aug 2016 15:23:50 +0800 From: Peter Xu Message-ID: <20160810072350.GK4201@pxdev.xzpeter.org> References: <1470753137-18354-1-git-send-email-davidkiarie4@gmail.com> <1470753137-18354-2-git-send-email-davidkiarie4@gmail.com> <20160810054155.GH4201@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC 1/2] hw/msi: Allow platform devices to use explicit SID List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie Cc: QEMU Developers , Peter Maydell , Eduardo Habkost , "Michael S. Tsirkin" , "J. Kiszka" , Valentine Sinitsyn , Paolo Bonzini On Wed, Aug 10, 2016 at 09:35:25AM +0300, David Kiarie wrote: > On Wed, Aug 10, 2016 at 8:41 AM, Peter Xu wrote: > > > On Tue, Aug 09, 2016 at 05:32:16PM +0300, David Kiarie wrote: > > > When using IOMMU platform devices like IOAPIC are required to make > > > interrupt remapping requests using explicit SID.We affiliate an MSI > > > route with a requester ID and a PCI device if present which ensures > > > that platform devices can call IOMMU interrupt remapping code with > > > explicit SID while maintaining compatility with the original code > > > which mainly dealt with PCI devices. > > > > > > Signed-off-by: David Kiarie > > > > Hi, > > > > This idea is good to me overall, with some tiny comments below. > > > > [...] > > > > > -static void ioapic_service(IOAPICCommonState *s) > > > +static void ioapic_write_ioapic_as(IOAPICCommonState *s, uint32_t > > data, uint64_t addr) > > > > Rename to ioapic_as_write()? > > > > [...] > > > > > @@ -385,12 +393,23 @@ static void ioapic_machine_done_notify(Notifier > > *notifier, void *data) > > > > > > if (kvm_irqchip_is_split()) { > > > X86IOMMUState *iommu = x86_iommu_get_default(); > > > + MSIMessage msg = {0, 0}; > > > + int i; > > > + > > > if (iommu) { > > > /* Register this IOAPIC with IOMMU IEC notifier, so that > > > * when there are IR invalidates, we can be notified to > > > * update kernel IR cache. */ > > > - x86_iommu_iec_register_notifier(iommu, > > ioapic_iec_notifier, s); > > > + s->devid = iommu->ioapic_bdf; > > > + /* update IOAPIC routes to the right SID */ > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > + kvm_irqchip_update_msi_route(kvm_state, i, msg, NULL, > > s->devid); > > > + } > > > + kvm_irqchip_commit_routes(kvm_state); > > > > Here, not sure whether it'll be better if we remove > > kvm_irqchip_add_msi_route() in kvm_arch_init_irq_routing() directly > > and call them here. So no extra update needed. > > > > Thought about that too but I was worried another device might reserve > routes before IOAPIC does. Right. Had a quick look, only thing I am not sure is how ivshmem setup its routes. It seems that it's done before this notifier. So maybe your method is good to keep. -- peterx