hv: vpci: clear vdev structure on device deassign

In devicemodel, a passthrough device is deassigned and then assigned to
guest on guest reboot. Each time hypervisor allocates a new pci_vdev
structure to keep its info. As it was stored in a statically-allocated
array, it will eventually use up all slots, resulting both resource
leak and out-of-bounds access.

Fix it by clearing the corresponding vdev structure on device deassign,
thus a bitmap is introduced to track the usage, replacing the existing
array count.

Tracked-On: #8590
Signed-off-by: Jiaqing Zhao <jiaqing.zhao@linux.intel.com>
Reviewed-by: Junjie Mao <junjie.mao@intel.com>

hypervisor/dm/vpci/vdev.c

@@ -75,7 +75,6 @@ void pci_vdev_write_vcfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes,
 
 /**
  * @pre vpci != NULL
- * @pre vpci->pci_vdev_cnt <= CONFIG_MAX_PCI_DEV_NUM
  */
 struct pci_vdev *pci_find_vdev(struct acrn_vpci *vpci, union pci_bdf vbdf)
 {

hypervisor/dm/vpci/vpci.c

@@ -278,7 +278,7 @@ void deinit_vpci(struct acrn_vm *vm)
 	struct pci_vdev *vdev, *parent_vdev;
 	uint32_t i;
 
-	for (i = 0U; i < vm->vpci.pci_vdev_cnt; i++) {
+	for (i = 0U; i < CONFIG_MAX_PCI_DEV_NUM; i++) {
 		vdev = (struct pci_vdev *) &(vm->vpci.pci_vdevs[i]);
 
 		/* Only deinit the VM's own devices */
@@ -657,42 +657,65 @@ static int32_t vpci_write_cfg(struct acrn_vpci *vpci, union pci_bdf bdf,
  *                          Otherwise, it is NULL.
  *
  * @pre vpci != NULL
- * @pre vpci.pci_vdev_cnt <= CONFIG_MAX_PCI_DEV_NUM
  *
  * @return If there's a successfully initialized vdev structure return it, otherwise return NULL;
  */
 struct pci_vdev *vpci_init_vdev(struct acrn_vpci *vpci, struct acrn_vm_pci_dev_config *dev_config, struct pci_vdev *parent_pf_vdev)
 {
-	struct pci_vdev *vdev = &vpci->pci_vdevs[vpci->pci_vdev_cnt];
+	struct pci_vdev *vdev = NULL;
+	uint32_t id = (uint32_t)ffz64_ex(vpci->vdev_bitmaps, CONFIG_MAX_PCI_DEV_NUM);
 
-	vpci->pci_vdev_cnt++;
-	vdev->vpci = vpci;
-	vdev->bdf.value = dev_config->vbdf.value;
-	vdev->pdev = dev_config->pdev;
-	vdev->pci_dev_config = dev_config;
-	vdev->phyfun = parent_pf_vdev;
+	if (id < CONFIG_MAX_PCI_DEV_NUM) {
+		bitmap_set_nolock((id & 0x3FU), &vpci->vdev_bitmaps[id >> 6U]);
 
-	hlist_add_head(&vdev->link, &vpci->vdevs_hlist_heads[hash64(dev_config->vbdf.value, VDEV_LIST_HASHBITS)]);
-	if (dev_config->vdev_ops != NULL) {
-		vdev->vdev_ops = dev_config->vdev_ops;
-	} else {
-		vdev->vdev_ops = &pci_pt_dev_ops;
-		ASSERT(dev_config->emu_type == PCI_DEV_TYPE_PTDEV,
-			"Only PCI_DEV_TYPE_PTDEV could not configure vdev_ops");
-		ASSERT(dev_config->pdev != NULL, "PCI PTDev is not present on platform!");
+		vdev = &vpci->pci_vdevs[id];
+		vdev->id = id;
+		vdev->vpci = vpci;
+		vdev->bdf.value = dev_config->vbdf.value;
+		vdev->pdev = dev_config->pdev;
+		vdev->pci_dev_config = dev_config;
+		vdev->phyfun = parent_pf_vdev;
+
+		hlist_add_head(&vdev->link, &vpci->vdevs_hlist_heads[hash64(dev_config->vbdf.value, VDEV_LIST_HASHBITS)]);
+		if (dev_config->vdev_ops != NULL) {
+			vdev->vdev_ops = dev_config->vdev_ops;
+		} else {
+			vdev->vdev_ops = &pci_pt_dev_ops;
+			ASSERT(dev_config->emu_type == PCI_DEV_TYPE_PTDEV,
+				"Only PCI_DEV_TYPE_PTDEV could not configure vdev_ops");
+			ASSERT(dev_config->pdev != NULL, "PCI PTDev is not present on platform!");
+		}
+		vdev->vdev_ops->init_vdev(vdev);
 	}
-
-	vdev->vdev_ops->init_vdev(vdev);
-
 	return vdev;
 }
 
+/**
+ * @brief Deinitialize a vdev structure.
+ * 
+ * The caller of the function vpci_init_vdev should guarantee execution atomically.
+ *
+ * @param vdev              Pointer to a vdev structure
+ *
+ * @pre vpci != NULL
+ * @pre vdev->vpci != NULL
+ */
+void vpci_deinit_vdev(struct pci_vdev *vdev)
+{
+	vdev->vdev_ops->deinit_vdev(vdev);
+
+	hlist_del(&vdev->link);
+	bitmap_clear_nolock((vdev->id & 0x3FU), &vdev->vpci->vdev_bitmaps[vdev->id >> 6U]);
+	memset(vdev, 0U, sizeof(struct pci_vdev));
+}
+
 /**
  * @pre vm != NULL
  */
 static int32_t vpci_init_vdevs(struct acrn_vm *vm)
 {
 	uint16_t idx;
+	struct pci_vdev *vdev;
 	struct acrn_vpci *vpci = &(vm->vpci);
 	const struct acrn_vm_config *vm_config = get_vm_config(vpci2vm(vpci)->vm_id);
 	int32_t ret = 0;
@@ -700,7 +723,11 @@ static int32_t vpci_init_vdevs(struct acrn_vm *vm)
 	for (idx = 0U; idx < vm_config->pci_dev_num; idx++) {
 		/* the vdev whose vBDF is unassigned will be created by hypercall */
 		if ((!is_postlaunched_vm(vm)) || (vm_config->pci_devs[idx].vbdf.value != UNASSIGNED_VBDF)) {
-			(void)vpci_init_vdev(vpci, &vm_config->pci_devs[idx], NULL);
+			vdev = vpci_init_vdev(vpci, &vm_config->pci_devs[idx], NULL);
+			if (vdev == NULL) {
+				pr_err("%s: failed to initialize vpci, increase MAX_PCI_DEV_NUM in scenario!\n", __func__);
+				break;
+			}
 			ret = check_pt_dev_pio_bars(&vpci->pci_vdevs[idx]);
 			if (ret != 0) {
 				break;
@@ -750,33 +777,40 @@ int32_t vpci_assign_pcidev(struct acrn_vm *tgt_vm, struct acrn_pcidev *pcidev)
 
 		spinlock_obtain(&tgt_vm->vpci.lock);
 		vdev = vpci_init_vdev(vpci, vdev_in_service_vm->pci_dev_config, vdev_in_service_vm->phyfun);
-		pci_vdev_write_vcfg(vdev, PCIR_INTERRUPT_LINE, 1U, pcidev->intr_line);
-		pci_vdev_write_vcfg(vdev, PCIR_INTERRUPT_PIN, 1U, pcidev->intr_pin);
-		for (idx = 0U; idx < vdev->nr_bars; idx++) {
-			/* VF is assigned to a User VM */
-			if (vdev->phyfun != NULL) {
-				vdev->vbars[idx] = vdev_in_service_vm->vbars[idx];
-				if (has_msix_cap(vdev) && (idx == vdev->msix.table_bar)) {
-					vdev->msix.mmio_hpa = vdev->vbars[idx].base_hpa;
-					vdev->msix.mmio_size = vdev->vbars[idx].size;
+		if (vdev != NULL) {
+			pci_vdev_write_vcfg(vdev, PCIR_INTERRUPT_LINE, 1U, pcidev->intr_line);
+			pci_vdev_write_vcfg(vdev, PCIR_INTERRUPT_PIN, 1U, pcidev->intr_pin);
+			for (idx = 0U; idx < vdev->nr_bars; idx++) {
+				/* VF is assigned to a User VM */
+				if (vdev->phyfun != NULL) {
+					vdev->vbars[idx] = vdev_in_service_vm->vbars[idx];
+					if (has_msix_cap(vdev) && (idx == vdev->msix.table_bar)) {
+						vdev->msix.mmio_hpa = vdev->vbars[idx].base_hpa;
+						vdev->msix.mmio_size = vdev->vbars[idx].size;
+					}
 				}
+				pci_vdev_write_vbar(vdev, idx, pcidev->bar[idx]);
 			}
-			pci_vdev_write_vbar(vdev, idx, pcidev->bar[idx]);
-		}
 
-		ret = check_pt_dev_pio_bars(vdev);
+			ret = check_pt_dev_pio_bars(vdev);
 
-		if (ret == 0) {
-			vdev->flags |= pcidev->type;
-			vdev->bdf.value = pcidev->virt_bdf;
-			/*We should re-add the vdev to hashlist since its vbdf has changed */
-			hlist_del(&vdev->link);
-			hlist_add_head(&vdev->link, &vpci->vdevs_hlist_heads[hash64(vdev->bdf.value, VDEV_LIST_HASHBITS)]);
-			vdev->parent_user = vdev_in_service_vm;
-			vdev_in_service_vm->user = vdev;
+			if (ret == 0) {
+				vdev->flags |= pcidev->type;
+				vdev->bdf.value = pcidev->virt_bdf;
+				/*We should re-add the vdev to hashlist since its vbdf has changed */
+				hlist_del(&vdev->link);
+				hlist_add_head(&vdev->link, &vpci->vdevs_hlist_heads[hash64(vdev->bdf.value, VDEV_LIST_HASHBITS)]);
+				vdev->parent_user = vdev_in_service_vm;
+				vdev_in_service_vm->user = vdev;
+			} else {
+				vdev->vdev_ops->deinit_vdev(vdev);
+				vdev_in_service_vm->vdev_ops->init_vdev(vdev_in_service_vm);
+			}
 		} else {
-			vdev->vdev_ops->deinit_vdev(vdev);
-			vdev_in_service_vm->vdev_ops->init_vdev(vdev_in_service_vm);
+			pr_fatal("%s, Failed to initialize PCI device %x:%x.%x for vm [%d]\n", __func__,
+				pcidev->phys_bdf >> 8U, (pcidev->phys_bdf >> 3U) & 0x1fU, pcidev->phys_bdf & 0x7U,
+				tgt_vm->vm_id);
+			ret = -EFAULT;
 		}
 		spinlock_release(&tgt_vm->vpci.lock);
 	} else {
@@ -801,15 +835,19 @@ int32_t vpci_deassign_pcidev(struct acrn_vm *tgt_vm, struct acrn_pcidev *pcidev)
 {
 	int32_t ret = 0;
 	struct pci_vdev *parent_vdev, *vdev;
+	struct acrn_vpci *vpci;
 	union pci_bdf bdf;
 
 	bdf.value = pcidev->virt_bdf;
 	vdev = pci_find_vdev(&tgt_vm->vpci, bdf);
 	if ((vdev != NULL) && (vdev->user == vdev) && (vdev->pdev != NULL) &&
 			(vdev->pdev->bdf.value == pcidev->phys_bdf)) {
+		vpci = vdev->vpci;
 		parent_vdev = vdev->parent_user;
 
-		vdev->vdev_ops->deinit_vdev(vdev);
+		spinlock_obtain(&vpci->lock);
+		vpci_deinit_vdev(vdev);
+		spinlock_release(&vpci->lock);
 
 		if (parent_vdev != NULL) {
 			spinlock_obtain(&parent_vdev->vpci->lock);

hypervisor/dm/vpci/vroot_port.c

@@ -116,6 +116,7 @@ static void init_ptm(struct pci_vdev *vdev, struct vrp_config *vrp_config)
 
 int32_t create_vrp(struct acrn_vm *vm, struct acrn_vdev *dev)
 {
+	int32_t ret = 0;
 	struct acrn_vm_config *vm_config = get_vm_config(vm->vm_id);
 	struct acrn_vm_pci_dev_config *dev_config = NULL;
 	struct pci_vdev *vdev;
@@ -140,6 +141,11 @@ int32_t create_vrp(struct acrn_vm *vm, struct acrn_vdev *dev)
 			dev_config->vdev_ops = &vrp_ops;
 
 			vdev = vpci_init_vdev(&vm->vpci, dev_config, NULL);
+			if (vdev == NULL) {
+				pr_err("%s: failed to create virtual root port\n", __func__);
+				ret = -EFAULT;
+				break;
+			}
 
 			init_ptm(vdev, vrp_config);
 
@@ -147,7 +153,7 @@ int32_t create_vrp(struct acrn_vm *vm, struct acrn_vdev *dev)
 		}
 	}
 
-	return 0;
+	return ret;
 }
 
 int32_t destroy_vrp(__unused struct pci_vdev *vdev)

hypervisor/include/dm/vpci.h

@@ -30,6 +30,7 @@
 #define VPCI_H_
 
 #include <asm/lib/spinlock.h>
+#include <lib/util.h>
 #include <pci.h>
 #include <list.h>
 
@@ -109,6 +110,7 @@ struct pci_vdev_ops {
 };
 
 struct pci_vdev {
+	uint32_t id;
 	struct acrn_vpci *vpci;
 	/* The bus/device/function triple of the virtual PCI device. */
 	union pci_bdf bdf;
@@ -173,10 +175,10 @@ struct acrn_vpci {
 	spinlock_t lock;
 	union pci_cfg_addr_reg addr;
 	struct pci_mmcfg_region pci_mmcfg;
-	uint32_t pci_vdev_cnt;
 	struct pci_mmio_res res32; 	/* 32-bit mmio start/end address */
 	struct pci_mmio_res res64; 	/* 64-bit mmio start/end address */
 	struct pci_vdev pci_vdevs[CONFIG_MAX_PCI_DEV_NUM];
+	uint64_t vdev_bitmaps[INT_DIV_ROUNDUP(CONFIG_MAX_PCI_DEV_NUM, 64U)];
 	struct hlist_head vdevs_hlist_heads [VDEV_LIST_HASHSIZE];
 };