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

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

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