From: Yifan Liu <yifan1.liu@...>

When passing through PCI devices we have an option "no_reset",
which, if specified, does not do device reset before passthrough.
However this option does not take effect when device is deinit-ed.

This patch makes no_reset to be effective in passthru_deinit also.

Signed-off-by: Yifan Liu <yifan1.liu@...>
---
devicemodel/hw/pci/passthrough.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/devicemodel/hw/pci/passthrough.c b/devicemodel/hw/pci/passthrough.c
index 01c2cb7e2..055b2f111 100644
--- a/devicemodel/hw/pci/passthrough.c
+++ b/devicemodel/hw/pci/passthrough.c
@@ -788,6 +788,7 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
struct acrn_pcidev pcidev = {};
uint16_t phys_bdf = 0;
char reset_path[60];
+ bool need_reset = true;
int fd;

if (!dev->arg) {
@@ -799,6 +800,8 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)

ptdev = (struct passthru_dev *) dev->arg;

+ need_reset = ptdev->need_reset;
+
pr_info("vm_reset_ptdev_intx:0x%x-%x, ioapic virpin=%d.\n",
virt_bdf, ptdev->phys_bdf, dev->lintr.ioapic_irq);
if (dev->lintr.pin != 0) {
@@ -824,7 +827,7 @@ passthru_deinit(struct vmctx *ctx, struct pci_vdev *dev, char *opts)
*/
vm_deassign_pcidev(ctx, &pcidev);
}
- if (!is_rtvm && phys_bdf) {
+ if (!is_rtvm && phys_bdf && need_reset) {
memset(reset_path, 0, sizeof(reset_path));
snprintf(reset_path, 40,
"/sys/bus/pci/devices/0000:%02x:%02x.%x/reset",
--
2.25.1

Starting from elementpath 4.0.0, the XPath2Parser class no longer has a
SYMBOLS member and completeness checking based on that are removed as
well. This patch updates the elementpath_overlay in the config tools to fix
the compilation issue when using recent elementpath versions.

Signed-off-by: Junjie Mao <junjie.mao@...>
---
.../scenario_config/elementpath_overlay.py | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/misc/config_tools/scenario_config/elementpath_overlay.py b/misc/config_tools/scenario_config/elementpath_overlay.py
index 0010fe496..e64f7c2fd 100644
--- a/misc/config_tools/scenario_config/elementpath_overlay.py
+++ b/misc/config_tools/scenario_config/elementpath_overlay.py
@@ -21,16 +21,17 @@ import library.rdt as rdt
BaseParser = elementpath.XPath2Parser

class CustomParser(BaseParser):
- SYMBOLS = BaseParser.SYMBOLS | {
- # Bit-wise operations
- 'bitwise-and',
+ if hasattr(BaseParser, "SYMBOLS"):
+ SYMBOLS = BaseParser.SYMBOLS | {
+ # Bit-wise operations
+ 'bitwise-and',

- 'bits-of',
- 'has',
- 'duplicate-values',
+ 'bits-of',
+ 'has',
+ 'duplicate-values',

- 'number-of-clos-id-needed',
- }
+ 'number-of-clos-id-needed',
+ }

method = CustomParser.method
function = CustomParser.function
--
2.30.2

When assigning a physical interrupt to a Post-launched VM, if it has
been assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 43 ++++++++++---------
hypervisor/common/hypercall.c | 3 +-
.../include/arch/x86/asm/guest/assign.h | 8 ++--
3 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..f7e688ba1 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
+ entry = NULL;
} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,15 +404,24 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
-static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ctlr vgsi_ctlr)
+/* deactivate & remove mapping entry of vpin for vm */
+static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t gsi, enum intx_ctlr gsi_ctlr, bool is_phy_gsi)
{
uint32_t phys_irq;
struct ptirq_remapping_info *entry;
struct intr_source intr_src;
- DEFINE_INTX_SID(virt_sid, virt_gsi, vgsi_ctlr);

- entry = find_ptirq_entry(PTDEV_INTR_INTX, &virt_sid, vm);
+ if (is_phy_gsi) {
+ DEFINE_INTX_SID(sid, gsi, INTX_CTLR_IOAPIC);
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &sid, 0);
+ if ((entry != NULL) && (entry->vm != vm)) {
+ entry = NULL;
+ }
+ } else {
+ DEFINE_INTX_SID(sid, gsi, gsi_ctlr);
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &sid, vm);
+ }
+
if (entry != NULL) {
if (is_entry_active(entry)) {
phys_irq = entry->allocated_pirq;
@@ -431,11 +434,11 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
- (vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
+ (entry->virt_sid.intx_id.ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
- entry->vm->vm_id, virt_gsi);
+ entry->vm->vm_id, entry->virt_sid.intx_id.gsi);
}

ptirq_release_entry(entry);
@@ -735,7 +738,7 @@ int32_t ptirq_intx_pin_remap(struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ct
uint32_t phys_gsi = virt_gsi;

remove_intx_remapping(vm, alt_virt_sid.intx_id.gsi,
- alt_virt_sid.intx_id.ctlr);
+ alt_virt_sid.intx_id.ctlr, false);
entry = add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
if (entry == NULL) {
pr_err("%s, add intx remapping failed", __func__);
@@ -806,12 +809,12 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
/*
* @pre vm != NULL
*/
-void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin)
+void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t gsi, bool pic_pin, bool is_phy_gsi)
{
enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;

spinlock_obtain(&ptdev_lock);
- remove_intx_remapping(vm, virt_gsi, vgsi_ctlr);
+ remove_intx_remapping(vm, gsi, vgsi_ctlr, is_phy_gsi);
spinlock_release(&ptdev_lock);
}

@@ -839,6 +842,6 @@ void ptirq_remove_configured_intx_remappings(const struct acrn_vm *vm)
uint16_t i;

for (i = 0; i < vm_config->pt_intx_num; i++) {
- ptirq_remove_intx_remapping(vm, vm_config->pt_intx[i].virt_gsi, false);
+ ptirq_remove_intx_remapping(vm, vm_config->pt_intx[i].virt_gsi, false, false);
}
}
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..5e296f710 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,6 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
+ ptirq_remove_intx_remapping(get_service_vm(), irq.intx.phys_pin, false, true);
ret = ptirq_add_intx_remapping(target_vm, irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
@@ -1067,7 +1068,7 @@ int32_t hcall_reset_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *targ
if ((vdev != NULL) && (vdev->pdev->bdf.value == irq.phys_bdf)) {
if (((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm))) ||
((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount()))) {
- ptirq_remove_intx_remapping(target_vm, irq.intx.virt_pin, irq.intx.pic_pin);
+ ptirq_remove_intx_remapping(target_vm, irq.intx.virt_pin, irq.intx.pic_pin, false);
ret = 0;
} else {
pr_err("%s: Invalid virt pin\n", __func__);
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..e601dcef6 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -112,18 +112,20 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
/**
* @brief Remove an interrupt remapping entry for INTx.
*
- * Deactivate & remove mapping entry of the given virt_pin for given vm.
+ * Deactivate & remove mapping entry of the given virt gsi for given vm or
+ * phys gsi assigned to this vm.
*
* @param[in] vm pointer to acrn_vm
- * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] gsi virtual gsi number or physical gsi number associated with the passthrough device
* @param[in] pic_pin true for pic, false for ioapic
+ * @param[in] is_phy_gsi true if gsi is physical, false if gsi is virtual
*
* @return None
*
* @pre vm != NULL
*
*/
-void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin);
+void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t gsi, bool pic_pin, bool is_phy_gsi);

/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
--
2.30.2

ptirq_remapping_info records which physical interrupt is mapped to
the virtual interrupt in a VM. As we need to knonw whether a physical
sid has been mapped to a VM and whether a virtual sid in a VM has been
used, there should be two hash tables to link and iterate
ptirq_remapping_info:
- One is used to lookup from physical sid, linking phys_link.
- The other is used to lookup from virtual sid in a VM, linking virt_link

Without this patch, phys_link or virt_link from different
ptirq_remapping_info was linked by one hash list head if they got the
same hash value, as shown in following diagram.
When looking for a ptirq_remapping_info from physical sid, the original
code took all hash list node as phys_link and failed to get
ptirq_remapping_info linked with virt_link and later references
to its members are wrong.
The same problem also occurred when looking for a ptirq_remapping_info
from virtual sid and vm.

---------- <- hash table
|hlist_head| --actual ptirq_remapping_info address
---------- --------- / --used as ptirq_remapping_info
| ... | |phys_link| ___/
---------- --------- --------- / ---------
|hlist_head| -> |phys_link| <-> |virt_link| <-> |phys_link|
---------- --------- --------- ---------
| ... | |virt_link| | other | |virt_link|
---------- --------- | members | ---------
| other | --------- | other |
| members | | members |
--------- ---------

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/common/ptdev.c | 49 +++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..f2f8633f4 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,10 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+/* lookup mapping info from phyical sid, hashing from sid + vm address(0) */
+static struct hlist_head phys_sid_htable[PTIRQ_ENTRY_HASHSIZE];
+/* lookup mapping info from virtual sid within a vm, hashing from sid + vm address */
+static struct hlist_head virt_sid_htable[PTIRQ_ENTRY_HASHSIZE];

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +41,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy sid or virtual sid in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ if (vm == NULL) {
+ b = &(phys_sid_htable[key]);
+
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value)) {
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &(virt_sid_htable[key]);
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value) && (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +221,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry, uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phys_sid_htable[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(virt_sid_htable[key]));
}

return ret;
--
2.30.2

This series fixes hypervisor side PCIe INTx passthru issue occurred
when passthru USB xDCI, SMbus controllers and IPU to Post-launched VM.

The first patch fixes the hash table implementation used to look up
`ptirq_remapping_info` from physical interrupt or virtual interrupt
in a VM.

The second patch fixes wrong IOAPIC and RTE states when assigning an
INTx interrupt to Post-launched VM.

v5:
- Fix possible NULL pointer deference in remove_intx_remapping in second
patch

v4:
- Rename hash table structures in first patch
- Add a parameter to ptirq_remove_intx_remapping to remove physical gsi
assigned to a VM
- Move removal and addition logic of INTx remapping to hypercall handler.

v3:
- Split into two patches.
- Fix the naming GSI to SID as it's common for both INTx and MSI
- Refine commit log in the first patch

v2:
- Add ptirq_reassign_intx_remapping to do intx reassignment from ServiceVM
to Postlaunched VM, instead of patching the logic in add_intx_remapping.

Qiang Zhang (2):
ptirq: Fix ptirq hash tables
ptirq: Fix INTx assignment for Post-launched VM

hypervisor/arch/x86/guest/assign.c | 43 ++++++++--------
hypervisor/common/hypercall.c | 3 +-
hypervisor/common/ptdev.c | 49 +++++++++++--------
.../include/arch/x86/asm/guest/assign.h | 8 +--
4 files changed, 59 insertions(+), 44 deletions(-)

--
2.30.2

When assigning a physical interrupt to a Post-launched VM, if it has
been assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 42 ++++++++++---------
hypervisor/common/hypercall.c | 3 +-
.../include/arch/x86/asm/guest/assign.h | 8 ++--
3 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..b6b098064 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
+ entry = NULL;
} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,15 +404,23 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
-static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ctlr vgsi_ctlr)
+/* deactivate & remove mapping entry of vpin for vm */
+static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t gsi, enum intx_ctlr gsi_ctlr, bool is_phy_gsi)
{
uint32_t phys_irq;
struct ptirq_remapping_info *entry;
struct intr_source intr_src;
- DEFINE_INTX_SID(virt_sid, virt_gsi, vgsi_ctlr);

- entry = find_ptirq_entry(PTDEV_INTR_INTX, &virt_sid, vm);
+ if (is_phy_gsi) {
+ DEFINE_INTX_SID(sid, gsi, INTX_CTLR_IOAPIC);
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &sid, 0);
+ if (entry->vm != vm)
+ entry = NULL;
+ } else {
+ DEFINE_INTX_SID(sid, gsi, gsi_ctlr);
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &sid, vm);
+ }
+
if (entry != NULL) {
if (is_entry_active(entry)) {
phys_irq = entry->allocated_pirq;
@@ -431,11 +433,11 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
- (vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
+ (entry->virt_sid.intx_id.ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
- entry->vm->vm_id, virt_gsi);
+ entry->vm->vm_id, entry->virt_sid.intx_id.gsi);
}

ptirq_release_entry(entry);
@@ -735,7 +737,7 @@ int32_t ptirq_intx_pin_remap(struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ct
uint32_t phys_gsi = virt_gsi;

remove_intx_remapping(vm, alt_virt_sid.intx_id.gsi,
- alt_virt_sid.intx_id.ctlr);
+ alt_virt_sid.intx_id.ctlr, false);
entry = add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
if (entry == NULL) {
pr_err("%s, add intx remapping failed", __func__);
@@ -806,12 +808,12 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
/*
* @pre vm != NULL
*/
-void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin)
+void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t gsi, bool pic_pin, bool is_phy_gsi)
{
enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;

spinlock_obtain(&ptdev_lock);
- remove_intx_remapping(vm, virt_gsi, vgsi_ctlr);
+ remove_intx_remapping(vm, gsi, vgsi_ctlr, is_phy_gsi);
spinlock_release(&ptdev_lock);
}

@@ -839,6 +841,6 @@ void ptirq_remove_configured_intx_remappings(const struct acrn_vm *vm)
uint16_t i;

for (i = 0; i < vm_config->pt_intx_num; i++) {
- ptirq_remove_intx_remapping(vm, vm_config->pt_intx[i].virt_gsi, false);
+ ptirq_remove_intx_remapping(vm, vm_config->pt_intx[i].virt_gsi, false, false);
}
}
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..5e296f710 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,6 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
+ ptirq_remove_intx_remapping(get_service_vm(), irq.intx.phys_pin, false, true);
ret = ptirq_add_intx_remapping(target_vm, irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
@@ -1067,7 +1068,7 @@ int32_t hcall_reset_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *targ
if ((vdev != NULL) && (vdev->pdev->bdf.value == irq.phys_bdf)) {
if (((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm))) ||
((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount()))) {
- ptirq_remove_intx_remapping(target_vm, irq.intx.virt_pin, irq.intx.pic_pin);
+ ptirq_remove_intx_remapping(target_vm, irq.intx.virt_pin, irq.intx.pic_pin, false);
ret = 0;
} else {
pr_err("%s: Invalid virt pin\n", __func__);
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..e601dcef6 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -112,18 +112,20 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
/**
* @brief Remove an interrupt remapping entry for INTx.
*
- * Deactivate & remove mapping entry of the given virt_pin for given vm.
+ * Deactivate & remove mapping entry of the given virt gsi for given vm or
+ * phys gsi assigned to this vm.
*
* @param[in] vm pointer to acrn_vm
- * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] gsi virtual gsi number or physical gsi number associated with the passthrough device
* @param[in] pic_pin true for pic, false for ioapic
+ * @param[in] is_phy_gsi true if gsi is physical, false if gsi is virtual
*
* @return None
*
* @pre vm != NULL
*
*/
-void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin);
+void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t gsi, bool pic_pin, bool is_phy_gsi);

/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
--
2.30.2

ptirq_remapping_info records which physical interrupt is mapped to
the virtual interrupt in a VM. As we need to knonw whether a physical
sid has been mapped to a VM and whether a virtual sid in a VM has been
used, there should be two hash tables to link and iterate
ptirq_remapping_info:
- One is used to lookup from physical sid, linking phys_link.
- The other is used to lookup from virtual sid in a VM, linking virt_link

Without this patch, phys_link or virt_link from different
ptirq_remapping_info was linked by one hash list head if they got the
same hash value, as shown in following diagram.
When looking for a ptirq_remapping_info from physical sid, the original
code took all hash list node as phys_link and failed to get
ptirq_remapping_info linked with virt_link and later references
to its members are wrong.
The same problem also occurred when looking for a ptirq_remapping_info
from virtual sid and vm.

---------- <- hash table
|hlist_head| --actual ptirq_remapping_info address
---------- --------- / --used as ptirq_remapping_info
| ... | |phys_link| ___/
---------- --------- --------- / ---------
|hlist_head| -> |phys_link| <-> |virt_link| <-> |phys_link|
---------- --------- --------- ---------
| ... | |virt_link| | other | |virt_link|
---------- --------- | members | ---------
| other | --------- | other |
| members | | members |
--------- ---------

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/common/ptdev.c | 49 +++++++++++++++++++++++----------------
1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..f2f8633f4 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,10 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+/* lookup mapping info from phyical sid, hashing from sid + vm address(0) */
+static struct hlist_head phys_sid_htable[PTIRQ_ENTRY_HASHSIZE];
+/* lookup mapping info from virtual sid within a vm, hashing from sid + vm address */
+static struct hlist_head virt_sid_htable[PTIRQ_ENTRY_HASHSIZE];

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +41,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy sid or virtual sid in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ if (vm == NULL) {
+ b = &(phys_sid_htable[key]);
+
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value)) {
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &(virt_sid_htable[key]);
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value) && (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +221,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry, uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phys_sid_htable[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(virt_sid_htable[key]));
}

return ret;
--
2.30.2

This series fixes hypervisor side PCIe INTx passthru issue occurred
when passthru USB xDCI, SMbus controllers and IPU to Post-launched VM.

The first patch fixes the hash table implementation used to look up
`ptirq_remapping_info` from physical interrupt or virtual interrupt
in a VM.

The second patch fixes wrong IOAPIC and RTE states when assigning an
INTx interrupt to Post-launched VM.

v4:
- Rename hash table structures in first patch
- Add a parameter to ptirq_remove_intx_remapping to remove physical gsi
assigned to a VM
- Move removal and addition logic of INTx remapping to hypercall handler.

v3:
- Split into two patches.
- Fix the naming GSI to SID as it's common for both INTx and MSI
- Refine commit log in the first patch

v2:
- Add ptirq_reassign_intx_remapping to do intx reassignment from ServiceVM
to Postlaunched VM, instead of patching the logic in add_intx_remapping.

Qiang Zhang (2):
ptirq: Fix ptirq hash tables
ptirq: Fix INTx assignment for Post-launched VM

hypervisor/arch/x86/guest/assign.c | 42 ++++++++--------
hypervisor/common/hypercall.c | 3 +-
hypervisor/common/ptdev.c | 49 +++++++++++--------
.../include/arch/x86/asm/guest/assign.h | 8 +--
4 files changed, 58 insertions(+), 44 deletions(-)

--
2.30.2

On Tue, Feb 28, 2023 at 04:47:55PM +0800, Fei Li wrote:

On 2023-02-27 at 20:34:52 +0800, Qiang Zhang wrote:

When assigning a physical interrupt to a Post-launched VM, if it has
been assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 41 ++++++++++++++-----
hypervisor/common/hypercall.c | 2 +-
.../include/arch/x86/asm/guest/assign.h | 21 ++++++++++
3 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..0baa9404b 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
+ entry = NULL;
} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,7 +404,7 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
+/* deactivate & remove mapping entry of vpin for vm */
static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ctlr vgsi_ctlr)
{
uint32_t phys_irq;
@@ -431,7 +425,7 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
(vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
@@ -442,6 +436,19 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e
}
}

+static struct ptirq_remapping_info *reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi,
+ uint32_t phys_gsi, enum intx_ctlr vgsi_ctlr)
+{
+ struct ptirq_remapping_info *entry = NULL;
+ DEFINE_INTX_SID(phys_sid, phys_gsi, INTX_CTLR_IOAPIC);
+
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &phys_sid, NULL);
+ if (entry)

This means you could re-assign a ptirq from pre-launched/Service/post-launched VM to any other VM ?

How about move the check/remove/add logic to hypercall.c ?

+ remove_intx_remapping(entry->vm, entry->virt_sid.intx_id.gsi, entry->virt_sid.intx_id.ctlr);
+
+ return add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+}
+
static void ptirq_handle_intx(struct acrn_vm *vm,
const struct ptirq_remapping_info *entry)
{
@@ -815,6 +822,18 @@ void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bo
spinlock_release(&ptdev_lock);
}

+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t phys_gsi, bool pic_pin)
+{
+ struct ptirq_remapping_info *entry;
+ enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;
+
+ spinlock_obtain(&ptdev_lock);
+ entry = reassign_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+ spinlock_release(&ptdev_lock);
+
+ return (entry != NULL) ? 0 : -ENODEV;
+}
+
/*
* @pre vm != NULL
*/
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..faec16545 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,7 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
- ret = ptirq_add_intx_remapping(target_vm, irq.intx.virt_pin,
+ ret = ptirq_reassign_intx_remapping(target_vm, irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
pr_err("%s: Invalid phys pin or virt pin\n", __func__);
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..1387b9825 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -125,6 +125,27 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
*/
void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin);

+/**
+ * @brief Assign a INTx to Postlaunched VM.
+ *
+ * If the physical pin was assigned to another VM, remove it first.
+ * Then assign the physical pin to target virtual VM.
+ *
+ * @param[in] vm pointer to acrn_vm
+ * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] phys_gsi physical pin number associated with the passthrough device
+ * @param[in] pic_pin true for pic, false for ioapic
+ *
+ * @return
+ * - 0: on success
+ * - \p -EINVAL: invalid virt_pin value
+ * - \p -ENODEV: failed to add the remapping entry
+ *
+ * @pre vm != NULL
+ *
+ */
+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t phys_gsi, bool pic_pin);
+
/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
*
--
2.30.2

On Tue, Feb 28, 2023 at 04:45:13PM +0800, Fei Li wrote:

On 2023-02-27 at 20:34:51 +0800, Qiang Zhang wrote:

ptirq_remapping_info records which physical interrupt is mapped to
the virtual interrupt in a VM. As we need to knonw whether a physical
sid has been mapped to a VM and whether a virtual sid in a VM has been

whether a physical IRQ (INTx or MSI/MSI-X) has been assigned to a VM and
Which a virtual IRQ been mapped to this physical IRQ in a VM ?

used, there should be two hash tables to link and iterate
ptirq_remapping_info:
- One is used to lookup from physical sid, linking phys_link.
- The other is used to lookup from virtual sid in a VM, linking virt_link

Without this patch, phys_link or virt_link from different
ptirq_remapping_info was linked by one hash list head if they got the
same hash value, as shown in following diagram.
When looking for a ptirq_remapping_info from physical sid, the original
code took all hash list node as phys_link and failed to get
ptirq_remapping_info linked with virt_link and later references
to its members are wrong.
The same problem also occurred when looking for a ptirq_remapping_info
from virtual sid and vm.

--actual ptirq_remapping_info address
--------- / --used as ptirq_remapping_info
|phys_link| ___/
---------- --------- --------- / ---------
|hlist_head| -> |phys_link| <-> |virt_link| <-> |phys_link|
---------- --------- --------- ---------
|virt_link| | other | |virt_link|
--------- | members | ---------
| other | --------- | other |
| members | | members |
--------- ---------

Maybe it's better to show all the hlist heads in this picture.

Will add.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/common/ptdev.c | 53 ++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..04b1f6531 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,14 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+struct ptirq_hash_table {
+ struct hlist_head buckets[PTIRQ_ENTRY_HASHSIZE];
+};
+
+/* lookup mapping info from phyical sid, hashing from sid + vm address(0) */
+static struct ptirq_hash_table phy_sid_htable;
+/* lookup mapping info from virtual sid within a vm, hashing from sid vm address */
+static struct ptirq_hash_table vm_virt_sid_htable;

virt already means for VM.

Will rename vm_virt_sid_htable to virt_sid_htable

Besides, it seems better to remove the encapsulation struct and define
hash tables in this common style:

struct hlist_head virt_sid_htable[PTIRQ_ENTRY_HASHSIZE];
struct hlist_head phys_sid_htable[PTIRQ_ENTRY_HASHSIZE];

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +45,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy sid or virtual sid in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);
+
+ if (vm == NULL) {
+ b = &phy_sid_htable.buckets[key];

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value)) {
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &vm_virt_sid_htable.buckets[key];
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value) && (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +225,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry, uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phy_sid_htable.buckets[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(vm_virt_sid_htable.buckets[key]));
}

return ret;
--
2.30.2

On 2023-02-27 at 20:34:52 +0800, Qiang Zhang wrote:

When assigning a physical interrupt to a Post-launched VM, if it has
been assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 41 ++++++++++++++-----
hypervisor/common/hypercall.c | 2 +-
.../include/arch/x86/asm/guest/assign.h | 21 ++++++++++
3 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..0baa9404b 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
+ entry = NULL;
} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,7 +404,7 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
+/* deactivate & remove mapping entry of vpin for vm */
static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ctlr vgsi_ctlr)
{
uint32_t phys_irq;
@@ -431,7 +425,7 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
(vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
@@ -442,6 +436,19 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e
}
}

+static struct ptirq_remapping_info *reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi,
+ uint32_t phys_gsi, enum intx_ctlr vgsi_ctlr)
+{
+ struct ptirq_remapping_info *entry = NULL;
+ DEFINE_INTX_SID(phys_sid, phys_gsi, INTX_CTLR_IOAPIC);
+
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &phys_sid, NULL);
+ if (entry)

This means you could re-assign a ptirq from pre-launched/Service/post-launched VM to any other VM ?

+ remove_intx_remapping(entry->vm, entry->virt_sid.intx_id.gsi, entry->virt_sid.intx_id.ctlr);
+
+ return add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+}
+
static void ptirq_handle_intx(struct acrn_vm *vm,
const struct ptirq_remapping_info *entry)
{
@@ -815,6 +822,18 @@ void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bo
spinlock_release(&ptdev_lock);
}

+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t phys_gsi, bool pic_pin)
+{
+ struct ptirq_remapping_info *entry;
+ enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;
+
+ spinlock_obtain(&ptdev_lock);
+ entry = reassign_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+ spinlock_release(&ptdev_lock);
+
+ return (entry != NULL) ? 0 : -ENODEV;
+}
+
/*
* @pre vm != NULL
*/
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..faec16545 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,7 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
- ret = ptirq_add_intx_remapping(target_vm, irq.intx.virt_pin,
+ ret = ptirq_reassign_intx_remapping(target_vm, irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
pr_err("%s: Invalid phys pin or virt pin\n", __func__);
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..1387b9825 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -125,6 +125,27 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
*/
void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin);

+/**
+ * @brief Assign a INTx to Postlaunched VM.
+ *
+ * If the physical pin was assigned to another VM, remove it first.
+ * Then assign the physical pin to target virtual VM.
+ *
+ * @param[in] vm pointer to acrn_vm
+ * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] phys_gsi physical pin number associated with the passthrough device
+ * @param[in] pic_pin true for pic, false for ioapic
+ *
+ * @return
+ * - 0: on success
+ * - \p -EINVAL: invalid virt_pin value
+ * - \p -ENODEV: failed to add the remapping entry
+ *
+ * @pre vm != NULL
+ *
+ */
+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t phys_gsi, bool pic_pin);
+
/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
*
--
2.30.2

On 2023-02-27 at 20:34:51 +0800, Qiang Zhang wrote:

ptirq_remapping_info records which physical interrupt is mapped to
the virtual interrupt in a VM. As we need to knonw whether a physical
sid has been mapped to a VM and whether a virtual sid in a VM has been

whether a physical IRQ (INTx or MSI/MSI-X) has been assigned to a VM and
Which a virtual IRQ been mapped to this physical IRQ in a VM ?

used, there should be two hash tables to link and iterate
ptirq_remapping_info:
- One is used to lookup from physical sid, linking phys_link.
- The other is used to lookup from virtual sid in a VM, linking virt_link

Without this patch, phys_link or virt_link from different
ptirq_remapping_info was linked by one hash list head if they got the
same hash value, as shown in following diagram.
When looking for a ptirq_remapping_info from physical sid, the original
code took all hash list node as phys_link and failed to get
ptirq_remapping_info linked with virt_link and later references
to its members are wrong.
The same problem also occurred when looking for a ptirq_remapping_info
from virtual sid and vm.

--actual ptirq_remapping_info address
--------- / --used as ptirq_remapping_info
|phys_link| ___/
---------- --------- --------- / ---------
|hlist_head| -> |phys_link| <-> |virt_link| <-> |phys_link|
---------- --------- --------- ---------
|virt_link| | other | |virt_link|
--------- | members | ---------
| other | --------- | other |
| members | | members |
--------- ---------

Maybe it's better to show all the hlist heads in this picture.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/common/ptdev.c | 53 ++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..04b1f6531 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,14 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+struct ptirq_hash_table {
+ struct hlist_head buckets[PTIRQ_ENTRY_HASHSIZE];
+};
+
+/* lookup mapping info from phyical sid, hashing from sid + vm address(0) */
+static struct ptirq_hash_table phy_sid_htable;
+/* lookup mapping info from virtual sid within a vm, hashing from sid vm address */
+static struct ptirq_hash_table vm_virt_sid_htable;

virt already means for VM.

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +45,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy sid or virtual sid in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);
+
+ if (vm == NULL) {
+ b = &phy_sid_htable.buckets[key];

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value)) {
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &vm_virt_sid_htable.buckets[key];
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value) && (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +225,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry, uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phy_sid_htable.buckets[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(vm_virt_sid_htable.buckets[key]));
}

return ret;
--
2.30.2

When assigning a physical interrupt to a Post-launched VM, if it has
been assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 41 ++++++++++++++-----
hypervisor/common/hypercall.c | 2 +-
.../include/arch/x86/asm/guest/assign.h | 21 ++++++++++
3 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..0baa9404b 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id, virt_gsi);
+ entry = NULL;
} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,7 +404,7 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm *vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
+/* deactivate & remove mapping entry of vpin for vm */
static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum intx_ctlr vgsi_ctlr)
{
uint32_t phys_irq;
@@ -431,7 +425,7 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
(vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
@@ -442,6 +436,19 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, e
}
}

+static struct ptirq_remapping_info *reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi,
+ uint32_t phys_gsi, enum intx_ctlr vgsi_ctlr)
+{
+ struct ptirq_remapping_info *entry = NULL;
+ DEFINE_INTX_SID(phys_sid, phys_gsi, INTX_CTLR_IOAPIC);
+
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &phys_sid, NULL);
+ if (entry)
+ remove_intx_remapping(entry->vm, entry->virt_sid.intx_id.gsi, entry->virt_sid.intx_id.ctlr);
+
+ return add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+}
+
static void ptirq_handle_intx(struct acrn_vm *vm,
const struct ptirq_remapping_info *entry)
{
@@ -815,6 +822,18 @@ void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bo
spinlock_release(&ptdev_lock);
}

+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t phys_gsi, bool pic_pin)
+{
+ struct ptirq_remapping_info *entry;
+ enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;
+
+ spinlock_obtain(&ptdev_lock);
+ entry = reassign_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+ spinlock_release(&ptdev_lock);
+
+ return (entry != NULL) ? 0 : -ENODEV;
+}
+
/*
* @pre vm != NULL
*/
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..faec16545 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,7 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin < get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin < vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
- ret = ptirq_add_intx_remapping(target_vm, irq.intx.virt_pin,
+ ret = ptirq_reassign_intx_remapping(target_vm, irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
pr_err("%s: Invalid phys pin or virt pin\n", __func__);
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..1387b9825 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -125,6 +125,27 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
*/
void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool pic_pin);

+/**
+ * @brief Assign a INTx to Postlaunched VM.
+ *
+ * If the physical pin was assigned to another VM, remove it first.
+ * Then assign the physical pin to target virtual VM.
+ *
+ * @param[in] vm pointer to acrn_vm
+ * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] phys_gsi physical pin number associated with the passthrough device
+ * @param[in] pic_pin true for pic, false for ioapic
+ *
+ * @return
+ * - 0: on success
+ * - \p -EINVAL: invalid virt_pin value
+ * - \p -ENODEV: failed to add the remapping entry
+ *
+ * @pre vm != NULL
+ *
+ */
+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t phys_gsi, bool pic_pin);
+
/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
*
--
2.30.2

ptirq_remapping_info records which physical interrupt is mapped to
the virtual interrupt in a VM. As we need to knonw whether a physical
sid has been mapped to a VM and whether a virtual sid in a VM has been
used, there should be two hash tables to link and iterate
ptirq_remapping_info:
- One is used to lookup from physical sid, linking phys_link.
- The other is used to lookup from virtual sid in a VM, linking virt_link

Without this patch, phys_link or virt_link from different
ptirq_remapping_info was linked by one hash list head if they got the
same hash value, as shown in following diagram.
When looking for a ptirq_remapping_info from physical sid, the original
code took all hash list node as phys_link and failed to get
ptirq_remapping_info linked with virt_link and later references
to its members are wrong.
The same problem also occurred when looking for a ptirq_remapping_info
from virtual sid and vm.

--actual ptirq_remapping_info address
--------- / --used as ptirq_remapping_info
|phys_link| ___/
---------- --------- --------- / ---------
|hlist_head| -> |phys_link| <-> |virt_link| <-> |phys_link|
---------- --------- --------- ---------
|virt_link| | other | |virt_link|
--------- | members | ---------
| other | --------- | other |
| members | | members |
--------- ---------

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/common/ptdev.c | 53 ++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..04b1f6531 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,14 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+struct ptirq_hash_table {
+ struct hlist_head buckets[PTIRQ_ENTRY_HASHSIZE];
+};
+
+/* lookup mapping info from phyical sid, hashing from sid + vm address(0) */
+static struct ptirq_hash_table phy_sid_htable;
+/* lookup mapping info from virtual sid within a vm, hashing from sid vm address */
+static struct ptirq_hash_table vm_virt_sid_htable;

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +45,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy sid or virtual sid in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);
+
+ if (vm == NULL) {
+ b = &phy_sid_htable.buckets[key];

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value)) {
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &vm_virt_sid_htable.buckets[key];
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value) && (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +225,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry, uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phy_sid_htable.buckets[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(vm_virt_sid_htable.buckets[key]));
}

return ret;
--
2.30.2

This series fixes hypervisor side PCIe INTx passthru issue occurred
when passthru USB xDCI, SMbus controllers and IPU to Post-launched VM.

The first patch fixes the hash table implementation used to look up
`ptirq_remapping_info` from physical sid or virtual sid in a VM.

The second patch fixes wrong IOAPIC and RTE states when assigning an
INTx interrupt to Post-launched VM.

Qiang

v3:
- Split into two patches.
- Fix the naming GSI to SID as it's common for both INTx and MSI
- Refine commit log in the first patch
v2:
- Add ptirq_reassign_intx_remapping to do intx reassignment from ServiceVM
to Postlaunched VM, instead of patching the logic in add_intx_remapping.

Qiang Zhang (2):
ptirq: Fix ptirq hash tables
ptirq: Fix INTx assignment for Post-launched VM

hypervisor/arch/x86/guest/assign.c | 41 ++++++++++----
hypervisor/common/hypercall.c | 2 +-
hypervisor/common/ptdev.c | 53 ++++++++++++-------
.../include/arch/x86/asm/guest/assign.h | 21 ++++++++
4 files changed, 85 insertions(+), 32 deletions(-)

--
2.30.2

On Mon, Feb 27, 2023 at 07:09:09AM +0000, Mao, Junjie wrote:

-----Original Message-----
From: Qiang Zhang <qiang4.zhang@...>
Sent: Monday, February 27, 2023 3:06 PM
To: Mao, Junjie <junjie.mao@...>
Cc: acrn-dev@...; Li, Fei1 <fei1.li@...>
Subject: Re: [PATCH v2 1/1] ptirq: Fix ptirq hash tables and uos intx assignment

On Mon, Feb 27, 2023 at 05:36:31AM +0000, Mao, Junjie wrote:

-----Original Message-----
From: Qiang Zhang <qiang4.zhang@...>
Sent: Monday, February 27, 2023 11:35 AM
To: acrn-dev@...
Cc: Mao, Junjie <junjie.mao@...>; Li, Fei1 <fei1.li@...>; Qiang Zhang
<qiang4.zhang@...>
Subject: [PATCH v2 1/1] ptirq: Fix ptirq hash tables and uos intx assignment

ptirq_remapping_info records which pGSI is mapped to the vGSI in a VM.
As we need to knonw whether a pGSI has been assigned to a VM and
whether a vGSI in a VM has been used, there should be two hash
tables to link and iterate ptirq_remapping_info:
- One is used to lookup from pGSI, linking phys_link.
- The other is used to lookup from vGSI in a VM, linking virt_link

When assigning a pGSI to a Post-launched VM, if the pGSI has been
assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM. Add ptirq_reassign_intx_remapping for this.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 41 ++++++++++----
hypervisor/common/hypercall.c | 2 +-
hypervisor/common/ptdev.c | 53 ++++++++++++-------
.../include/arch/x86/asm/guest/assign.h | 21 ++++++++
4 files changed, 85 insertions(+), 32 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..0baa9404b 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct

acrn_vm

*vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d
with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm-

vm_id, virt_gsi);

- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with
vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id,
virt_gsi);
+ entry = NULL;

With this change you also need to replace the call to ptirq_add_intx_remapping() with

your "reassign" version in create_vm() in arch/x86/guest/vm.c. That piece of logic grants
some legacy interrupt lines to a pre-launched VM which may be created before or after the
service VM starts, depending on the processing speed of different cores.

INTx remappings are just added to Pre-launched VM according to static
configurations. In `prepare_vm`, INTx remappings for Pre-launched VMs are
handled before ServiceVM starts. So there is no reassign cases for
Pre-launched VMs.

I don't think we ever guarantee "INTx remappings for Pre-launched VMs are handled before ServiceVM starts". The launch of pre-launched VM(s) and the service VM is simultaneous. While it may be highly probable because INTx remappings for the service VM are established only when it attempts to access its vIOAPIC, there is no 100% guarantee on that.

Have we added any synchronization on VM launch order recently?

I think this synchronization has already existed.

SOS won't got launched until create_vm finishs setting ptirq_remapping_info
for all Prelaunched VMs and loaded_pre_vm_nr reaches PRE_VM_NUM.

```c
int32_t prepare_vm(uint16_t vm_id, struct acrn_vm_config *vm_config)
{
int32_t err = 0;
struct acrn_vm *vm = NULL;

#ifdef CONFIG_SECURITY_VM_FIXUP
security_vm_fixup(vm_id);
#endif
if (get_vmid_by_name(vm_config->name) != vm_id) {
pr_err("Invalid VM name: %s", vm_config->name);
err = -1;
} else {
/* Service VM and pre-launched VMs launch on all pCPUs defined in vm_config->cpu_affinity */
err = create_vm(vm_id, vm_config->cpu_affinity, vm_config, &vm);
}

if (err == 0) {
if (is_prelaunched_vm(vm)) {
build_vrsdp(vm);
}

if (is_service_vm(vm)) {
/* We need to ensure all modules of pre-launched VMs have been loaded already
* before loading Service VM modules, otherwise the module of pre-launched VMs could
* be corrupted because Service VM kernel might pick any usable RAM to extract kernel
* when KASLR enabled.
* In case the pre-launched VMs aren't loaded successfuly that cause deadlock here,
* use a 10000ms timer to break the waiting loop.
*/
uint64_t start_tick = cpu_ticks();

while (loaded_pre_vm_nr != PRE_VM_NUM) {
uint64_t timeout = ticks_to_ms(cpu_ticks() - start_tick);

if (timeout > 10000U) {
pr_err("Loading pre-launched VMs timeout!");
break;
}
}
}

err = prepare_os_image(vm);

if (is_prelaunched_vm(vm)) {
loaded_pre_vm_nr++;
}
}

return err;
}
```

Thanks
Qiang

---
Best Regards
Junjie Mao

Thanks
Qiang

---
Best Regards
Junjie Mao

} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,7 +404,7 @@ static struct ptirq_remapping_info *add_intx_remapping(struct

acrn_vm

*vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
+/* deactivate & remove mapping entry of vpin for vm */
static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum
intx_ctlr vgsi_ctlr)
{
uint32_t phys_irq;
@@ -431,7 +425,7 @@ static void remove_intx_remapping(const struct acrn_vm *vm,

uint32_t

virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
(vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
@@ -442,6 +436,19 @@ static void remove_intx_remapping(const struct acrn_vm *vm,

uint32_t

virt_gsi, e
}
}

+static struct ptirq_remapping_info *reassign_intx_remapping(struct acrn_vm *vm,

uint32_t

virt_gsi,
+ uint32_t phys_gsi, enum intx_ctlr vgsi_ctlr)
+{
+ struct ptirq_remapping_info *entry = NULL;
+ DEFINE_INTX_SID(phys_sid, phys_gsi, INTX_CTLR_IOAPIC);
+
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &phys_sid, NULL);
+ if (entry)
+ remove_intx_remapping(entry->vm, entry->virt_sid.intx_id.gsi, entry-

virt_sid.intx_id.ctlr);

+
+ return add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+}
+
static void ptirq_handle_intx(struct acrn_vm *vm,
const struct ptirq_remapping_info *entry)
{
@@ -815,6 +822,18 @@ void ptirq_remove_intx_remapping(const struct acrn_vm *vm,

uint32_t

virt_gsi, bo
spinlock_release(&ptdev_lock);
}

+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
phys_gsi, bool pic_pin)
+{
+ struct ptirq_remapping_info *entry;
+ enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;
+
+ spinlock_obtain(&ptdev_lock);
+ entry = reassign_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+ spinlock_release(&ptdev_lock);
+
+ return (entry != NULL) ? 0 : -ENODEV;
+}
+
/*
* @pre vm != NULL
*/
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..faec16545 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,7 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct
acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin <
get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin <
vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
- ret = ptirq_add_intx_remapping(target_vm,
irq.intx.virt_pin,
+ ret = ptirq_reassign_intx_remapping(target_vm,
irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
pr_err("%s: Invalid phys pin or virt pin\n", __func__);
diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..6308b7514 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,14 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+struct ptirq_hash_table {
+ struct hlist_head buckets[PTIRQ_ENTRY_HASHSIZE];
+};
+
+/* lookup mapping info from phyical gsi, hashing from gsi + vm address(0) */
+static struct ptirq_hash_table phy_gsi_htable;
+/* lookup mapping info from vgsi within a vm, hashing from vm address + vgsi */
+static struct ptirq_hash_table vm_vgsi_htable;

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +45,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy gsi or vgsi in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);
+
+ if (vm == NULL) {
+ b = &phy_gsi_htable.buckets[key];

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value))
{
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &vm_vgsi_htable.buckets[key];
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value)
&& (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +225,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry,
uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phy_gsi_htable.buckets[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(vm_vgsi_htable.buckets[key]));
}

return ret;
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h
b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..1387b9825 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -125,6 +125,27 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t
virt_gsi, uint32_t
*/
void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool
pic_pin);

+/**
+ * @brief Assign a INTx to Postlaunched VM.
+ *
+ * If the physical pin was assigned to another VM, remove it first.
+ * Then assign the physical pin to target virtual VM.
+ *
+ * @param[in] vm pointer to acrn_vm
+ * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] phys_gsi physical pin number associated with the passthrough device
+ * @param[in] pic_pin true for pic, false for ioapic
+ *
+ * @return
+ * - 0: on success
+ * - \p -EINVAL: invalid virt_pin value
+ * - \p -ENODEV: failed to add the remapping entry
+ *
+ * @pre vm != NULL
+ *
+ */
+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
phys_gsi, bool pic_pin);
+
/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
*
--
2.30.2

On Mon, Feb 27, 2023 at 05:36:31AM +0000, Mao, Junjie wrote:

-----Original Message-----
From: Qiang Zhang <qiang4.zhang@...>
Sent: Monday, February 27, 2023 11:35 AM
To: acrn-dev@...
Cc: Mao, Junjie <junjie.mao@...>; Li, Fei1 <fei1.li@...>; Qiang Zhang
<qiang4.zhang@...>
Subject: [PATCH v2 1/1] ptirq: Fix ptirq hash tables and uos intx assignment

ptirq_remapping_info records which pGSI is mapped to the vGSI in a VM.
As we need to knonw whether a pGSI has been assigned to a VM and
whether a vGSI in a VM has been used, there should be two hash
tables to link and iterate ptirq_remapping_info:
- One is used to lookup from pGSI, linking phys_link.
- The other is used to lookup from vGSI in a VM, linking virt_link

When assigning a pGSI to a Post-launched VM, if the pGSI has been
assigned to ServiceVM, we should remove that mapping first to reset
ioapic pin state and rte, and build new mapping for the Post-launched
VM. Add ptirq_reassign_intx_remapping for this.

Tracked-On: #8370
Signed-off-by: Qiang Zhang <qiang4.zhang@...>
---
hypervisor/arch/x86/guest/assign.c | 41 ++++++++++----
hypervisor/common/hypercall.c | 2 +-
hypervisor/common/ptdev.c | 53 ++++++++++++-------
.../include/arch/x86/asm/guest/assign.h | 21 ++++++++
4 files changed, 85 insertions(+), 32 deletions(-)

diff --git a/hypervisor/arch/x86/guest/assign.c b/hypervisor/arch/x86/guest/assign.c
index a49b3314b..0baa9404b 100644
--- a/hypervisor/arch/x86/guest/assign.c
+++ b/hypervisor/arch/x86/guest/assign.c
@@ -382,15 +382,9 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm
*vm, uint3
pr_err("INTX re-add vpin %d", virt_gsi);
}
} else if (entry->vm != vm) {
- if (is_service_vm(entry->vm)) {
- entry->vm = vm;
- entry->virt_sid.value = virt_sid.value;
- entry->polarity = 0U;
- } else {
- pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d
with vgsi%d",
- phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm-

vm_id, virt_gsi);

- entry = NULL;
- }
+ pr_err("INTX gsi%d already in vm%d with vgsi%d, not able to add into vm%d with
vgsi%d",
+ phys_gsi, entry->vm->vm_id, entry->virt_sid.intx_id.gsi, vm->vm_id,
virt_gsi);
+ entry = NULL;

With this change you also need to replace the call to ptirq_add_intx_remapping() with your "reassign" version in create_vm() in arch/x86/guest/vm.c. That piece of logic grants some legacy interrupt lines to a pre-launched VM which may be created before or after the service VM starts, depending on the processing speed of different cores.

INTx remappings are just added to Pre-launched VM according to static
configurations. In `prepare_vm`, INTx remappings for Pre-launched VMs are
handled before ServiceVM starts. So there is no reassign cases for
Pre-launched VMs.

Thanks
Qiang

---
Best Regards
Junjie Mao

} else {
/* The mapping has already been added to the VM. No action
* required.
@@ -410,7 +404,7 @@ static struct ptirq_remapping_info *add_intx_remapping(struct acrn_vm
*vm, uint3
return entry;
}

-/* deactive & remove mapping entry of vpin for vm */
+/* deactivate & remove mapping entry of vpin for vm */
static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, enum
intx_ctlr vgsi_ctlr)
{
uint32_t phys_irq;
@@ -431,7 +425,7 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t
virt_gsi, e

dmar_free_irte(&intr_src, entry->irte_idx);
dev_dbg(DBG_LEVEL_IRQ,
- "deactive %s intx entry:pgsi=%d, pirq=%d ",
+ "deactivate %s intx entry:pgsi=%d, pirq=%d ",
(vgsi_ctlr == INTX_CTLR_PIC) ? "vPIC" : "vIOAPIC",
entry->phys_sid.intx_id.gsi, phys_irq);
dev_dbg(DBG_LEVEL_IRQ, "from vm%d vgsi=%d\n",
@@ -442,6 +436,19 @@ static void remove_intx_remapping(const struct acrn_vm *vm, uint32_t
virt_gsi, e
}
}

+static struct ptirq_remapping_info *reassign_intx_remapping(struct acrn_vm *vm, uint32_t
virt_gsi,
+ uint32_t phys_gsi, enum intx_ctlr vgsi_ctlr)
+{
+ struct ptirq_remapping_info *entry = NULL;
+ DEFINE_INTX_SID(phys_sid, phys_gsi, INTX_CTLR_IOAPIC);
+
+ entry = find_ptirq_entry(PTDEV_INTR_INTX, &phys_sid, NULL);
+ if (entry)
+ remove_intx_remapping(entry->vm, entry->virt_sid.intx_id.gsi, entry-

virt_sid.intx_id.ctlr);

+
+ return add_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+}
+
static void ptirq_handle_intx(struct acrn_vm *vm,
const struct ptirq_remapping_info *entry)
{
@@ -815,6 +822,18 @@ void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t
virt_gsi, bo
spinlock_release(&ptdev_lock);
}

+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
phys_gsi, bool pic_pin)
+{
+ struct ptirq_remapping_info *entry;
+ enum intx_ctlr vgsi_ctlr = pic_pin ? INTX_CTLR_PIC : INTX_CTLR_IOAPIC;
+
+ spinlock_obtain(&ptdev_lock);
+ entry = reassign_intx_remapping(vm, virt_gsi, phys_gsi, vgsi_ctlr);
+ spinlock_release(&ptdev_lock);
+
+ return (entry != NULL) ? 0 : -ENODEV;
+}
+
/*
* @pre vm != NULL
*/
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 5269380c1..faec16545 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -1016,7 +1016,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vcpu *vcpu, struct
acrn_vm *target
if ((((!irq.intx.pic_pin) && (irq.intx.virt_pin <
get_vm_gsicount(target_vm)))
|| ((irq.intx.pic_pin) && (irq.intx.virt_pin <
vpic_pincount())))
&& is_gsi_valid(irq.intx.phys_pin)) {
- ret = ptirq_add_intx_remapping(target_vm,
irq.intx.virt_pin,
+ ret = ptirq_reassign_intx_remapping(target_vm,
irq.intx.virt_pin,
irq.intx.phys_pin, irq.intx.pic_pin);
} else {
pr_err("%s: Invalid phys pin or virt pin\n", __func__);
diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index e7e49934a..6308b7514 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -22,9 +22,14 @@ struct ptirq_remapping_info ptirq_entries[CONFIG_MAX_PT_IRQ_ENTRIES];
static uint64_t ptirq_entry_bitmaps[PTIRQ_BITMAP_ARRAY_SIZE];
spinlock_t ptdev_lock = { .head = 0U, .tail = 0U, };

-static struct ptirq_entry_head {
- struct hlist_head list;
-} ptirq_entry_heads[PTIRQ_ENTRY_HASHSIZE];
+struct ptirq_hash_table {
+ struct hlist_head buckets[PTIRQ_ENTRY_HASHSIZE];
+};
+
+/* lookup mapping info from phyical gsi, hashing from gsi + vm address(0) */
+static struct ptirq_hash_table phy_gsi_htable;
+/* lookup mapping info from vgsi within a vm, hashing from vm address + vgsi */
+static struct ptirq_hash_table vm_vgsi_htable;

static inline uint16_t ptirq_alloc_entry_id(void)
{
@@ -40,28 +45,36 @@ static inline uint16_t ptirq_alloc_entry_id(void)
return (id < CONFIG_MAX_PT_IRQ_ENTRIES) ? id: INVALID_PTDEV_ENTRY_ID;
}

+/* to find ptirq_remapping_info from phy gsi or vgsi in a vm */
struct ptirq_remapping_info *find_ptirq_entry(uint32_t intr_type,
const union source_id *sid, const struct acrn_vm *vm)
{
struct hlist_node *p;
+ struct hlist_head *b;
struct ptirq_remapping_info *n, *entry = NULL;
- uint64_t key = hash64(sid->value, PTIRQ_ENTRY_HASHBITS);
- struct ptirq_entry_head *b = &ptirq_entry_heads[key];
+ uint64_t key = hash64(sid->value + (uint64_t)vm, PTIRQ_ENTRY_HASHBITS);
+
+ if (vm == NULL) {
+ b = &phy_gsi_htable.buckets[key];

- hlist_for_each(p, &b->list) {
- if (vm == NULL) {
+ hlist_for_each(p, b) {
n = hlist_entry(p, struct ptirq_remapping_info, phys_link);
- } else {
- n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->phys_sid.value))
{
+ entry = n;
+ break;
+ }
+ }
}
-
- if (is_entry_active(n)) {
- if ((intr_type == n->intr_type) &&
- ((vm == NULL) ?
- (sid->value == n->phys_sid.value) :
- ((vm == n->vm) && (sid->value == n->virt_sid.value)))) {
- entry = n;
- break;
+ } else {
+ b = &vm_vgsi_htable.buckets[key];
+ hlist_for_each(p, b) {
+ n = hlist_entry(p, struct ptirq_remapping_info, virt_link);
+ if (is_entry_active(n)) {
+ if ((intr_type == n->intr_type) && (sid->value == n->virt_sid.value)
&& (vm == n->vm)) {
+ entry = n;
+ break;
+ }
}
}
}
@@ -212,9 +225,9 @@ int32_t ptirq_activate_entry(struct ptirq_remapping_info *entry,
uint32_t phys_i
entry->active = true;

key = hash64(entry->phys_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->phys_link, &(ptirq_entry_heads[key].list));
- key = hash64(entry->virt_sid.value, PTIRQ_ENTRY_HASHBITS);
- hlist_add_head(&entry->virt_link, &(ptirq_entry_heads[key].list));
+ hlist_add_head(&entry->phys_link, &(phy_gsi_htable.buckets[key]));
+ key = hash64(entry->virt_sid.value + (uint64_t)entry->vm, PTIRQ_ENTRY_HASHBITS);
+ hlist_add_head(&entry->virt_link, &(vm_vgsi_htable.buckets[key]));
}

return ret;
diff --git a/hypervisor/include/arch/x86/asm/guest/assign.h
b/hypervisor/include/arch/x86/asm/guest/assign.h
index 9b3b666af..1387b9825 100644
--- a/hypervisor/include/arch/x86/asm/guest/assign.h
+++ b/hypervisor/include/arch/x86/asm/guest/assign.h
@@ -125,6 +125,27 @@ int32_t ptirq_add_intx_remapping(struct acrn_vm *vm, uint32_t
virt_gsi, uint32_t
*/
void ptirq_remove_intx_remapping(const struct acrn_vm *vm, uint32_t virt_gsi, bool
pic_pin);

+/**
+ * @brief Assign a INTx to Postlaunched VM.
+ *
+ * If the physical pin was assigned to another VM, remove it first.
+ * Then assign the physical pin to target virtual VM.
+ *
+ * @param[in] vm pointer to acrn_vm
+ * @param[in] virt_gsi virtual pin number associated with the passthrough device
+ * @param[in] phys_gsi physical pin number associated with the passthrough device
+ * @param[in] pic_pin true for pic, false for ioapic
+ *
+ * @return
+ * - 0: on success
+ * - \p -EINVAL: invalid virt_pin value
+ * - \p -ENODEV: failed to add the remapping entry
+ *
+ * @pre vm != NULL
+ *
+ */
+int32_t ptirq_reassign_intx_remapping(struct acrn_vm *vm, uint32_t virt_gsi, uint32_t
phys_gsi, bool pic_pin);
+
/**
* @brief Remove interrupt remapping entry/entries for MSI/MSI-x.
*
--
2.30.2