From d6bf0605733c7dc7654fb09f4978dbf7fa6b7d6c Mon Sep 17 00:00:00 2001 From: dongshen Date: Thu, 25 Jul 2019 16:50:11 -0700 Subject: [PATCH] HV: remove redundant function calling The caller function has already done the checking to make sure the req is targeted for the called functions, so there is no need to do the same checking in called functions. Remove calling of is_bar_offset() in vdev_pt_read_cfg/vdev_pt_write_cfg: In vpci.c's vpci_read_pt_dev_cfg and vpci_write_dev_cfg, vbar_access is called first to make sure the req is targed for vdev pt (vbar emulation) before dispatching the request to vdev_pt_read_cfg/vdev_pt_write_cfg, so there is no need to call is_bar_offset() again to do the same checking in vdev_pt_read_cfg/vdev_pt_write_cfg. The same goes for msicap_access/msixcap_access vbar_access should only check if the req is for bar access, it should not care about whether the bar access is 4 bytes or 4 bytes aligned. The called function vdev_pt_write_vbar will check and ignore the write access if it is not 4 bytes or 4 bytes aligned, although this is counted as a bar access. vdev_pt_read_vbar will check if the read access is 4 bytes or 4 bytes aligned, although this is counted as a bar access, set read value (*val) to -1 if the access is not 4 bytes (or aligned). Tracked-On: #3475 Signed-off-by: dongshen Acked-by: Eddie Dong --- hypervisor/dm/vpci/pci_pt.c | 19 +++++------ hypervisor/dm/vpci/vmsi.c | 58 ++++++++++++++-------------------- hypervisor/dm/vpci/vmsix.c | 46 +++++++++++---------------- hypervisor/dm/vpci/vpci.c | 4 +-- hypervisor/dm/vpci/vpci_priv.h | 8 ++--- 5 files changed, 57 insertions(+), 78 deletions(-) diff --git a/hypervisor/dm/vpci/pci_pt.c b/hypervisor/dm/vpci/pci_pt.c index faab74a39..a374b0baf 100644 --- a/hypervisor/dm/vpci/pci_pt.c +++ b/hypervisor/dm/vpci/pci_pt.c @@ -131,14 +131,14 @@ static uint64_t get_pbar_base(const struct pci_pdev *pdev, uint32_t idx) */ int32_t vdev_pt_read_cfg(const struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t *val) { - int32_t ret = -ENODEV; - - if (is_bar_offset(vdev->nr_bars, offset)) { + /* bar access must be 4 bytes and offset must also be 4 bytes aligned */ + if ((bytes == 4U) && ((offset & 0x3U) == 0U)) { *val = pci_vdev_read_cfg(vdev, offset, bytes); - ret = 0; + } else { + *val = ~0U; } - return ret; + return 0; } /** @@ -442,15 +442,12 @@ static void vdev_pt_write_vbar(struct pci_vdev *vdev, uint32_t offset, uint32_t */ int32_t vdev_pt_write_cfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t val) { - int32_t ret = -ENODEV; - - /* bar write access must be 4 bytes and offset must also be 4 bytes aligned*/ - if (is_bar_offset(vdev->nr_bars, offset) && (bytes == 4U) && ((offset & 0x3U) == 0U)) { + /* bar write access must be 4 bytes and offset must also be 4 bytes aligned */ + if ((bytes == 4U) && ((offset & 0x3U) == 0U)) { vdev_pt_write_vbar(vdev, offset, val); - ret = 0; } - return ret; + return 0; } /** diff --git a/hypervisor/dm/vpci/vmsi.c b/hypervisor/dm/vpci/vmsi.c index 4f5338e39..1426faf26 100644 --- a/hypervisor/dm/vpci/vmsi.c +++ b/hypervisor/dm/vpci/vmsi.c @@ -103,18 +103,15 @@ static int32_t vmsi_remap(const struct pci_vdev *vdev, bool enable) */ int32_t vmsi_read_cfg(const struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t *val) { - int32_t ret = -ENODEV; - /* For PIO access, we emulate Capability Structures only */ - if (msicap_access(vdev, offset)) { - *val = pci_vdev_read_cfg(vdev, offset, bytes); - ret = 0; - } + *val = pci_vdev_read_cfg(vdev, offset, bytes); - return ret; + return 0; } /** + * @brief Writing MSI Capability Structure + * * @pre vdev != NULL */ int32_t vmsi_write_cfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t val) @@ -122,37 +119,30 @@ int32_t vmsi_write_cfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, u bool message_changed = false; bool enable; uint32_t msgctrl; - int32_t ret = -ENODEV; - /* Writing MSI Capability Structure */ - if (msicap_access(vdev, offset)) { + /* Save msgctrl for comparison */ + msgctrl = pci_vdev_read_cfg(vdev, vdev->msi.capoff + PCIR_MSI_CTRL, 2U); - /* 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 */ - 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)vmsi_remap(vdev, enable); - } else { - if (message_changed && ((msgctrl & PCIM_MSICTRL_MSI_ENABLE) != 0U)) { - (void)vmsi_remap(vdev, true); - } - } - - ret = 0; + /* 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; } - return ret; + /* Write to vdev */ + 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)vmsi_remap(vdev, enable); + } else { + if (message_changed && ((msgctrl & PCIM_MSICTRL_MSI_ENABLE) != 0U)) { + (void)vmsi_remap(vdev, true); + } + } + + return 0; } /** diff --git a/hypervisor/dm/vpci/vmsix.c b/hypervisor/dm/vpci/vmsix.c index 777a1e6b4..71f8ccdba 100644 --- a/hypervisor/dm/vpci/vmsix.c +++ b/hypervisor/dm/vpci/vmsix.c @@ -168,51 +168,43 @@ static int32_t vmsix_remap_one_entry(const struct pci_vdev *vdev, uint32_t index */ int32_t vmsix_read_cfg(const struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t *val) { - int32_t ret = -ENODEV; /* For PIO access, we emulate Capability Structures only */ + *val = pci_vdev_read_cfg(vdev, offset, bytes); - if (msixcap_access(vdev, offset)) { - *val = pci_vdev_read_cfg(vdev, offset, bytes); - ret = 0; - } - - return ret; + return 0; } /** + * @brief Writing MSI-X Capability Structure + * * @pre vdev != NULL * @pre vdev->pdev != NULL */ int32_t vmsix_write_cfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t val) { uint32_t msgctrl; - int32_t ret = -ENODEV; - /* Writing MSI-X Capability Structure */ - if (msixcap_access(vdev, offset)) { - msgctrl = pci_vdev_read_cfg(vdev, vdev->msix.capoff + PCIR_MSIX_CTRL, 2U); + msgctrl = pci_vdev_read_cfg(vdev, vdev->msix.capoff + PCIR_MSIX_CTRL, 2U); - /* Write to vdev */ - pci_vdev_write_cfg(vdev, offset, bytes, val); + /* Write to vdev */ + pci_vdev_write_cfg(vdev, offset, bytes, val); - /* Writing Message Control field? */ - if ((offset - vdev->msix.capoff) == PCIR_MSIX_CTRL) { - if (((msgctrl ^ val) & PCIM_MSIXCTRL_MSIX_ENABLE) != 0U) { - if ((val & PCIM_MSIXCTRL_MSIX_ENABLE) != 0U) { - (void)vmsix_remap(vdev, true); - } else { - (void)vmsix_remap(vdev, false); - } - } - - if (((msgctrl ^ val) & PCIM_MSIXCTRL_FUNCTION_MASK) != 0U) { - pci_pdev_write_cfg(vdev->pdev->bdf, offset, 2U, val); + /* Writing Message Control field? */ + if ((offset - vdev->msix.capoff) == PCIR_MSIX_CTRL) { + if (((msgctrl ^ val) & PCIM_MSIXCTRL_MSIX_ENABLE) != 0U) { + if ((val & PCIM_MSIXCTRL_MSIX_ENABLE) != 0U) { + (void)vmsix_remap(vdev, true); + } else { + (void)vmsix_remap(vdev, false); } } - ret = 0; + + if (((msgctrl ^ val) & PCIM_MSIXCTRL_FUNCTION_MASK) != 0U) { + pci_pdev_write_cfg(vdev->pdev->bdf, offset, 2U, val); + } } - return ret; + return 0; } /** diff --git a/hypervisor/dm/vpci/vpci.c b/hypervisor/dm/vpci/vpci.c index 04d9dac00..3a51c1363 100644 --- a/hypervisor/dm/vpci/vpci.c +++ b/hypervisor/dm/vpci/vpci.c @@ -354,7 +354,7 @@ static void vpci_deinit_pt_dev(struct pci_vdev *vdev) static int32_t vpci_write_pt_dev_cfg(struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t val) { - if (vbar_access(vdev, offset, bytes)) { + if (vbar_access(vdev, offset)) { (void)vdev_pt_write_cfg(vdev, offset, bytes, val); } else if (msicap_access(vdev, offset)) { (void)vmsi_write_cfg(vdev, offset, bytes, val); @@ -371,7 +371,7 @@ static int32_t vpci_write_pt_dev_cfg(struct pci_vdev *vdev, uint32_t offset, static int32_t vpci_read_pt_dev_cfg(const struct pci_vdev *vdev, uint32_t offset, uint32_t bytes, uint32_t *val) { - if (vbar_access(vdev, offset, bytes)) { + if (vbar_access(vdev, offset)) { (void)vdev_pt_read_cfg(vdev, offset, bytes, val); } else if (msicap_access(vdev, offset)) { (void)vmsi_read_cfg(vdev, offset, bytes, val); diff --git a/hypervisor/dm/vpci/vpci_priv.h b/hypervisor/dm/vpci/vpci_priv.h index 2c4cda5cc..ff2fdd11c 100644 --- a/hypervisor/dm/vpci/vpci_priv.h +++ b/hypervisor/dm/vpci/vpci_priv.h @@ -104,9 +104,9 @@ static inline bool msixcap_access(const struct pci_vdev *vdev, uint32_t offset) /** * @pre vdev != NULL */ -static inline bool vbar_access(const struct pci_vdev *vdev, uint32_t offset, uint32_t bytes) +static inline bool vbar_access(const struct pci_vdev *vdev, uint32_t offset) { - return (is_bar_offset(vdev->nr_bars, offset) && (bytes == 4U) && ((offset & 0x3U) == 0U)); + return is_bar_offset(vdev->nr_bars, offset); } /** @@ -114,7 +114,7 @@ static inline bool vbar_access(const struct pci_vdev *vdev, uint32_t offset, uin */ static inline bool has_msi_cap(const struct pci_vdev *vdev) { - return (vdev->msi.capoff != 0U); + return (vdev->msi.capoff != 0U); } /** @@ -122,7 +122,7 @@ static inline bool has_msi_cap(const struct pci_vdev *vdev) */ static inline bool msicap_access(const struct pci_vdev *vdev, uint32_t offset) { - return (has_msi_cap(vdev) && in_range(offset, vdev->msi.capoff, vdev->msi.caplen)); + return (has_msi_cap(vdev) && in_range(offset, vdev->msi.capoff, vdev->msi.caplen)); } /**