Topics

[PATCH v3] hv: vpci: fix msi enable issue under some cases


Junming Liu
 

Current code would disable MSI before check RW permission,
however, if MSI interrupt has been enabled,
vCPU writes the content in MSI registers,
and all bits of the content is read-only.
The disable MSI operation was wrong behavior.

This patch move MSI disable to remap_vmsi function
before writing the physical MSI registers
to pair MSI enable to solve this problem.

v2 -> v3:
* refine commit comments
v1 -> v2:
* move enable_disable_msi(vdev, false) into remap_vmsi
interface to improve code readability.

Tracked-On: #5847

Signed-off-by: liujunming <junming.liu@intel.com>
---
hypervisor/dm/vpci/vmsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hypervisor/dm/vpci/vmsi.c b/hypervisor/dm/vpci/vmsi.c
index 0f9c0c9..73f775e 100644
--- a/hypervisor/dm/vpci/vmsi.c
+++ b/hypervisor/dm/vpci/vmsi.c
@@ -80,6 +80,7 @@ static void remap_vmsi(const struct pci_vdev *vdev)
info.data.full = vmsi_msgdata;

if (ptirq_prepare_msix_remap(vm, vdev->bdf.value, pbdf.value, 0U, &info, INVALID_IRTE_ID) == 0) {
+ enable_disable_msi(vdev, false);
pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR, 0x4U, (uint32_t)info.addr.full);
if (vdev->msi.is_64bit) {
pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR_HIGH, 0x4U,
@@ -107,8 +108,6 @@ void write_vmsi_cap_reg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes,
static const uint8_t msi_ro_mask[0xEU] = { 0xffU, 0xffU, 0x8eU, 0xffU };
uint32_t msgctrl, old, ro_mask = ~0U;

- enable_disable_msi(vdev, false);
-
(void)memcpy_s((void *)&ro_mask, bytes, (void *)&msi_ro_mask[offset - vdev->msi.capoff], bytes);
if (ro_mask != ~0U) {
old = pci_vdev_read_vcfg(vdev, offset, bytes);
--
2.7.4


Li, Fei1
 

On Mon, Mar 22, 2021 at 11:52:14PM +0800, Junming Liu wrote:
Current code would disable MSI before check RW permission,
however, if MSI interrupt has been enabled,
vCPU writes the content in MSI registers,
and all bits of the content is read-only.
The disable MSI operation was wrong behavior.

This patch move MSI disable to remap_vmsi function
before writing the physical MSI registers
to pair MSI enable to solve this problem.

v2 -> v3:
* refine commit comments
v1 -> v2:
* move enable_disable_msi(vdev, false) into remap_vmsi
interface to improve code readability.

Tracked-On: #5847

Signed-off-by: liujunming <junming.liu@intel.com>
---
hypervisor/dm/vpci/vmsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hypervisor/dm/vpci/vmsi.c b/hypervisor/dm/vpci/vmsi.c
index 0f9c0c9..73f775e 100644
--- a/hypervisor/dm/vpci/vmsi.c
+++ b/hypervisor/dm/vpci/vmsi.c
@@ -80,6 +80,7 @@ static void remap_vmsi(const struct pci_vdev *vdev)
info.data.full = vmsi_msgdata;

if (ptirq_prepare_msix_remap(vm, vdev->bdf.value, pbdf.value, 0U, &info, INVALID_IRTE_ID) == 0) {
+ enable_disable_msi(vdev, false);
Hi Junming

After second thinking, it should better to change the code like

enable_disable_msi(vdev, false);
if (ptirq_prepare_msix_remap) {
...
}
enable_disable_msi(vdev, true);

We should disable the MSI before we modify IRTE in VTD.

pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR, 0x4U, (uint32_t)info.addr.full);
if (vdev->msi.is_64bit) {
pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR_HIGH, 0x4U,
@@ -107,8 +108,6 @@ void write_vmsi_cap_reg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes,
static const uint8_t msi_ro_mask[0xEU] = { 0xffU, 0xffU, 0x8eU, 0xffU };
uint32_t msgctrl, old, ro_mask = ~0U;

- enable_disable_msi(vdev, false);
-
(void)memcpy_s((void *)&ro_mask, bytes, (void *)&msi_ro_mask[offset - vdev->msi.capoff], bytes);
if (ro_mask != ~0U) {
old = pci_vdev_read_vcfg(vdev, offset, bytes);
--
2.7.4






Junming Liu
 

-----Original Message-----
From: acrn-dev@lists.projectacrn.org <acrn-dev@lists.projectacrn.org> On
Behalf Of Li, Fei1
Sent: Monday, March 22, 2021 4:33 PM
To: acrn-dev@lists.projectacrn.org
Cc: Liu, Junming <junming.liu@intel.com>
Subject: Re: [acrn-dev] [PATCH v3] hv: vpci: fix msi enable issue under some
cases

On Mon, Mar 22, 2021 at 11:52:14PM +0800, Junming Liu wrote:
Current code would disable MSI before check RW permission, however, if
MSI interrupt has been enabled, vCPU writes the content in MSI
registers, and all bits of the content is read-only.
The disable MSI operation was wrong behavior.

This patch move MSI disable to remap_vmsi function before writing the
physical MSI registers to pair MSI enable to solve this problem.

v2 -> v3:
* refine commit comments
v1 -> v2:
* move enable_disable_msi(vdev, false) into remap_vmsi
interface to improve code readability.

Tracked-On: #5847

Signed-off-by: liujunming <junming.liu@intel.com>
---
hypervisor/dm/vpci/vmsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hypervisor/dm/vpci/vmsi.c b/hypervisor/dm/vpci/vmsi.c
index 0f9c0c9..73f775e 100644
--- a/hypervisor/dm/vpci/vmsi.c
+++ b/hypervisor/dm/vpci/vmsi.c
@@ -80,6 +80,7 @@ static void remap_vmsi(const struct pci_vdev *vdev)
info.data.full = vmsi_msgdata;

if (ptirq_prepare_msix_remap(vm, vdev->bdf.value, pbdf.value, 0U,
&info, INVALID_IRTE_ID) == 0) {
+ enable_disable_msi(vdev, false);
Hi Junming

After second thinking, it should better to change the code like

enable_disable_msi(vdev, false);
if (ptirq_prepare_msix_remap) {
...
}
enable_disable_msi(vdev, true);

We should disable the MSI before we modify IRTE in VTD.
Thanks, will refine it in the next version.
pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR, 0x4U,
(uint32_t)info.addr.full);
if (vdev->msi.is_64bit) {
pci_pdev_write_cfg(pbdf, capoff +
PCIR_MSI_ADDR_HIGH, 0x4U, @@
-107,8 +108,6 @@ void write_vmsi_cap_reg(struct pci_vdev *vdev,
uint32_t offset, uint32_t bytes,
static const uint8_t msi_ro_mask[0xEU] = { 0xffU, 0xffU, 0x8eU,
0xffU };
uint32_t msgctrl, old, ro_mask = ~0U;

- enable_disable_msi(vdev, false);
-
(void)memcpy_s((void *)&ro_mask, bytes, (void
*)&msi_ro_mask[offset - vdev->msi.capoff], bytes);
if (ro_mask != ~0U) {
old = pci_vdev_read_vcfg(vdev, offset, bytes);
--
2.7.4








Eddie Dong
 

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

-----Original Message-----
From: acrn-dev@lists.projectacrn.org <acrn-dev@lists.projectacrn.org> On
Behalf Of Junming Liu
Sent: Monday, March 22, 2021 11:52 PM
To: acrn-dev@lists.projectacrn.org
Cc: Liu, Junming <junming.liu@intel.com>
Subject: [acrn-dev] [PATCH v3] hv: vpci: fix msi enable issue under some cases

Current code would disable MSI before check RW permission, however, if MSI
interrupt has been enabled, vCPU writes the content in MSI registers, and all
bits of the content is read-only.
The disable MSI operation was wrong behavior.

This patch move MSI disable to remap_vmsi function before writing the
physical MSI registers to pair MSI enable to solve this problem.

v2 -> v3:
* refine commit comments
v1 -> v2:
* move enable_disable_msi(vdev, false) into remap_vmsi
interface to improve code readability.

Tracked-On: #5847

Signed-off-by: liujunming <junming.liu@intel.com>
---
hypervisor/dm/vpci/vmsi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hypervisor/dm/vpci/vmsi.c b/hypervisor/dm/vpci/vmsi.c index
0f9c0c9..73f775e 100644
--- a/hypervisor/dm/vpci/vmsi.c
+++ b/hypervisor/dm/vpci/vmsi.c
@@ -80,6 +80,7 @@ static void remap_vmsi(const struct pci_vdev *vdev)
info.data.full = vmsi_msgdata;

if (ptirq_prepare_msix_remap(vm, vdev->bdf.value, pbdf.value, 0U, &info,
INVALID_IRTE_ID) == 0) {
+ enable_disable_msi(vdev, false);
pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR, 0x4U,
(uint32_t)info.addr.full);
if (vdev->msi.is_64bit) {
pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR_HIGH,
0x4U, @@ -107,8 +108,6 @@ void write_vmsi_cap_reg(struct pci_vdev *vdev,
uint32_t offset, uint32_t bytes,
static const uint8_t msi_ro_mask[0xEU] = { 0xffU, 0xffU, 0x8eU, 0xffU };
uint32_t msgctrl, old, ro_mask = ~0U;

- enable_disable_msi(vdev, false);
-
(void)memcpy_s((void *)&ro_mask, bytes, (void *)&msi_ro_mask[offset -
vdev->msi.capoff], bytes);
if (ro_mask != ~0U) {
old = pci_vdev_read_vcfg(vdev, offset, bytes);
--
2.7.4