Re: [PATCH v3 3/7] hw/ppc/spapr: Convert HPTE_VALID() macro as hpte_is_valid() method

[Nicholas Piggin]

On Thu Dec 19, 2024 at 4:21 AM AEST, Philippe Mathieu-Daudé wrote:
> Convert HPTE_VALID() macro as hpte_is_valid() method.
> Since sPAPR is in big endian configuration at reset,
> use the big endian LD/ST API to access the hash PTEs.

My knowlege of old platforms isn't great here, but I believe sPAPR
always has big-endian data structures in hardware and hypervisor
interfaces (which encompasses practically everything QEMU should be
concerned with). So patch is good, you could just reword the
changelog to make it clearer.

 sPAPR data structures including the hash page table are big-endian
 regardless of current CPU endian mode, so use the big-endian LD/ST
 API to access the hash PTEs.

>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ppc/spapr.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4845bf3244b..b67ab1ee685 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1406,7 +1406,11 @@ static uint64_t *hpte_get(SpaprMachineState *s, 
> unsigned index)
>      return &table[2 * index];
>  }
>  
> -#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> +static bool hpte_is_valid(SpaprMachineState *s, unsigned index)
> +{
> +    return ldq_be_p(hpte_get(s, index)) & HPTE64_V_VALID;
> +}

With the previous change to hpte_get_ptr(), here we could have a new
uint64_t hpte_get(s, index) helper that calculates the pointer and does
the load?

Otherwise,

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

> +
>  #define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & 
> HPTE64_V_HPTE_DIRTY)
>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= 
> tswap64(~HPTE64_V_HPTE_DIRTY))
>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= 
> tswap64(HPTE64_V_HPTE_DIRTY))
> @@ -2204,7 +2208,7 @@ static void htab_save_first_pass(QEMUFile *f, 
> SpaprMachineState *spapr,
>  
>          /* Consume invalid HPTEs */
>          while ((index < htabslots)
> -               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
> +               && !hpte_is_valid(spapr->htab, index)) {
>              CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>          }
> @@ -2212,7 +2216,7 @@ static void htab_save_first_pass(QEMUFile *f, 
> SpaprMachineState *spapr,
>          /* Consume valid HPTEs */
>          chunkstart = index;
>          while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
> -               && HPTE_VALID(hpte_get(spapr->htab, index))) {
> +               && hpte_is_valid(spapr->htab, index)) {
>              CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>          }
> @@ -2262,7 +2266,7 @@ static int htab_save_later_pass(QEMUFile *f, 
> SpaprMachineState *spapr,
>          /* Consume valid dirty HPTEs */
>          while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
>                 && HPTE_DIRTY(hpte_get(spapr->htab, index))
> -               && HPTE_VALID(hpte_get(spapr->htab, index))) {
> +               && hpte_is_valid(spapr->htab, index)) {
>              CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>              examined++;
> @@ -2272,7 +2276,7 @@ static int htab_save_later_pass(QEMUFile *f, 
> SpaprMachineState *spapr,
>          /* Consume invalid dirty HPTEs */
>          while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
>                 && HPTE_DIRTY(hpte_get(spapr->htab, index))
> -               && !HPTE_VALID(hpte_get(spapr->htab, index))) {
> +               && !hpte_is_valid(spapr->htab, index)) {
>              CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
>              examined++;