Re: [PATCH-for-10.1 2/2] hw/pci-host/designware: Use deposit/extract API

[Gustavo Romero]

Hi Phil,

On 3/31/25 12:20, Philippe Mathieu-Daudé wrote:

Prefer the safer (less bug-prone) deposit/extract API
to access lower/upper 32-bit of 64-bit registers.

Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
---
  hw/pci-host/designware.c | 47 ++++++++++++++--------------------------
  1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 5598d18f478..3f2282b2596 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -22,6 +22,7 @@
  #include "qapi/error.h"
  #include "qemu/module.h"
  #include "qemu/log.h"
+#include "qemu/bitops.h"
  #include "hw/pci/msi.h"
  #include "hw/pci/pci_bridge.h"
  #include "hw/pci/pci_host.h"
@@ -162,11 +163,9 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t 
address, int len)
          break;

case DESIGNWARE_PCIE_MSI_ADDR_LO:

-        val = root->msi.base;
-        break;
-
      case DESIGNWARE_PCIE_MSI_ADDR_HI:
-        val = root->msi.base >> 32;
+        val = extract64(root->msi.base,
+                        address == DESIGNWARE_PCIE_MSI_ADDR_LO ? 0 : 32, 32);
          break;

case DESIGNWARE_PCIE_MSI_INTR0_ENABLE:

@@ -190,19 +189,15 @@ designware_pcie_root_config_read(PCIDevice *d, uint32_t 
address, int len)
          break;

case DESIGNWARE_PCIE_ATU_LOWER_BASE:

-        val = viewport->base;
-        break;
-
      case DESIGNWARE_PCIE_ATU_UPPER_BASE:
-        val = viewport->base >> 32;
+        val = extract64(viewport->base,
+                        address == DESIGNWARE_PCIE_ATU_LOWER_BASE ? 0 : 32, 
32);
          break;

case DESIGNWARE_PCIE_ATU_LOWER_TARGET:

-        val = viewport->target;
-        break;
-
      case DESIGNWARE_PCIE_ATU_UPPER_TARGET:
-        val = viewport->target >> 32;
+        val = extract64(viewport->target,
+                        address == DESIGNWARE_PCIE_ATU_LOWER_TARGET ? 0 : 32, 
32);
          break;

case DESIGNWARE_PCIE_ATU_LIMIT:

@@ -321,14 +316,10 @@ static void designware_pcie_root_config_write(PCIDevice 
*d, uint32_t address,
          break;

case DESIGNWARE_PCIE_MSI_ADDR_LO:

-        root->msi.base &= 0xFFFFFFFF00000000ULL;
-        root->msi.base |= val;
-        designware_pcie_root_update_msi_mapping(root);
-        break;
-
      case DESIGNWARE_PCIE_MSI_ADDR_HI:
-        root->msi.base &= 0x00000000FFFFFFFFULL;
-        root->msi.base |= (uint64_t)val << 32;
+        root->msi.base = deposit64(root->msi.base,
+                                   address == DESIGNWARE_PCIE_MSI_ADDR_LO
+                                   ? 0 : 32, 32, val);
          designware_pcie_root_update_msi_mapping(root);
          break;

@@ -355,23 +346,17 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,

          break;

case DESIGNWARE_PCIE_ATU_LOWER_BASE:

-        viewport->base &= 0xFFFFFFFF00000000ULL;
-        viewport->base |= val;
-        break;
-
      case DESIGNWARE_PCIE_ATU_UPPER_BASE:
-        viewport->base &= 0x00000000FFFFFFFFULL;
-        viewport->base |= (uint64_t)val << 32;
+        viewport->base = deposit64(root->msi.base,
+                                   address == DESIGNWARE_PCIE_ATU_LOWER_BASE
+                                   ? 0 : 32, 32, val);
          break;

case DESIGNWARE_PCIE_ATU_LOWER_TARGET:

-        viewport->target &= 0xFFFFFFFF00000000ULL;
-        viewport->target |= val;
-        break;
-
      case DESIGNWARE_PCIE_ATU_UPPER_TARGET:
-        viewport->target &= 0x00000000FFFFFFFFULL;
-        viewport->target |= (uint64_t)val << 32;
+        viewport->target = deposit64(root->msi.base,
+                                     address == 
DESIGNWARE_PCIE_ATU_LOWER_TARGET
+                                     ? 0 : 32, 32, val);
          break;

case DESIGNWARE_PCIE_ATU_LIMIT:

Reviewed-by: Gustavo Romero <gustavo.rom...@linaro.org>


Cheers,
Gustavo