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

On Tue, Apr 11, 2023 at 12:28:10AM +0000, Junjie Mao wrote:

-----Original Message-----
From: Qiang Zhang <qiang4.zhang@...>
Sent: Thursday, March 9, 2023 9:57 AM
To: acrn-dev@...
Cc: Mao, Junjie <junjie.mao@...>; Li, Fei1 <fei1.li@...>
Subject: [PATCH v5 1/2] ptirq: Fix ptirq hash tables

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@...>

Reviewed-by: Junjie Mao <junjie.mao@...>

Some minor comments below. Thanks.

---
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 */

vm address -> acrn_vm structure address

"VM address" looks quite confusing.

Will change in PR

+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);

Since there are two places where you hash a virtual sid into a key, it would be better if we can abstract the calculation as a static inline function to ensure consistency.

OK. will change in PR

---
Best Regards
Junjie Mao

- 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

On Tue, Apr 11, 2023 at 12:51:16AM +0000, Eddie Dong wrote:

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On
Behalf Of Qiang Zhang
Sent: Thursday, March 9, 2023 9:57 AM
To: acrn-dev@...
Cc: Mao, Junjie <junjie.mao@...>; Li, Fei1 <fei1.li@...>
Subject: [acrn-dev] [PATCH v5 1/2] ptirq: Fix ptirq hash tables

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 */

We use vm==NULL to indicate whether the sid is physical side or VM side sid. Can you extend this comment to include this too?

OK. will change in PR

The rest is fine to me.

PR after fixing Junjie's comment.

Acked-by: Eddie Dong <eddie.dong@...>

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