From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment Date: Tue, 28 Aug 2012 02:30:39 +0200 Message-ID: <503C112F.7060307@web.de> References: <825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig1546E9EDE2FDFAEFFE876EC8" Cc: Avi Kivity , Marcelo Tosatti , Alex Williamson , qemu-devel@nongnu.org, kvm@vger.kernel.org, "Michael S. Tsirkin" To: Blue Swirl Return-path: Received: from mout.web.de ([212.227.15.3]:52042 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755104Ab2H1Aaw (ORCPT ); Mon, 27 Aug 2012 20:30:52 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig1546E9EDE2FDFAEFFE876EC8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Blue, thanks for the review. I addressed most of them, the others a commented below. On 2012-08-27 20:56, Blue Swirl wrote: >> +typedef struct AssignedDevice { >> + PCIDevice dev; >> + PCIHostDeviceAddress host; >> + uint32_t dev_id; >> + uint32_t features; >> + int intpin; >> + AssignedDevRegion v_addrs[PCI_NUM_REGIONS - 1]; >> + PCIDevRegions real_device; >> + PCIINTxRoute intx_route; >> + AssignedIRQType assigned_irq_type; >> + struct { >> +#define ASSIGNED_DEVICE_CAP_MSI (1 << 0) >> +#define ASSIGNED_DEVICE_CAP_MSIX (1 << 1) >> + uint32_t available; >> +#define ASSIGNED_DEVICE_MSI_ENABLED (1 << 0) >> +#define ASSIGNED_DEVICE_MSIX_ENABLED (1 << 1) >> +#define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2) >> + uint32_t state; >> + } cap; >> + uint8_t emulate_config_read[PCI_CONFIG_SPACE_SIZE]; >> + uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE]; >> + int msi_virq_nr; >> + int *msi_virq; >> + MSIXTableEntry *msix_table; >> + target_phys_addr_t msix_table_addr; >> + uint16_t msix_max; >> + MemoryRegion mmio; >> + char *configfd_name; >=20 > const? Not if this would mean more casts. DEFINE_PROP_STRING, where this is used, doesn't allow this. =2E.. >> + } else { >> + uint32_t port =3D addr + dev_region->u.r_baseport; >> + >> + if (data) { >> + DEBUG("out data=3D%lx, size=3D%d, e_phys=3D%lx, host=3D%x= \n", >> + *data, size, addr, port); >> + switch (size) { >> + case 1: >> + outb(*data, port); >> + break; >> + case 2: >> + outw(*data, port); >> + break; >> + case 4: >> + outl(*data, port); >> + break; >=20 > Maybe add case 8: and default: with abort(), also below. PIO is never 8 bytes long, the generic layer protects us. =2E.. >> + >> + fclose(f); >> + >> + /* read and fill vendor ID */ >> + v =3D get_real_vendor_id(dir, &id); >> + if (v) { >> + return 1; >> + } >> + pci_dev->dev.config[0] =3D id & 0xff; >> + pci_dev->dev.config[1] =3D (id & 0xff00) >> 8; >> + >> + /* read and fill device ID */ >> + v =3D get_real_device_id(dir, &id); >> + if (v) { >> + return 1; >> + } >> + pci_dev->dev.config[2] =3D id & 0xff; >> + pci_dev->dev.config[3] =3D (id & 0xff00) >> 8; >> + >> + pci_word_test_and_clear_mask(pci_dev->emulate_config_write + PCI_= COMMAND, >> + PCI_COMMAND_MASTER | PCI_COMMAND_INT= X_DISABLE); >> + >> + dev->region_number =3D r; >> + return 0; >> +} >=20 > Pretty long function, how about refactoring? Possibly, but I'd prefer to do such changes in-tree, after the more important refactoring on MSI[-X] is done. =2E.. >> + if (ctrl_byte & PCI_MSI_FLAGS_ENABLE) { >> + uint8_t *pos =3D pci_dev->config + pci_dev->msi_cap; >> + MSIMessage msg; >> + int virq; >> + >> + msg.address =3D pci_get_long(pos + PCI_MSI_ADDRESS_LO); >> + msg.data =3D pci_get_word(pos + PCI_MSI_DATA_32); >> + virq =3D kvm_irqchip_add_msi_route(kvm_state, msg); >> + if (virq < 0) { >> + perror("assigned_dev_update_msi: kvm_irqchip_add_msi_rout= e"); >> + return; >> + } >> + >> + assigned_dev->msi_virq =3D g_malloc(sizeof(*assigned_dev->msi= _virq)); >=20 > Is this ever freed? Yep, in free_msi_virqs. If you think you spotted a path where this is not the case, let me know. =2E.. >> + >> +static Property da_properties[] =3D { >=20 > const? Nope, properties must remain writable. >=20 >> + DEFINE_PROP_PCI_HOST_DEVADDR("host", AssignedDevice, host), >> + DEFINE_PROP_BIT("prefer_msi", AssignedDevice, features, >> + ASSIGNED_DEVICE_PREFER_MSI_BIT, false), >> + DEFINE_PROP_BIT("share_intx", AssignedDevice, features, >> + ASSIGNED_DEVICE_SHARE_INTX_BIT, true), >> + DEFINE_PROP_INT32("bootindex", AssignedDevice, bootindex, -1), >> + DEFINE_PROP_STRING("configfd", AssignedDevice, configfd_name), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + Jan --------------enig1546E9EDE2FDFAEFFE876EC8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iEYEARECAAYFAlA8ETMACgkQitSsb3rl5xSZWgCfccrm5KKBDBMXaOL+vMTXMr3F TUUAnR9710TpgWX86WVlAtbnPrYaSxIf =Tq9s -----END PGP SIGNATURE----- --------------enig1546E9EDE2FDFAEFFE876EC8-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42516) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T69hi-0002kV-Oc for qemu-devel@nongnu.org; Mon, 27 Aug 2012 20:30:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T69hh-0006eL-AR for qemu-devel@nongnu.org; Mon, 27 Aug 2012 20:30:50 -0400 Received: from mout.web.de ([212.227.15.3]:57706) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T69hg-0006e8-Vn for qemu-devel@nongnu.org; Mon, 27 Aug 2012 20:30:49 -0400 Message-ID: <503C112F.7060307@web.de> Date: Tue, 28 Aug 2012 02:30:39 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig1546E9EDE2FDFAEFFE876EC8" Subject: Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: kvm@vger.kernel.org, "Michael S. Tsirkin" , Marcelo Tosatti , qemu-devel@nongnu.org, Alex Williamson , Avi Kivity This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig1546E9EDE2FDFAEFFE876EC8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Blue, thanks for the review. I addressed most of them, the others a commented below. On 2012-08-27 20:56, Blue Swirl wrote: >> +typedef struct AssignedDevice { >> + PCIDevice dev; >> + PCIHostDeviceAddress host; >> + uint32_t dev_id; >> + uint32_t features; >> + int intpin; >> + AssignedDevRegion v_addrs[PCI_NUM_REGIONS - 1]; >> + PCIDevRegions real_device; >> + PCIINTxRoute intx_route; >> + AssignedIRQType assigned_irq_type; >> + struct { >> +#define ASSIGNED_DEVICE_CAP_MSI (1 << 0) >> +#define ASSIGNED_DEVICE_CAP_MSIX (1 << 1) >> + uint32_t available; >> +#define ASSIGNED_DEVICE_MSI_ENABLED (1 << 0) >> +#define ASSIGNED_DEVICE_MSIX_ENABLED (1 << 1) >> +#define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2) >> + uint32_t state; >> + } cap; >> + uint8_t emulate_config_read[PCI_CONFIG_SPACE_SIZE]; >> + uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE]; >> + int msi_virq_nr; >> + int *msi_virq; >> + MSIXTableEntry *msix_table; >> + target_phys_addr_t msix_table_addr; >> + uint16_t msix_max; >> + MemoryRegion mmio; >> + char *configfd_name; >=20 > const? Not if this would mean more casts. DEFINE_PROP_STRING, where this is used, doesn't allow this. =2E.. >> + } else { >> + uint32_t port =3D addr + dev_region->u.r_baseport; >> + >> + if (data) { >> + DEBUG("out data=3D%lx, size=3D%d, e_phys=3D%lx, host=3D%x= \n", >> + *data, size, addr, port); >> + switch (size) { >> + case 1: >> + outb(*data, port); >> + break; >> + case 2: >> + outw(*data, port); >> + break; >> + case 4: >> + outl(*data, port); >> + break; >=20 > Maybe add case 8: and default: with abort(), also below. PIO is never 8 bytes long, the generic layer protects us. =2E.. >> + >> + fclose(f); >> + >> + /* read and fill vendor ID */ >> + v =3D get_real_vendor_id(dir, &id); >> + if (v) { >> + return 1; >> + } >> + pci_dev->dev.config[0] =3D id & 0xff; >> + pci_dev->dev.config[1] =3D (id & 0xff00) >> 8; >> + >> + /* read and fill device ID */ >> + v =3D get_real_device_id(dir, &id); >> + if (v) { >> + return 1; >> + } >> + pci_dev->dev.config[2] =3D id & 0xff; >> + pci_dev->dev.config[3] =3D (id & 0xff00) >> 8; >> + >> + pci_word_test_and_clear_mask(pci_dev->emulate_config_write + PCI_= COMMAND, >> + PCI_COMMAND_MASTER | PCI_COMMAND_INT= X_DISABLE); >> + >> + dev->region_number =3D r; >> + return 0; >> +} >=20 > Pretty long function, how about refactoring? Possibly, but I'd prefer to do such changes in-tree, after the more important refactoring on MSI[-X] is done. =2E.. >> + if (ctrl_byte & PCI_MSI_FLAGS_ENABLE) { >> + uint8_t *pos =3D pci_dev->config + pci_dev->msi_cap; >> + MSIMessage msg; >> + int virq; >> + >> + msg.address =3D pci_get_long(pos + PCI_MSI_ADDRESS_LO); >> + msg.data =3D pci_get_word(pos + PCI_MSI_DATA_32); >> + virq =3D kvm_irqchip_add_msi_route(kvm_state, msg); >> + if (virq < 0) { >> + perror("assigned_dev_update_msi: kvm_irqchip_add_msi_rout= e"); >> + return; >> + } >> + >> + assigned_dev->msi_virq =3D g_malloc(sizeof(*assigned_dev->msi= _virq)); >=20 > Is this ever freed? Yep, in free_msi_virqs. If you think you spotted a path where this is not the case, let me know. =2E.. >> + >> +static Property da_properties[] =3D { >=20 > const? Nope, properties must remain writable. >=20 >> + DEFINE_PROP_PCI_HOST_DEVADDR("host", AssignedDevice, host), >> + DEFINE_PROP_BIT("prefer_msi", AssignedDevice, features, >> + ASSIGNED_DEVICE_PREFER_MSI_BIT, false), >> + DEFINE_PROP_BIT("share_intx", AssignedDevice, features, >> + ASSIGNED_DEVICE_SHARE_INTX_BIT, true), >> + DEFINE_PROP_INT32("bootindex", AssignedDevice, bootindex, -1), >> + DEFINE_PROP_STRING("configfd", AssignedDevice, configfd_name), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + Jan --------------enig1546E9EDE2FDFAEFFE876EC8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iEYEARECAAYFAlA8ETMACgkQitSsb3rl5xSZWgCfccrm5KKBDBMXaOL+vMTXMr3F TUUAnR9710TpgWX86WVlAtbnPrYaSxIf =Tq9s -----END PGP SIGNATURE----- --------------enig1546E9EDE2FDFAEFFE876EC8--