From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khlebnikov Subject: Re: [PATCH] device-assignment: move irqs update to piix emulation Date: Mon, 28 Mar 2011 12:27:00 +0400 Message-ID: <4D904654.6010803@parallels.com> References: <20110327192254.20098.21743.stgit@localhost6> <1301260670.14532.145.camel@x201> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" To: Alex Williamson Return-path: Received: from relay.parallels.com ([195.214.232.42]:36854 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753306Ab1C1I1D (ORCPT ); Mon, 28 Mar 2011 04:27:03 -0400 In-Reply-To: <1301260670.14532.145.camel@x201> Sender: kvm-owner@vger.kernel.org List-ID: Alex Williamson wrote: > On Sun, 2011-03-27 at 23:22 +0400, Konstantin Khlebnikov wrote: >> Move assigned devices irq reroute hook from generic pci code to piix emulation. >> >> Actually without this patch this hook had never worked, because pci.c not >> include config.h and CONFIG_KVM_DEVICE_ASSIGNMENT there always undefined. > > Actually it's worked fine until very recently. I've got a patch on the > list to move pci.c back to a target object so the right config gets > included. Ok, my comment is inaccurate. > >> Signed-off-by: Konstantin Khlebnikov >> --- >> hw/pc.h | 4 ---- >> hw/pci.c | 8 -------- >> hw/piix_pci.c | 14 ++++++++++++++ >> 3 files changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/hw/pc.h b/hw/pc.h >> index 1291e2d..34f32ee 100644 >> --- a/hw/pc.h >> +++ b/hw/pc.h >> @@ -172,10 +172,6 @@ extern int no_hpet; >> void pcspk_init(ISADevice *pit); >> int pcspk_audio_init(qemu_irq *pic); >> >> -/* piix_pci.c */ >> -/* config space register for IRQ routing */ >> -#define PIIX_CONFIG_IRQ_ROUTE 0x60 >> - >> struct PCII440FXState; >> typedef struct PCII440FXState PCII440FXState; >> >> diff --git a/hw/pci.c b/hw/pci.c >> index 730df5f..27af9fe 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -31,7 +31,6 @@ >> #include "loader.h" >> #include "hw/pc.h" >> #include "kvm.h" >> -#include "device-assignment.h" >> #include "qemu-objects.h" >> #include "range.h" >> >> @@ -1165,13 +1164,6 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l) >> d->config[addr + i]&= ~(val& w1cmask); /* W1C: Write 1 to Clear */ >> } >> >> -#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT >> - if (kvm_enabled()&& kvm_irqchip_in_kernel()&& >> - addr>= PIIX_CONFIG_IRQ_ROUTE&& >> - addr< PIIX_CONFIG_IRQ_ROUTE + 4) >> - assigned_dev_update_irqs(); >> -#endif /* CONFIG_KVM_DEVICE_ASSIGNMENT */ >> - >> if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) || >> ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) || >> ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) || >> diff --git a/hw/piix_pci.c b/hw/piix_pci.c >> index 24bfe8e..d1fcdcd 100644 >> --- a/hw/piix_pci.c >> +++ b/hw/piix_pci.c >> @@ -30,6 +30,7 @@ >> #include "sysbus.h" >> #include "range.h" >> #include "kvm.h" >> +#include "device-assignment.h" >> >> /* >> * I440FX chipset data sheet. >> @@ -353,6 +354,18 @@ static int piix3_initfn(PCIDevice *dev) >> return 0; >> } >> >> +static void piix3_write_config(PCIDevice *dev, >> + uint32_t addr, uint32_t val, int len) >> +{ >> + pci_default_write_config(dev, addr, val, len); >> + >> +#ifdef CONFIG_KVM_DEVICE_ASSIGNMENT >> + if (kvm_enabled()&& kvm_irqchip_in_kernel()&& >> + ranges_overlap(addr, len, 0x60, 4)) > > I think this should still have a #define somewhere, or else 0x60 becomes > an unsearchable magic number. Otherwise, this looks fine to me. I've > actually sent out RFC patches making a similar change to try to make > this device assignment hook more generic. In piix emulation a lot of magic constants. I see only two defines for hardware registers. Before my patch magic 0x60 was used there in four places =) > > Alex > >> + assigned_dev_update_irqs(); >> +#endif /* CONFIG_KVM_DEVICE_ASSIGNMENT */ >> +} >> + >> static PCIDeviceInfo i440fx_info[] = { >> { >> .qdev.name = "i440FX", >> @@ -371,6 +384,7 @@ static PCIDeviceInfo i440fx_info[] = { >> .qdev.no_user = 1, >> .no_hotplug = 1, >> .init = piix3_initfn, >> + .config_write = piix3_write_config, >> },{ >> /* end of list */ >> } >> > > >