diff mbox series

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

Message ID 20250331152041.74533-3-philmd@linaro.org
State New
Headers show
Series hw/pci-host/designware: Fix ATU_UPPER_TARGET register access | expand

Commit Message

Philippe Mathieu-Daudé March 31, 2025, 3:20 p.m. UTC 
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é <philmd@linaro.org>
---
 hw/pci-host/designware.c | 47 ++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 31 deletions(-)

Comments

Gustavo Romero March 31, 2025, 4:16 p.m. UTC | #1 
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é <philmd@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.romero@linaro.org>


Cheers,
Gustavo
diff mbox series

Patch

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: