1. Patchew
  2. QEMU
  3. [PATCH v3 0/7] hw/ppc: Remove tswap() calls
  4. View patch

[PATCH v3 2/7] hw/ppc/spapr: Convert HPTE() macro as hpte_get() method

Philippe Mathieu-Daudé posted 7 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v3 2/7] hw/ppc/spapr: Convert HPTE() macro as hpte_get() method
Posted by Philippe Mathieu-Daudé 3 months, 3 weeks ago 
Convert HPTE() macro as hpte_get() method.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/spapr.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3b022e8da9e..4845bf3244b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1399,7 +1399,13 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
     }
 }
 
-#define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
+static uint64_t *hpte_get(SpaprMachineState *s, unsigned index)
+{
+    uint64_t *table = s->htab;
+
+    return &table[2 * index];
+}
+
 #define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
 #define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
 #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
@@ -1614,7 +1620,7 @@ int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp)
         spapr->htab_shift = shift;
 
         for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
-            DIRTY_HPTE(HPTE(spapr->htab, i));
+            DIRTY_HPTE(hpte_get(spapr->htab, i));
         }
     }
     /* We're setting up a hash table, so that means we're not radix */
@@ -2172,7 +2178,7 @@ static void htab_save_chunk(QEMUFile *f, SpaprMachineState *spapr,
     qemu_put_be32(f, chunkstart);
     qemu_put_be16(f, n_valid);
     qemu_put_be16(f, n_invalid);
-    qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
+    qemu_put_buffer(f, (void *)hpte_get(spapr->htab, chunkstart),
                     HASH_PTE_SIZE_64 * n_valid);
 }
 
@@ -2198,16 +2204,16 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
 
         /* Consume invalid HPTEs */
         while ((index < htabslots)
-               && !HPTE_VALID(HPTE(spapr->htab, index))) {
-            CLEAN_HPTE(HPTE(spapr->htab, index));
+               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
+            CLEAN_HPTE(hpte_get(spapr->htab, index));
             index++;
         }
 
         /* Consume valid HPTEs */
         chunkstart = index;
         while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
-               && HPTE_VALID(HPTE(spapr->htab, index))) {
-            CLEAN_HPTE(HPTE(spapr->htab, index));
+               && HPTE_VALID(hpte_get(spapr->htab, index))) {
+            CLEAN_HPTE(hpte_get(spapr->htab, index));
             index++;
         }
 
@@ -2247,7 +2253,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
 
         /* Consume non-dirty HPTEs */
         while ((index < htabslots)
-               && !HPTE_DIRTY(HPTE(spapr->htab, index))) {
+               && !HPTE_DIRTY(hpte_get(spapr->htab, index))) {
             index++;
             examined++;
         }
@@ -2255,9 +2261,9 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
         chunkstart = index;
         /* Consume valid dirty HPTEs */
         while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
-               && HPTE_DIRTY(HPTE(spapr->htab, index))
-               && HPTE_VALID(HPTE(spapr->htab, index))) {
-            CLEAN_HPTE(HPTE(spapr->htab, index));
+               && HPTE_DIRTY(hpte_get(spapr->htab, index))
+               && HPTE_VALID(hpte_get(spapr->htab, index))) {
+            CLEAN_HPTE(hpte_get(spapr->htab, index));
             index++;
             examined++;
         }
@@ -2265,9 +2271,9 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
         invalidstart = index;
         /* Consume invalid dirty HPTEs */
         while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
-               && HPTE_DIRTY(HPTE(spapr->htab, index))
-               && !HPTE_VALID(HPTE(spapr->htab, index))) {
-            CLEAN_HPTE(HPTE(spapr->htab, index));
+               && HPTE_DIRTY(hpte_get(spapr->htab, index))
+               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
+            CLEAN_HPTE(hpte_get(spapr->htab, index));
             index++;
             examined++;
         }
@@ -2449,11 +2455,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
 
         if (spapr->htab) {
             if (n_valid) {
-                qemu_get_buffer(f, HPTE(spapr->htab, index),
+                qemu_get_buffer(f, (void *)hpte_get(spapr->htab, index),
                                 HASH_PTE_SIZE_64 * n_valid);
             }
             if (n_invalid) {
-                memset(HPTE(spapr->htab, index + n_valid), 0,
+                memset(hpte_get(spapr->htab, index + n_valid), 0,
                        HASH_PTE_SIZE_64 * n_invalid);
             }
         } else {
-- 
2.45.2
Re: [PATCH v3 2/7] hw/ppc/spapr: Convert HPTE() macro as hpte_get() method
Posted by Harsh Prateek Bora 3 months, 2 weeks ago 
Hi Philippe,

On 12/18/24 23:51, Philippe Mathieu-Daudé wrote:
> Convert HPTE() macro as hpte_get() method.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/spapr.c | 38 ++++++++++++++++++++++----------------
>   1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3b022e8da9e..4845bf3244b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1399,7 +1399,13 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
>       }
>   }
>   
> -#define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
> +static uint64_t *hpte_get(SpaprMachineState *s, unsigned index)
> +{
> +    uint64_t *table = s->htab;
> +
> +    return &table[2 * index];
> +}
> +
>   #define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
>   #define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
>   #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> @@ -1614,7 +1620,7 @@ int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp)
>           spapr->htab_shift = shift;
>   
>           for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
> -            DIRTY_HPTE(HPTE(spapr->htab, i));
> +            DIRTY_HPTE(hpte_get(spapr->htab, i));

Prev HPTE expected _table i.e. spapr->htab as arg, but this new hpte_get
expects SpaprMachineState* i.e. spapr as arg. Shouldn't the arg be
updated accordingly? Wondering it didnt complain during build.

As Nick suggested, hpte_get_ptr or get_hpte_ptr may be better renaming.

regards,
Harsh

>           }
>       }
>       /* We're setting up a hash table, so that means we're not radix */
> @@ -2172,7 +2178,7 @@ static void htab_save_chunk(QEMUFile *f, SpaprMachineState *spapr,
>       qemu_put_be32(f, chunkstart);
>       qemu_put_be16(f, n_valid);
>       qemu_put_be16(f, n_invalid);
> -    qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> +    qemu_put_buffer(f, (void *)hpte_get(spapr->htab, chunkstart),
>                       HASH_PTE_SIZE_64 * n_valid);
>   }
>   
> @@ -2198,16 +2204,16 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>   
>           /* Consume invalid HPTEs */
>           while ((index < htabslots)
> -               && !HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>               index++;
>           }
>   
>           /* Consume valid HPTEs */
>           chunkstart = index;
>           while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
> -               && HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>               index++;
>           }
>   
> @@ -2247,7 +2253,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>   
>           /* Consume non-dirty HPTEs */
>           while ((index < htabslots)
> -               && !HPTE_DIRTY(HPTE(spapr->htab, index))) {
> +               && !HPTE_DIRTY(hpte_get(spapr->htab, index))) {
>               index++;
>               examined++;
>           }
> @@ -2255,9 +2261,9 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>           chunkstart = index;
>           /* Consume valid dirty HPTEs */
>           while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
> -               && HPTE_DIRTY(HPTE(spapr->htab, index))
> -               && HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && HPTE_DIRTY(hpte_get(spapr->htab, index))
> +               && HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>               index++;
>               examined++;
>           }
> @@ -2265,9 +2271,9 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>           invalidstart = index;
>           /* Consume invalid dirty HPTEs */
>           while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
> -               && HPTE_DIRTY(HPTE(spapr->htab, index))
> -               && !HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && HPTE_DIRTY(hpte_get(spapr->htab, index))
> +               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>               index++;
>               examined++;
>           }
> @@ -2449,11 +2455,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>   
>           if (spapr->htab) {
>               if (n_valid) {
> -                qemu_get_buffer(f, HPTE(spapr->htab, index),
> +                qemu_get_buffer(f, (void *)hpte_get(spapr->htab, index),
>                                   HASH_PTE_SIZE_64 * n_valid);
>               }
>               if (n_invalid) {
> -                memset(HPTE(spapr->htab, index + n_valid), 0,
> +                memset(hpte_get(spapr->htab, index + n_valid), 0,
>                          HASH_PTE_SIZE_64 * n_invalid);
>               }
>           } else {
Re: [PATCH v3 2/7] hw/ppc/spapr: Convert HPTE() macro as hpte_get() method
Posted by Philippe Mathieu-Daudé 3 months, 2 weeks ago 
On 19/12/24 07:31, Harsh Prateek Bora wrote:
> Hi Philippe,
> 
> On 12/18/24 23:51, Philippe Mathieu-Daudé wrote:
>> Convert HPTE() macro as hpte_get() method.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/ppc/spapr.c | 38 ++++++++++++++++++++++----------------
>>   1 file changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 3b022e8da9e..4845bf3244b 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1399,7 +1399,13 @@ static bool spapr_get_pate(PPCVirtualHypervisor 
>> *vhyp, PowerPCCPU *cpu,
>>       }
>>   }
>> -#define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
>> +static uint64_t *hpte_get(SpaprMachineState *s, unsigned index)
>> +{
>> +    uint64_t *table = s->htab;
>> +
>> +    return &table[2 * index];
>> +}
>> +
>>   #define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & 
>> HPTE64_V_VALID)
>>   #define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & 
>> HPTE64_V_HPTE_DIRTY)
>>   #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= 
>> tswap64(~HPTE64_V_HPTE_DIRTY))
>> @@ -1614,7 +1620,7 @@ int spapr_reallocate_hpt(SpaprMachineState 
>> *spapr, int shift, Error **errp)
>>           spapr->htab_shift = shift;
>>           for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
>> -            DIRTY_HPTE(HPTE(spapr->htab, i));
>> +            DIRTY_HPTE(hpte_get(spapr->htab, i));
> 
> Prev HPTE expected _table i.e. spapr->htab as arg, but this new hpte_get
> expects SpaprMachineState* i.e. spapr as arg. Shouldn't the arg be
> updated accordingly?

Good catch!

> Wondering it didnt complain during build.

Because the macros blindly cast, dropping the type checks.

> 
> As Nick suggested, hpte_get_ptr or get_hpte_ptr may be better renaming.

Sure. Thanks!

> 
> regards,
> Harsh
Re: [PATCH v3 2/7] hw/ppc/spapr: Convert HPTE() macro as hpte_get() method
Posted by Nicholas Piggin 3 months, 3 weeks ago 
On Thu Dec 19, 2024 at 4:21 AM AEST, Philippe Mathieu-Daudé wrote:
> Convert HPTE() macro as hpte_get() method.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Nitpick, could we call this hpte_ptr() or hpte_get_ptr()?

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  hw/ppc/spapr.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3b022e8da9e..4845bf3244b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1399,7 +1399,13 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
>      }
>  }
>  
> -#define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
> +static uint64_t *hpte_get(SpaprMachineState *s, unsigned index)
> +{
> +    uint64_t *table = s->htab;
> +
> +    return &table[2 * index];
> +}
> +
>  #define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
>  #define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> @@ -1614,7 +1620,7 @@ int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp)
>          spapr->htab_shift = shift;
>  
>          for (i = 0; i < size / HASH_PTE_SIZE_64; i++) {
> -            DIRTY_HPTE(HPTE(spapr->htab, i));
> +            DIRTY_HPTE(hpte_get(spapr->htab, i));
>          }
>      }
>      /* We're setting up a hash table, so that means we're not radix */
> @@ -2172,7 +2178,7 @@ static void htab_save_chunk(QEMUFile *f, SpaprMachineState *spapr,
>      qemu_put_be32(f, chunkstart);
>      qemu_put_be16(f, n_valid);
>      qemu_put_be16(f, n_invalid);
> -    qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> +    qemu_put_buffer(f, (void *)hpte_get(spapr->htab, chunkstart),
>                      HASH_PTE_SIZE_64 * n_valid);
>  }
>  
> @@ -2198,16 +2204,16 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>  
>          /* Consume invalid HPTEs */
>          while ((index < htabslots)
> -               && !HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>          }
>  
>          /* Consume valid HPTEs */
>          chunkstart = index;
>          while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
> -               && HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>          }
>  
> @@ -2247,7 +2253,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>  
>          /* Consume non-dirty HPTEs */
>          while ((index < htabslots)
> -               && !HPTE_DIRTY(HPTE(spapr->htab, index))) {
> +               && !HPTE_DIRTY(hpte_get(spapr->htab, index))) {
>              index++;
>              examined++;
>          }
> @@ -2255,9 +2261,9 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>          chunkstart = index;
>          /* Consume valid dirty HPTEs */
>          while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
> -               && HPTE_DIRTY(HPTE(spapr->htab, index))
> -               && HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && HPTE_DIRTY(hpte_get(spapr->htab, index))
> +               && HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>              examined++;
>          }
> @@ -2265,9 +2271,9 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>          invalidstart = index;
>          /* Consume invalid dirty HPTEs */
>          while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
> -               && HPTE_DIRTY(HPTE(spapr->htab, index))
> -               && !HPTE_VALID(HPTE(spapr->htab, index))) {
> -            CLEAN_HPTE(HPTE(spapr->htab, index));
> +               && HPTE_DIRTY(hpte_get(spapr->htab, index))
> +               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
> +            CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>              examined++;
>          }
> @@ -2449,11 +2455,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>  
>          if (spapr->htab) {
>              if (n_valid) {
> -                qemu_get_buffer(f, HPTE(spapr->htab, index),
> +                qemu_get_buffer(f, (void *)hpte_get(spapr->htab, index),
>                                  HASH_PTE_SIZE_64 * n_valid);
>              }
>              if (n_invalid) {
> -                memset(HPTE(spapr->htab, index + n_valid), 0,
> +                memset(hpte_get(spapr->htab, index + n_valid), 0,
>                         HASH_PTE_SIZE_64 * n_invalid);
>              }
>          } else {

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.