The patch fix the msi clear pending bit issue, add new struct element
in the msicap struct, we will use this element by address pointer.

Signed-off-by: Liu Long <long.liu@...>
---
devicemodel/hw/pci/core.c | 13 ++++++-------
devicemodel/include/pci_core.h | 5 ++++-
2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/devicemodel/hw/pci/core.c b/devicemodel/hw/pci/core.c
index 4f52a76a4..e44045b12 100644
--- a/devicemodel/hw/pci/core.c
+++ b/devicemodel/hw/pci/core.c
@@ -961,7 +961,7 @@ pci_emul_add_capability(struct pci_vdev *dev, u_char *capdata, int caplen)
int
pci_emul_find_capability(struct pci_vdev *dev, uint8_t capid, int *p_capoff)
{
- int coff;
+ int coff = 0;
uint16_t sts;

sts = pci_get_cfgdata16(dev, PCIR_STATUS);
@@ -1069,7 +1069,7 @@ static int
pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
{
uint16_t msgctrl;
- int rc, offset;
+ int rc, offset = 0;

if (msi_cap > PCIR_MSI_PENDING) {
pr_err("%s: Msi capability length is out of msi length!\n", __func__);
@@ -1079,7 +1079,7 @@ pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
if (rc)
return -1;

- msgctrl = pci_get_cfgdata16(dev, offset);
+ msgctrl = pci_get_cfgdata16(dev, offset + 2);
if (msgctrl & PCIM_MSICTRL_64BIT)
offset = offset + msi_cap;
else
@@ -1088,8 +1088,7 @@ pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
if (is_write)
pci_set_cfgdata32(dev, offset, *val);
else
- *val = pci_get_cfgdata16(dev, offset);
-
+ *val = pci_get_cfgdata32(dev, offset);
return 0;
}

@@ -1133,7 +1132,7 @@ pci_set_msi_pending(struct pci_vdev *dev, uint32_t index, bool set)
if (set)
val = (1 << index) | val;
else
- val = (~(1 << index)) | val;
+ val = (~(1 << index)) & val;
pci_access_msi(dev, PCIR_MSI_PENDING, &val, true);
}

@@ -1160,7 +1159,7 @@ pci_populate_msicap(struct msicap *msicap, int msgnum, int nextptr)
int
pci_emul_add_msicap(struct pci_vdev *dev, int msgnum)
{
- struct msicap msicap;
+ struct msicap msicap = {0};

return pci_populate_msicap(&msicap, msgnum, 0) ||
pci_emul_add_capability(dev, (u_char *)&msicap, sizeof(msicap));
diff --git a/devicemodel/include/pci_core.h b/devicemodel/include/pci_core.h
index 5aa562c3b..f24bb33bf 100644
--- a/devicemodel/include/pci_core.h
+++ b/devicemodel/include/pci_core.h
@@ -200,8 +200,11 @@ struct msicap {
uint32_t addrlo;
uint32_t addrhi;
uint16_t msgdata;
+ uint16_t reserve;
+ uint32_t maskbit;
+ uint32_t pendbit;
} __attribute__((packed));
-static_assert(sizeof(struct msicap) == 14, "compile-time assertion failed");
+static_assert(sizeof(struct msicap) == 24, "compile-time assertion failed");

struct msixcap {
uint8_t capid;
--
2.25.1

On Wed, Jun 08, 2022 at 04:35:07PM +0800, Liu Long wrote:

The patch fix the msi clear pending bit issue, add new struct element
in the msicap struct, we will use this element by address pointer.

Signed-off-by: Liu Long <long.liu@...>
---
devicemodel/hw/pci/core.c | 13 ++++++-------
devicemodel/include/pci_core.h | 5 ++++-
2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/devicemodel/hw/pci/core.c b/devicemodel/hw/pci/core.c
index 4f52a76a4..e44045b12 100644
--- a/devicemodel/hw/pci/core.c
+++ b/devicemodel/hw/pci/core.c
@@ -961,7 +961,7 @@ pci_emul_add_capability(struct pci_vdev *dev, u_char *capdata, int caplen)
int
pci_emul_find_capability(struct pci_vdev *dev, uint8_t capid, int *p_capoff)
{
- int coff;
+ int coff = 0;
uint16_t sts;

sts = pci_get_cfgdata16(dev, PCIR_STATUS);
@@ -1069,7 +1069,7 @@ static int
pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
{
uint16_t msgctrl;
- int rc, offset;
+ int rc, offset = 0;

if (msi_cap > PCIR_MSI_PENDING) {
pr_err("%s: Msi capability length is out of msi length!\n", __func__);
@@ -1079,7 +1079,7 @@ pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
if (rc)
return -1;

- msgctrl = pci_get_cfgdata16(dev, offset);
+ msgctrl = pci_get_cfgdata16(dev, offset + 2);

offset + PCIR_MSI_CTRL

if (msgctrl & PCIM_MSICTRL_64BIT)
offset = offset + msi_cap;
else
@@ -1088,8 +1088,7 @@ pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
if (is_write)
pci_set_cfgdata32(dev, offset, *val);
else
- *val = pci_get_cfgdata16(dev, offset);
-
+ *val = pci_get_cfgdata32(dev, offset);
return 0;
}

@@ -1133,7 +1132,7 @@ pci_set_msi_pending(struct pci_vdev *dev, uint32_t index, bool set)
if (set)
val = (1 << index) | val;
else
- val = (~(1 << index)) | val;
+ val = (~(1 << index)) & val;
pci_access_msi(dev, PCIR_MSI_PENDING, &val, true);
}

@@ -1160,7 +1159,7 @@ pci_populate_msicap(struct msicap *msicap, int msgnum, int nextptr)
int
pci_emul_add_msicap(struct pci_vdev *dev, int msgnum)
{
- struct msicap msicap;
+ struct msicap msicap = {0};

return pci_populate_msicap(&msicap, msgnum, 0) ||
pci_emul_add_capability(dev, (u_char *)&msicap, sizeof(msicap));
diff --git a/devicemodel/include/pci_core.h b/devicemodel/include/pci_core.h
index 5aa562c3b..f24bb33bf 100644
--- a/devicemodel/include/pci_core.h
+++ b/devicemodel/include/pci_core.h
@@ -200,8 +200,11 @@ struct msicap {
uint32_t addrlo;
uint32_t addrhi;
uint16_t msgdata;
+ uint16_t reserve;
+ uint32_t maskbit;
+ uint32_t pendbit;

If you add such fields into the msicap, then we should use them to check
mask and pending? Otherwise, what're their meaning?

} __attribute__((packed));
-static_assert(sizeof(struct msicap) == 14, "compile-time assertion failed");
+static_assert(sizeof(struct msicap) == 24, "compile-time assertion failed");

struct msixcap {
uint8_t capid;
--
2.25.1

On Thu, Jun 09, 2022 at 09:03:35AM +0000, Long Liu wrote:

-----Original Message-----
From: acrn-dev@... <acrn-dev@...> On Behalf Of Yu Wang
Sent: Wednesday, June 8, 2022 7:50 PM
To: Liu Long <long.liu@...>
Cc: acrn-dev@...; Li, Fei1 <fei1.li@...>
Subject: Re: [acrn-dev] [PATCH] ACRN: DM: Fix the MSI mask and unmask bugs.

On Wed, Jun 08, 2022 at 04:35:07PM +0800, Liu Long wrote:

The patch fix the msi clear pending bit issue, add new struct element
in the msicap struct, we will use this element by address pointer.

Please explain the detailed fixes. This patch has several fix, please
list them one by one.

Signed-off-by: Liu Long <long.liu@...>
---
devicemodel/hw/pci/core.c | 13 ++++++-------
devicemodel/include/pci_core.h | 5 ++++-
2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/devicemodel/hw/pci/core.c b/devicemodel/hw/pci/core.c
index 4f52a76a4..e44045b12 100644
--- a/devicemodel/hw/pci/core.c
+++ b/devicemodel/hw/pci/core.c
@@ -961,7 +961,7 @@ pci_emul_add_capability(struct pci_vdev *dev,
u_char *capdata, int caplen) int pci_emul_find_capability(struct
pci_vdev *dev, uint8_t capid, int *p_capoff) {
- int coff;
+ int coff = 0;
uint16_t sts;

sts = pci_get_cfgdata16(dev, PCIR_STATUS); @@ -1069,7 +1069,7 @@
static int pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t
*val, bool is_write) {
uint16_t msgctrl;
- int rc, offset;
+ int rc, offset = 0;

if (msi_cap > PCIR_MSI_PENDING) {
pr_err("%s: Msi capability length is out of msi length!\n",
__func__); @@ -1079,7 +1079,7 @@ pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
if (rc)
return -1;

- msgctrl = pci_get_cfgdata16(dev, offset);
+ msgctrl = pci_get_cfgdata16(dev, offset + 2);

offset + PCIR_MSI_CTRL

if (msgctrl & PCIM_MSICTRL_64BIT)
offset = offset + msi_cap;
else
@@ -1088,8 +1088,7 @@ pci_access_msi(struct pci_vdev *dev, int msi_cap, uint32_t *val, bool is_write)
if (is_write)
pci_set_cfgdata32(dev, offset, *val);
else
- *val = pci_get_cfgdata16(dev, offset);
-
+ *val = pci_get_cfgdata32(dev, offset);
return 0;
}

@@ -1133,7 +1132,7 @@ pci_set_msi_pending(struct pci_vdev *dev, uint32_t index, bool set)
if (set)
val = (1 << index) | val;
else
- val = (~(1 << index)) | val;
+ val = (~(1 << index)) & val;
pci_access_msi(dev, PCIR_MSI_PENDING, &val, true); }

@@ -1160,7 +1159,7 @@ pci_populate_msicap(struct msicap *msicap, int
msgnum, int nextptr) int pci_emul_add_msicap(struct pci_vdev *dev,
int msgnum) {
- struct msicap msicap;
+ struct msicap msicap = {0};

return pci_populate_msicap(&msicap, msgnum, 0) ||
pci_emul_add_capability(dev, (u_char *)&msicap, sizeof(msicap));
diff --git a/devicemodel/include/pci_core.h
b/devicemodel/include/pci_core.h index 5aa562c3b..f24bb33bf 100644
--- a/devicemodel/include/pci_core.h
+++ b/devicemodel/include/pci_core.h
@@ -200,8 +200,11 @@ struct msicap {
uint32_t addrlo;
uint32_t addrhi;
uint16_t msgdata;
+ uint16_t reserve;
+ uint32_t maskbit;
+ uint32_t pendbit;

If you add such fields into the msicap, then we should use them to check mask and pending? Otherwise, what're their meaning?

[Long:] We will use these fields by address pointer, just like the base + offset.

I see.

} __attribute__((packed));
-static_assert(sizeof(struct msicap) == 14, "compile-time assertion
failed");
+static_assert(sizeof(struct msicap) == 24, "compile-time assertion
+failed");

struct msixcap {
uint8_t capid;
--
2.25.1