From 5f5ba1d647967f9546e12010996af315d9c13346 Mon Sep 17 00:00:00 2001 From: Li Fei1 Date: Thu, 28 Nov 2019 17:45:57 +0800 Subject: [PATCH] hv: vmsi: refine write_vmsi_cfg implementation 1. disable physical MSI before writing the virtual MSI CFG space 2. do the remap_vmsi if the guest wants to enable MSI or update MSI address or data 3. disable INTx and enable MSI after step 2. The previous Message Control check depends on the guest write MSI Message Control Register at the offset of Message Control Register. However, the guest could access this register at the offset of MSI Capability ID register. This patch remove this constraint. Also, The previous implementation didn't really disable MSI when guest wanted to disable MSI. Tracked-On: #3475 Signed-off-by: Li Fei1 --- hypervisor/dm/vpci/vmsi.c | 106 ++++++++++++++--------------------- hypervisor/include/dm/vpci.h | 1 + 2 files changed, 44 insertions(+), 63 deletions(-) diff --git a/hypervisor/dm/vpci/vmsi.c b/hypervisor/dm/vpci/vmsi.c index c33b6ad17..2b9f982b9 100644 --- a/hypervisor/dm/vpci/vmsi.c +++ b/hypervisor/dm/vpci/vmsi.c @@ -35,69 +35,64 @@ /** + * @pre vdev != NULL + * @pre vdev->pdev != NULL + */ +static inline void enable_disable_msi(const struct pci_vdev *vdev, bool enable) +{ + union pci_bdf pbdf = vdev->pdev->bdf; + uint32_t capoff = vdev->msi.capoff; + uint32_t msgctrl = pci_pdev_read_cfg(pbdf, capoff + PCIR_MSI_CTRL, 2U); + + if (enable) { + msgctrl |= PCIM_MSICTRL_MSI_ENABLE; + } else { + msgctrl &= ~PCIM_MSICTRL_MSI_ENABLE; + } + pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_CTRL, 2U, msgctrl); +} +/** + * @brief Remap vMSI virtual address and data to MSI physical address and data + * This function is called when physical MSI is disabled. + * * @pre vdev != NULL * @pre vdev->vpci != NULL * @pre vdev->vpci->vm != NULL * @pre vdev->pdev != NULL */ -static int32_t remap_vmsi(const struct pci_vdev *vdev, bool enable) +static void remap_vmsi(const struct pci_vdev *vdev) { - struct ptirq_msi_info info; + struct ptirq_msi_info info = {}; union pci_bdf pbdf = vdev->pdev->bdf; struct acrn_vm *vm = vdev->vpci->vm; uint32_t capoff = vdev->msi.capoff; - uint32_t msgctrl, msgdata; - uint32_t addrlo, addrhi; - int32_t ret; - - /* Disable MSI during configuration */ - msgctrl = pci_vdev_read_cfg(vdev, capoff + PCIR_MSI_CTRL, 2U); - if ((msgctrl & PCIM_MSICTRL_MSI_ENABLE) == PCIM_MSICTRL_MSI_ENABLE) { - pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_CTRL, 2U, msgctrl & ~PCIM_MSICTRL_MSI_ENABLE); - } + uint32_t vmsi_msgdata, vmsi_addrlo, vmsi_addrhi = 0U; /* Read the MSI capability structure from virtual device */ - addrlo = pci_vdev_read_cfg_u32(vdev, capoff + PCIR_MSI_ADDR); - if ((msgctrl & PCIM_MSICTRL_64BIT) != 0U) { - msgdata = pci_vdev_read_cfg_u16(vdev, capoff + PCIR_MSI_DATA_64BIT); - addrhi = pci_vdev_read_cfg_u32(vdev, capoff + PCIR_MSI_ADDR_HIGH); + vmsi_addrlo = pci_vdev_read_cfg_u32(vdev, capoff + PCIR_MSI_ADDR); + if (vdev->msi.is_64bit) { + vmsi_addrhi = pci_vdev_read_cfg_u32(vdev, capoff + PCIR_MSI_ADDR_HIGH); + vmsi_msgdata = pci_vdev_read_cfg_u16(vdev, capoff + PCIR_MSI_DATA_64BIT); } else { - msgdata = pci_vdev_read_cfg_u16(vdev, capoff + PCIR_MSI_DATA); - addrhi = 0U; + vmsi_msgdata = pci_vdev_read_cfg_u16(vdev, capoff + PCIR_MSI_DATA); } + info.vmsi_addr.full = (uint64_t)vmsi_addrlo | ((uint64_t)vmsi_addrhi << 32U); + info.vmsi_data.full = vmsi_msgdata; - info.vmsi_addr.full = (uint64_t)addrlo | ((uint64_t)addrhi << 32U); - - /* MSI is being enabled or disabled */ - if (enable) { - info.vmsi_data.full = msgdata; - } else { - info.vmsi_data.full = 0U; - } - - ret = ptirq_prepare_msix_remap(vm, vdev->bdf.value, pbdf.value, 0U, &info); - if (ret == 0) { - /* Update MSI Capability structure to physical device */ - if ((msgctrl & PCIM_MSICTRL_64BIT) != 0U) { + if (ptirq_prepare_msix_remap(vm, vdev->bdf.value, pbdf.value, 0U, &info) == 0) { + pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR, 0x4U, (uint32_t)info.pmsi_addr.full); + if (vdev->msi.is_64bit) { + pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR_HIGH, 0x4U, + (uint32_t)(info.pmsi_addr.full >> 32U)); pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_DATA_64BIT, 0x2U, (uint16_t)info.pmsi_data.full); } else { pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_DATA, 0x2U, (uint16_t)info.pmsi_data.full); } - if (enable) { - pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR, 0x4U, (uint32_t)info.pmsi_addr.full); - if ((msgctrl & PCIM_MSICTRL_64BIT) != 0U) { - pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_ADDR_HIGH, 0x4U, - (uint32_t)(info.pmsi_addr.full >> 32U)); - } - - /* If MSI Enable is being set, make sure INTxDIS bit is set */ - enable_disable_pci_intx(pbdf, false); - pci_pdev_write_cfg(pbdf, capoff + PCIR_MSI_CTRL, 2U, msgctrl | PCIM_MSICTRL_MSI_ENABLE); - } + /* If MSI Enable is being set, make sure INTxDIS bit is set */ + enable_disable_pci_intx(pbdf, false); + enable_disable_msi(vdev, true); } - - return ret; } /** @@ -116,30 +111,14 @@ void vmsi_read_cfg(const struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, */ void vmsi_write_cfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t val) { - bool message_changed = false; - bool enable; uint32_t msgctrl; - /* Save msgctrl for comparison */ - msgctrl = pci_vdev_read_cfg(vdev, vdev->msi.capoff + PCIR_MSI_CTRL, 2U); - - /* Either Message Data or message Addr is being changed */ - if (((offset - vdev->msi.capoff) >= PCIR_MSI_ADDR) && (val != pci_vdev_read_cfg(vdev, offset, bytes))) { - message_changed = true; - } - - /* Write to vdev */ + enable_disable_msi(vdev, false); pci_vdev_write_cfg(vdev, offset, bytes, val); - /* Do remap if MSI Enable bit is being changed */ - if (((offset - vdev->msi.capoff) == PCIR_MSI_CTRL) && - (((msgctrl ^ val) & PCIM_MSICTRL_MSI_ENABLE) != 0U)) { - enable = ((val & PCIM_MSICTRL_MSI_ENABLE) != 0U); - (void)remap_vmsi(vdev, enable); - } else { - if (message_changed && ((msgctrl & PCIM_MSICTRL_MSI_ENABLE) != 0U)) { - (void)remap_vmsi(vdev, true); - } + msgctrl = pci_vdev_read_cfg(vdev, vdev->msi.capoff + PCIR_MSI_CTRL, 2U); + if ((msgctrl & PCIM_MSICTRL_MSI_ENABLE) != 0U) { + remap_vmsi(vdev); } } @@ -169,6 +148,7 @@ void init_vmsi(struct pci_vdev *vdev) if (has_msi_cap(vdev)) { val = pci_pdev_read_cfg(pdev->bdf, vdev->msi.capoff, 4U); vdev->msi.caplen = ((val & (PCIM_MSICTRL_64BIT << 16U)) != 0U) ? 14U : 10U; + vdev->msi.is_64bit = ((val & (PCIM_MSICTRL_64BIT << 16U)) != 0U); val &= ~((uint32_t)PCIM_MSICTRL_MMC_MASK << 16U); val &= ~((uint32_t)PCIM_MSICTRL_MME_MASK << 16U); diff --git a/hypervisor/include/dm/vpci.h b/hypervisor/include/dm/vpci.h index 4f0cafb38..a51107550 100644 --- a/hypervisor/include/dm/vpci.h +++ b/hypervisor/include/dm/vpci.h @@ -51,6 +51,7 @@ struct msix_table_entry { /* MSI capability structure */ struct pci_msi { + bool is_64bit; uint32_t capoff; uint32_t caplen; };