[PATCH v3 0/7] hw/ppc: Remove tswap() calls

[PATCH v3 0/7] hw/ppc: Remove tswap() calls

[Philippe Mathieu-Daudé]

Remove the tswap() calls on ePAPR, and convert
them to big-endian LD/ST API on sPAPR.

Build-tested only.

Philippe Mathieu-Daudé (7):
  meson: Run some compiler checks using -Wno-unused-value
  hw/ppc/spapr: Convert HPTE() macro as hpte_get() method
  hw/ppc/spapr: Convert HPTE_VALID() macro as hpte_is_valid() method
  hw/ppc/spapr: Convert HPTE_DIRTY() macro as hpte_is_dirty() method
  hw/ppc/spapr: Convert CLEAN_HPTE() macro as hpte_set_clean() method
  hw/ppc/spapr: Convert DIRTY_HPTE() macro as hpte_set_dirty() method
  hw/ppc/epapr: Do not swap ePAPR magic value

 meson.build           | 11 +++++---
 hw/ppc/sam460ex.c     |  2 +-
 hw/ppc/spapr.c        | 63 +++++++++++++++++++++++++++++--------------
 hw/ppc/virtex_ml507.c |  2 +-
 4 files changed, 52 insertions(+), 26 deletions(-)

-- 
2.45.2

[PATCH v3 1/7] meson: Run some compiler checks using -Wno-unused-value

[Philippe Mathieu-Daudé]

When running Clang static analyzer on macOS I'm getting:

  include/qemu/osdep.h:634:8: error: redefinition of 'iovec'
    634 | struct iovec {
        |        ^
  /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/_types/_iovec_t.h:31:8: note: previous definition is here
     31 | struct iovec {
        |        ^
  1 error generated.

Looking at meson-logs.txt, the analyzer enables -Wunused-value
making meson generated code to fail:

    Code:
    #include <sys/uio.h>
            void bar(void) {
                sizeof(struct iovec);
            }
    -----------
    stderr:
    meson-private/tmpe8_1b_00/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value]
        3 |             sizeof(struct iovec);
          |             ^~~~~~~~~~~~~~~~~~~~
    1 error generated.
    -----------
    Checking for type "struct iovec" : NO

    Code:
    #include <utmpx.h>
            void bar(void) {
                sizeof(struct utmpx);
            }
    -----------
    stderr:
    meson-private/tmp3n0u490p/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value]
        3 |             sizeof(struct utmpx);
          |             ^~~~~~~~~~~~~~~~~~~~
    1 error generated.
    -----------
    Checking for type "struct utmpx" : NO

    Code:

            #include <getopt.h>
            int main(void) {
                /* If it's not defined as a macro, try to use as a symbol */
                #ifndef optreset
                    optreset;
                #endif
                return 0;
            }
    -----------
    stderr:
    meson-private/tmp1rzob_os/testfile.c:6:17: error: expression result unused [-Werror,-Wunused-value]
        6 |                 optreset;
          |                 ^~~~~~~~
    1 error generated.
    -----------
    Header "getopt.h" has symbol "optreset" : NO

    Code:

            #include <vmnet/vmnet.h>
            int main(void) {
                /* If it's not defined as a macro, try to use as a symbol */
                #ifndef VMNET_BRIDGED_MODE
                    VMNET_BRIDGED_MODE;
                #endif
                return 0;
            }
    -----------
    stderr:
    meson-private/tmpl9jgsxpt/testfile.c:6:17: error: expression result unused [-Werror,-Wunused-value]
        6 |                 VMNET_BRIDGED_MODE;
          |                 ^~~~~~~~~~~~~~~~~~
    1 error generated.
    -----------
    Header "vmnet/vmnet.h" has symbol "VMNET_BRIDGED_MODE" with dependency appleframeworks: NO
    ../meson.build:1174: WARNING: vmnet.framework API is outdated, disabling

Fix by explicitly disabling -Wunused-value from these meson checks.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Probably meson should do that in has_header_symbol() / has_type()?
---
 meson.build | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index 85f74854735..9d93dcd95d7 100644
--- a/meson.build
+++ b/meson.build
@@ -1189,7 +1189,8 @@ cocoa = dependency('appleframeworks',
 vmnet = dependency('appleframeworks', modules: 'vmnet', required: get_option('vmnet'))
 if vmnet.found() and not cc.has_header_symbol('vmnet/vmnet.h',
                                               'VMNET_BRIDGED_MODE',
-                                              dependencies: vmnet)
+                                              dependencies: vmnet,
+                                              args: '-Wno-unused-value')
   vmnet = not_found
   if get_option('vmnet').enabled()
     error('vmnet.framework API is outdated')
@@ -2713,7 +2714,7 @@ config_host_data.set('CONFIG_RTNETLINK',
 config_host_data.set('CONFIG_SYSMACROS',
                      cc.has_header_symbol('sys/sysmacros.h', 'makedev'))
 config_host_data.set('HAVE_OPTRESET',
-                     cc.has_header_symbol('getopt.h', 'optreset'))
+                     cc.has_header_symbol('getopt.h', 'optreset', args: '-Wno-unused-value'))
 config_host_data.set('HAVE_IPPROTO_MPTCP',
                      cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP'))
 
@@ -2731,10 +2732,12 @@ config_host_data.set('HAVE_BLK_ZONE_REP_CAPACITY',
 # has_type
 config_host_data.set('CONFIG_IOVEC',
                      cc.has_type('struct iovec',
-                                 prefix: '#include <sys/uio.h>'))
+                                 prefix: '#include <sys/uio.h>',
+                                 args: '-Wno-unused-value'))
 config_host_data.set('HAVE_UTMPX',
                      cc.has_type('struct utmpx',
-                                 prefix: '#include <utmpx.h>'))
+                                 prefix: '#include <utmpx.h>',
+                                 args: '-Wno-unused-value'))
 
 config_host_data.set('CONFIG_EVENTFD', cc.links('''
   #include <sys/eventfd.h>
-- 
2.45.2

Re: [PATCH v3 1/7] meson: Run some compiler checks using -Wno-unused-value

[Nicholas Piggin]

On Thu Dec 19, 2024 at 4:21 AM AEST, Philippe Mathieu-Daudé wrote:
> When running Clang static analyzer on macOS I'm getting:
>
>   include/qemu/osdep.h:634:8: error: redefinition of 'iovec'
>     634 | struct iovec {
>         |        ^
>   /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/_types/_iovec_t.h:31:8: note: previous definition is here
>      31 | struct iovec {
>         |        ^
>   1 error generated.
>
> Looking at meson-logs.txt, the analyzer enables -Wunused-value
> making meson generated code to fail:
>
>     Code:
>     #include <sys/uio.h>
>             void bar(void) {
>                 sizeof(struct iovec);
>             }
>     -----------
>     stderr:
>     meson-private/tmpe8_1b_00/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value]
>         3 |             sizeof(struct iovec);
>           |             ^~~~~~~~~~~~~~~~~~~~
>     1 error generated.
>     -----------
>     Checking for type "struct iovec" : NO
>
>     Code:
>     #include <utmpx.h>
>             void bar(void) {
>                 sizeof(struct utmpx);
>             }
>     -----------
>     stderr:
>     meson-private/tmp3n0u490p/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value]
>         3 |             sizeof(struct utmpx);
>           |             ^~~~~~~~~~~~~~~~~~~~
>     1 error generated.
>     -----------
>     Checking for type "struct utmpx" : NO
>
>     Code:
>
>             #include <getopt.h>
>             int main(void) {
>                 /* If it's not defined as a macro, try to use as a symbol */
>                 #ifndef optreset
>                     optreset;
>                 #endif
>                 return 0;
>             }
>     -----------
>     stderr:
>     meson-private/tmp1rzob_os/testfile.c:6:17: error: expression result unused [-Werror,-Wunused-value]
>         6 |                 optreset;
>           |                 ^~~~~~~~
>     1 error generated.
>     -----------
>     Header "getopt.h" has symbol "optreset" : NO
>
>     Code:
>
>             #include <vmnet/vmnet.h>
>             int main(void) {
>                 /* If it's not defined as a macro, try to use as a symbol */
>                 #ifndef VMNET_BRIDGED_MODE
>                     VMNET_BRIDGED_MODE;
>                 #endif
>                 return 0;
>             }
>     -----------
>     stderr:
>     meson-private/tmpl9jgsxpt/testfile.c:6:17: error: expression result unused [-Werror,-Wunused-value]
>         6 |                 VMNET_BRIDGED_MODE;
>           |                 ^~~~~~~~~~~~~~~~~~
>     1 error generated.
>     -----------
>     Header "vmnet/vmnet.h" has symbol "VMNET_BRIDGED_MODE" with dependency appleframeworks: NO
>     ../meson.build:1174: WARNING: vmnet.framework API is outdated, disabling
>
> Fix by explicitly disabling -Wunused-value from these meson checks.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Probably meson should do that in has_header_symbol() / has_type()?

I don't know about the build system to answer this, but should we
instead disable -Werror on these tests to be a bit more future-proof?
Compilers often add new warnings or catch more cases of existing
warnings.

Alternative would be to keep -Werror but fail the build if a test
throws a warning, but that seems like a lot more work for little
benefit...

Thanks,
Nick

> ---
>  meson.build | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 85f74854735..9d93dcd95d7 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1189,7 +1189,8 @@ cocoa = dependency('appleframeworks',
>  vmnet = dependency('appleframeworks', modules: 'vmnet', required: get_option('vmnet'))
>  if vmnet.found() and not cc.has_header_symbol('vmnet/vmnet.h',
>                                                'VMNET_BRIDGED_MODE',
> -                                              dependencies: vmnet)
> +                                              dependencies: vmnet,
> +                                              args: '-Wno-unused-value')
>    vmnet = not_found
>    if get_option('vmnet').enabled()
>      error('vmnet.framework API is outdated')
> @@ -2713,7 +2714,7 @@ config_host_data.set('CONFIG_RTNETLINK',
>  config_host_data.set('CONFIG_SYSMACROS',
>                       cc.has_header_symbol('sys/sysmacros.h', 'makedev'))
>  config_host_data.set('HAVE_OPTRESET',
> -                     cc.has_header_symbol('getopt.h', 'optreset'))
> +                     cc.has_header_symbol('getopt.h', 'optreset', args: '-Wno-unused-value'))
>  config_host_data.set('HAVE_IPPROTO_MPTCP',
>                       cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP'))
>  
> @@ -2731,10 +2732,12 @@ config_host_data.set('HAVE_BLK_ZONE_REP_CAPACITY',
>  # has_type
>  config_host_data.set('CONFIG_IOVEC',
>                       cc.has_type('struct iovec',
> -                                 prefix: '#include <sys/uio.h>'))
> +                                 prefix: '#include <sys/uio.h>',
> +                                 args: '-Wno-unused-value'))
>  config_host_data.set('HAVE_UTMPX',
>                       cc.has_type('struct utmpx',
> -                                 prefix: '#include <utmpx.h>'))
> +                                 prefix: '#include <utmpx.h>',
> +                                 args: '-Wno-unused-value'))
>  
>  config_host_data.set('CONFIG_EVENTFD', cc.links('''
>    #include <sys/eventfd.h>

Re: [PATCH v3 1/7] meson: Run some compiler checks using -Wno-unused-value

[Philippe Mathieu-Daudé]

On 19/12/24 01:37, Nicholas Piggin wrote:
> On Thu Dec 19, 2024 at 4:21 AM AEST, Philippe Mathieu-Daudé wrote:
>> When running Clang static analyzer on macOS I'm getting:
>>
>>    include/qemu/osdep.h:634:8: error: redefinition of 'iovec'
>>      634 | struct iovec {
>>          |        ^
>>    /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/_types/_iovec_t.h:31:8: note: previous definition is here
>>       31 | struct iovec {
>>          |        ^
>>    1 error generated.
>>
>> Looking at meson-logs.txt, the analyzer enables -Wunused-value
>> making meson generated code to fail:
>>
>>      Code:
>>      #include <sys/uio.h>
>>              void bar(void) {
>>                  sizeof(struct iovec);
>>              }
>>      -----------
>>      stderr:
>>      meson-private/tmpe8_1b_00/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value]
>>          3 |             sizeof(struct iovec);
>>            |             ^~~~~~~~~~~~~~~~~~~~
>>      1 error generated.
>>      -----------
>>      Checking for type "struct iovec" : NO
>>
>>      Code:
>>      #include <utmpx.h>
>>              void bar(void) {
>>                  sizeof(struct utmpx);
>>              }
>>      -----------
>>      stderr:
>>      meson-private/tmp3n0u490p/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value]
>>          3 |             sizeof(struct utmpx);
>>            |             ^~~~~~~~~~~~~~~~~~~~
>>      1 error generated.
>>      -----------
>>      Checking for type "struct utmpx" : NO
>>
>>      Code:
>>
>>              #include <getopt.h>
>>              int main(void) {
>>                  /* If it's not defined as a macro, try to use as a symbol */
>>                  #ifndef optreset
>>                      optreset;
>>                  #endif
>>                  return 0;
>>              }
>>      -----------
>>      stderr:
>>      meson-private/tmp1rzob_os/testfile.c:6:17: error: expression result unused [-Werror,-Wunused-value]
>>          6 |                 optreset;
>>            |                 ^~~~~~~~
>>      1 error generated.
>>      -----------
>>      Header "getopt.h" has symbol "optreset" : NO
>>
>>      Code:
>>
>>              #include <vmnet/vmnet.h>
>>              int main(void) {
>>                  /* If it's not defined as a macro, try to use as a symbol */
>>                  #ifndef VMNET_BRIDGED_MODE
>>                      VMNET_BRIDGED_MODE;
>>                  #endif
>>                  return 0;
>>              }
>>      -----------
>>      stderr:
>>      meson-private/tmpl9jgsxpt/testfile.c:6:17: error: expression result unused [-Werror,-Wunused-value]
>>          6 |                 VMNET_BRIDGED_MODE;
>>            |                 ^~~~~~~~~~~~~~~~~~
>>      1 error generated.
>>      -----------
>>      Header "vmnet/vmnet.h" has symbol "VMNET_BRIDGED_MODE" with dependency appleframeworks: NO
>>      ../meson.build:1174: WARNING: vmnet.framework API is outdated, disabling
>>
>> Fix by explicitly disabling -Wunused-value from these meson checks.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> RFC: Probably meson should do that in has_header_symbol() / has_type()?
> 
> I don't know about the build system to answer this, but should we
> instead disable -Werror on these tests to be a bit more future-proof?
> Compilers often add new warnings or catch more cases of existing
> warnings.

Sorry, I didn't mean to include this patch in this series. I happen
to have my series on top of it and forgot to change the base commit.

> Alternative would be to keep -Werror but fail the build if a test
> throws a warning, but that seems like a lot more work for little
> benefit...

I'm trying to fix it on the meson side with this:

-- >8 --
diff --git a/mesonbuild/compilers/mixins/clike.py 
b/mesonbuild/compilers/mixins/clike.py
index d56547b47..9d6957973 100644
--- a/mesonbuild/compilers/mixins/clike.py
+++ b/mesonbuild/compilers/mixins/clike.py
@@ -360,7 +360,7 @@ class CLikeCompiler(Compiler):
          int main(void) {{
              /* If it's not defined as a macro, try to use as a symbol */
              #ifndef {symbol}
-                {symbol};
+            (void) {symbol};
              #endif
              return 0;
          }}'''
@@ -885,7 +885,8 @@ class CLikeCompiler(Compiler):
                   dependencies: T.Optional[T.List['Dependency']] = 
None) -> T.Tuple[bool, bool]:
          t = f'''{prefix}
          void bar(void) {{
-            (void) sizeof({typename});
+            size_t foo = sizeof({typename});
+            (void) foo;
          }}'''
          return self.compiles(t, env, extra_args=extra_args,
                               dependencies=dependencies)
---

Re: [PATCH v3 1/7] meson: Run some compiler checks using -Wno-unused-value

[Richard Henderson]

On 12/19/24 09:39, Philippe Mathieu-Daudé wrote:
> @@ -885,7 +885,8 @@ class CLikeCompiler(Compiler):
>                    dependencies: T.Optional[T.List['Dependency']] = None) -> T.Tuple[bool, 
> bool]:
>           t = f'''{prefix}
>           void bar(void) {{
> -            (void) sizeof({typename});
> +            size_t foo = sizeof({typename});
> +            (void) foo;
>           }}'''

As I mentioned elsewhere, sizeof is a compile-time constant.
The function wrapper is getting in the way.  This can be just

unsigned long foo = sizeof({typename});

I.e. initialization of a global variable, with no need for <stddef.h> or any other system 
header for size_t.


r~

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

[Philippe Mathieu-Daudé]

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

[Nicholas Piggin]

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 {

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

[Harsh Prateek Bora]

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

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.

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;
+}
+
 #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++;
-- 
2.45.2

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++;

[PATCH v3 4/7] hw/ppc/spapr: Convert HPTE_DIRTY() macro as hpte_is_dirty() method

[Philippe Mathieu-Daudé]

Convert HPTE_DIRTY() macro as hpte_is_dirty() method.
Since sPAPR is in big endian configuration at reset,
use the big endian LD/ST API to access the HPTEs.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b67ab1ee685..5bc49598a97 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1411,7 +1411,11 @@ static bool hpte_is_valid(SpaprMachineState *s, unsigned index)
     return ldq_be_p(hpte_get(s, index)) & HPTE64_V_VALID;
 }
 
-#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
+static bool hpte_is_dirty(SpaprMachineState *s, unsigned index)
+{
+    return ldq_be_p(hpte_get(s, index)) & 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))
 
@@ -2257,7 +2261,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
 
         /* Consume non-dirty HPTEs */
         while ((index < htabslots)
-               && !HPTE_DIRTY(hpte_get(spapr->htab, index))) {
+               && !hpte_is_dirty(spapr->htab, index)) {
             index++;
             examined++;
         }
@@ -2265,7 +2269,7 @@ 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_get(spapr->htab, index))
+               && hpte_is_dirty(spapr->htab, index)
                && hpte_is_valid(spapr->htab, index)) {
             CLEAN_HPTE(hpte_get(spapr->htab, index));
             index++;
@@ -2275,7 +2279,7 @@ 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_get(spapr->htab, index))
+               && hpte_is_dirty(spapr->htab, index)
                && !hpte_is_valid(spapr->htab, index)) {
             CLEAN_HPTE(hpte_get(spapr->htab, index));
             index++;
-- 
2.45.2

Re: [PATCH v3 4/7] hw/ppc/spapr: Convert HPTE_DIRTY() macro as hpte_is_dirty() method

[Nicholas Piggin]

On Thu Dec 19, 2024 at 4:21 AM AEST, Philippe Mathieu-Daudé wrote:
> Convert HPTE_DIRTY() macro as hpte_is_dirty() method.
> Since sPAPR is in big endian configuration at reset,
> use the big endian LD/ST API to access the HPTEs.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

All the other helper changes look good +/- suggestions from the
first one.

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

> ---
>  hw/ppc/spapr.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b67ab1ee685..5bc49598a97 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1411,7 +1411,11 @@ static bool hpte_is_valid(SpaprMachineState *s, unsigned index)
>      return ldq_be_p(hpte_get(s, index)) & HPTE64_V_VALID;
>  }
>  
> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> +static bool hpte_is_dirty(SpaprMachineState *s, unsigned index)
> +{
> +    return ldq_be_p(hpte_get(s, index)) & 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))
>  
> @@ -2257,7 +2261,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>  
>          /* Consume non-dirty HPTEs */
>          while ((index < htabslots)
> -               && !HPTE_DIRTY(hpte_get(spapr->htab, index))) {
> +               && !hpte_is_dirty(spapr->htab, index)) {
>              index++;
>              examined++;
>          }
> @@ -2265,7 +2269,7 @@ 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_get(spapr->htab, index))
> +               && hpte_is_dirty(spapr->htab, index)
>                 && hpte_is_valid(spapr->htab, index)) {
>              CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;
> @@ -2275,7 +2279,7 @@ 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_get(spapr->htab, index))
> +               && hpte_is_dirty(spapr->htab, index)
>                 && !hpte_is_valid(spapr->htab, index)) {
>              CLEAN_HPTE(hpte_get(spapr->htab, index));
>              index++;

Re: [PATCH v3 4/7] hw/ppc/spapr: Convert HPTE_DIRTY() macro as hpte_is_dirty() method

[Harsh Prateek Bora]

Hi Philippe,

Similar issue here as with patch 2 ..

On 12/18/24 23:51, Philippe Mathieu-Daudé wrote:
> Convert HPTE_DIRTY() macro as hpte_is_dirty() method.
> Since sPAPR is in big endian configuration at reset,
> use the big endian LD/ST API to access the HPTEs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/spapr.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b67ab1ee685..5bc49598a97 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1411,7 +1411,11 @@ static bool hpte_is_valid(SpaprMachineState *s, unsigned index)
>       return ldq_be_p(hpte_get(s, index)) & HPTE64_V_VALID;
>   }
>   
> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_HPTE_DIRTY)
> +static bool hpte_is_dirty(SpaprMachineState *s, unsigned index)
> +{
> +    return ldq_be_p(hpte_get(s, index)) & 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))
>   
> @@ -2257,7 +2261,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>   
>           /* Consume non-dirty HPTEs */
>           while ((index < htabslots)
> -               && !HPTE_DIRTY(hpte_get(spapr->htab, index))) {
> +               && !hpte_is_dirty(spapr->htab, index)) {

hpte_is_dirty expects SpaprMachineState * as arg, need to update 
accordingly.

regards,
Harsh
>               index++;
>               examined++;
>           }
> @@ -2265,7 +2269,7 @@ 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_get(spapr->htab, index))
> +               && hpte_is_dirty(spapr->htab, index)
>                  && hpte_is_valid(spapr->htab, index)) {
>               CLEAN_HPTE(hpte_get(spapr->htab, index));
>               index++;
> @@ -2275,7 +2279,7 @@ 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_get(spapr->htab, index))
> +               && hpte_is_dirty(spapr->htab, index)
>                  && !hpte_is_valid(spapr->htab, index)) {
>               CLEAN_HPTE(hpte_get(spapr->htab, index));
>               index++;

[PATCH v3 5/7] hw/ppc/spapr: Convert CLEAN_HPTE() macro as hpte_set_clean() method

[Philippe Mathieu-Daudé]

Convert CLEAN_HPTE() macro as hpte_set_clean() method.
Since sPAPR is in big endian configuration at reset,
use the big endian LD/ST API to access the HPTEs.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5bc49598a97..4e1fe832c29 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1416,7 +1416,12 @@ static bool hpte_is_dirty(SpaprMachineState *s, unsigned index)
     return ldq_be_p(hpte_get(s, index)) & HPTE64_V_HPTE_DIRTY;
 }
 
-#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
+static void hpte_set_clean(SpaprMachineState *s, unsigned index)
+{
+    stq_be_p(hpte_get(s, index),
+             ldq_be_p(hpte_get(s, index)) & ~HPTE64_V_HPTE_DIRTY);
+}
+
 #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
 
 /*
@@ -2213,7 +2218,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
         /* Consume invalid HPTEs */
         while ((index < htabslots)
                && !hpte_is_valid(spapr->htab, index)) {
-            CLEAN_HPTE(hpte_get(spapr->htab, index));
+            hpte_set_clean(spapr->htab, index);
             index++;
         }
 
@@ -2221,7 +2226,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
         chunkstart = index;
         while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
                && hpte_is_valid(spapr->htab, index)) {
-            CLEAN_HPTE(hpte_get(spapr->htab, index));
+            hpte_set_clean(spapr->htab, index);
             index++;
         }
 
@@ -2271,7 +2276,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
         while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
                && hpte_is_dirty(spapr->htab, index)
                && hpte_is_valid(spapr->htab, index)) {
-            CLEAN_HPTE(hpte_get(spapr->htab, index));
+            hpte_set_clean(spapr->htab, index);
             index++;
             examined++;
         }
@@ -2281,7 +2286,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
         while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
                && hpte_is_dirty(spapr->htab, index)
                && !hpte_is_valid(spapr->htab, index)) {
-            CLEAN_HPTE(hpte_get(spapr->htab, index));
+            hpte_set_clean(spapr->htab, index);
             index++;
             examined++;
         }
-- 
2.45.2

Re: [PATCH v3 5/7] hw/ppc/spapr: Convert CLEAN_HPTE() macro as hpte_set_clean() method

[Nicholas Piggin]

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

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

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ppc/spapr.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5bc49598a97..4e1fe832c29 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1416,7 +1416,12 @@ static bool hpte_is_dirty(SpaprMachineState *s, unsigned index)
>      return ldq_be_p(hpte_get(s, index)) & HPTE64_V_HPTE_DIRTY;
>  }
>  
> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> +static void hpte_set_clean(SpaprMachineState *s, unsigned index)
> +{
> +    stq_be_p(hpte_get(s, index),
> +             ldq_be_p(hpte_get(s, index)) & ~HPTE64_V_HPTE_DIRTY);
> +}
> +
>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>  
>  /*
> @@ -2213,7 +2218,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>          /* Consume invalid HPTEs */
>          while ((index < htabslots)
>                 && !hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>              index++;
>          }
>  
> @@ -2221,7 +2226,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>          chunkstart = index;
>          while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
>                 && hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>              index++;
>          }
>  
> @@ -2271,7 +2276,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>          while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
>                 && hpte_is_dirty(spapr->htab, index)
>                 && hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>              index++;
>              examined++;
>          }
> @@ -2281,7 +2286,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>          while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
>                 && hpte_is_dirty(spapr->htab, index)
>                 && !hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>              index++;
>              examined++;
>          }

Re: [PATCH v3 5/7] hw/ppc/spapr: Convert CLEAN_HPTE() macro as hpte_set_clean() method

[Harsh Prateek Bora]

On 12/18/24 23:51, Philippe Mathieu-Daudé wrote:
> Convert CLEAN_HPTE() macro as hpte_set_clean() method.
> Since sPAPR is in big endian configuration at reset,
> use the big endian LD/ST API to access the HPTEs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/spapr.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5bc49598a97..4e1fe832c29 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1416,7 +1416,12 @@ static bool hpte_is_dirty(SpaprMachineState *s, unsigned index)
>       return ldq_be_p(hpte_get(s, index)) & HPTE64_V_HPTE_DIRTY;
>   }
>   
> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> +static void hpte_set_clean(SpaprMachineState *s, unsigned index)
> +{
> +    stq_be_p(hpte_get(s, index),
> +             ldq_be_p(hpte_get(s, index)) & ~HPTE64_V_HPTE_DIRTY);
> +}
> +
>   #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>   
>   /*
> @@ -2213,7 +2218,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>           /* Consume invalid HPTEs */
>           while ((index < htabslots)
>                  && !hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);

Ditto. I see patch 6 also have similar issue. Please take care.

Thanks
Harsh

>               index++;
>           }
>   
> @@ -2221,7 +2226,7 @@ static void htab_save_first_pass(QEMUFile *f, SpaprMachineState *spapr,
>           chunkstart = index;
>           while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
>                  && hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>               index++;
>           }
>   
> @@ -2271,7 +2276,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>           while ((index < htabslots) && (index - chunkstart < USHRT_MAX)
>                  && hpte_is_dirty(spapr->htab, index)
>                  && hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>               index++;
>               examined++;
>           }
> @@ -2281,7 +2286,7 @@ static int htab_save_later_pass(QEMUFile *f, SpaprMachineState *spapr,
>           while ((index < htabslots) && (index - invalidstart < USHRT_MAX)
>                  && hpte_is_dirty(spapr->htab, index)
>                  && !hpte_is_valid(spapr->htab, index)) {
> -            CLEAN_HPTE(hpte_get(spapr->htab, index));
> +            hpte_set_clean(spapr->htab, index);
>               index++;
>               examined++;
>           }

[PATCH v3 6/7] hw/ppc/spapr: Convert DIRTY_HPTE() macro as hpte_set_dirty() method

[Philippe Mathieu-Daudé]

Convert DIRTY_HPTE() macro as hpte_set_dirty() method.
Since sPAPR is in big endian configuration at reset,
use the big endian LD/ST API to access the HPTEs.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4e1fe832c29..dedf6fb2916 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1422,7 +1422,11 @@ static void hpte_set_clean(SpaprMachineState *s, unsigned index)
              ldq_be_p(hpte_get(s, index)) & ~HPTE64_V_HPTE_DIRTY);
 }
 
-#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
+static void hpte_set_dirty(SpaprMachineState *s, unsigned index)
+{
+    stq_be_p(hpte_get(s, index),
+             ldq_be_p(hpte_get(s, index)) | HPTE64_V_HPTE_DIRTY);
+}
 
 /*
  * Get the fd to access the kernel htab, re-opening it if necessary
@@ -1633,7 +1637,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_get(spapr->htab, i));
+            hpte_set_dirty(spapr->htab, i);
         }
     }
     /* We're setting up a hash table, so that means we're not radix */
-- 
2.45.2

Re: [PATCH v3 6/7] hw/ppc/spapr: Convert DIRTY_HPTE() macro as hpte_set_dirty() method

[Nicholas Piggin]

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

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

>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ppc/spapr.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4e1fe832c29..dedf6fb2916 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1422,7 +1422,11 @@ static void hpte_set_clean(SpaprMachineState *s, unsigned index)
>               ldq_be_p(hpte_get(s, index)) & ~HPTE64_V_HPTE_DIRTY);
>  }
>  
> -#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> +static void hpte_set_dirty(SpaprMachineState *s, unsigned index)
> +{
> +    stq_be_p(hpte_get(s, index),
> +             ldq_be_p(hpte_get(s, index)) | HPTE64_V_HPTE_DIRTY);
> +}
>  
>  /*
>   * Get the fd to access the kernel htab, re-opening it if necessary
> @@ -1633,7 +1637,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_get(spapr->htab, i));
> +            hpte_set_dirty(spapr->htab, i);
>          }
>      }
>      /* We're setting up a hash table, so that means we're not radix */

[PATCH v3 7/7] hw/ppc/epapr: Do not swap ePAPR magic value

[Philippe Mathieu-Daudé]

The ePAPR magic value in $r6 doesn't need to be byte swapped.

See ePAPR-v1.1.pdf chapter 5.4.1 "Boot CPU Initial Register State"
and the following mailing-list thread:
https://lore.kernel.org/qemu-devel/CAFEAcA_NR4XW5DNL4nq7vnH4XRH5UWbhQCxuLyKqYk6_FCBrAA@mail.gmail.com/

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/sam460ex.c     | 2 +-
 hw/ppc/virtex_ml507.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 78e2a46e753..db9c8f3fa6e 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -234,7 +234,7 @@ static void main_cpu_reset(void *opaque)
 
         /* Create a mapping for the kernel.  */
         booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1 << 31);
-        env->gpr[6] = tswap32(EPAPR_MAGIC);
+        env->gpr[6] = EPAPR_MAGIC;
         env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
 
     } else {
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index f378e5c4a90..6197d31d88f 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -119,7 +119,7 @@ static void main_cpu_reset(void *opaque)
     /* Create a mapping spanning the 32bit addr space. */
     booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1U << 31);
     booke_set_tlb(&env->tlb.tlbe[1], 0x80000000, 0x80000000, 1U << 31);
-    env->gpr[6] = tswap32(EPAPR_MAGIC);
+    env->gpr[6] = EPAPR_MAGIC;
     env->gpr[7] = bi->ima_size;
 }
 
-- 
2.45.2

Re: [PATCH v3 7/7] hw/ppc/epapr: Do not swap ePAPR magic value

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

On Wed, 18 Dec 2024, Philippe Mathieu-Daudé wrote:
> The ePAPR magic value in $r6 doesn't need to be byte swapped.
>
> See ePAPR-v1.1.pdf chapter 5.4.1 "Boot CPU Initial Register State"
> and the following mailing-list thread:
> https://lore.kernel.org/qemu-devel/CAFEAcA_NR4XW5DNL4nq7vnH4XRH5UWbhQCxuLyKqYk6_FCBrAA@mail.gmail.com/
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/ppc/sam460ex.c     | 2 +-
> hw/ppc/virtex_ml507.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 78e2a46e753..db9c8f3fa6e 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -234,7 +234,7 @@ static void main_cpu_reset(void *opaque)
>
>         /* Create a mapping for the kernel.  */
>         booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1 << 31);
> -        env->gpr[6] = tswap32(EPAPR_MAGIC);
> +        env->gpr[6] = EPAPR_MAGIC;

I don't know how to test this (or if anything actually uses it). What I'm 
not sure about is what endianness env->gpr is? It's a host array so maybe 
it needs to match the host endianness which is little endian most of the 
time as opposed to PPC big endian on this machine. So maybe tswap is wrong 
but is removing it right? Maybe we need to only swap on LE hosts. I think 
it's only used by Linux kernels so maybe trying to boot one could test 
this change but I'm not sure.

Regards,
BALATON Zoltan

>         env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
>
>     } else {
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index f378e5c4a90..6197d31d88f 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -119,7 +119,7 @@ static void main_cpu_reset(void *opaque)
>     /* Create a mapping spanning the 32bit addr space. */
>     booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1U << 31);
>     booke_set_tlb(&env->tlb.tlbe[1], 0x80000000, 0x80000000, 1U << 31);
> -    env->gpr[6] = tswap32(EPAPR_MAGIC);
> +    env->gpr[6] = EPAPR_MAGIC;
>     env->gpr[7] = bi->ima_size;
> }
>
>

Re: [PATCH v3 7/7] hw/ppc/epapr: Do not swap ePAPR magic value

[Nicholas Piggin]

On Thu Dec 19, 2024 at 5:18 AM AEST, BALATON Zoltan wrote:
> On Wed, 18 Dec 2024, Philippe Mathieu-Daudé wrote:
> > The ePAPR magic value in $r6 doesn't need to be byte swapped.
> >
> > See ePAPR-v1.1.pdf chapter 5.4.1 "Boot CPU Initial Register State"
> > and the following mailing-list thread:
> > https://lore.kernel.org/qemu-devel/CAFEAcA_NR4XW5DNL4nq7vnH4XRH5UWbhQCxuLyKqYk6_FCBrAA@mail.gmail.com/
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > hw/ppc/sam460ex.c     | 2 +-
> > hw/ppc/virtex_ml507.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> > index 78e2a46e753..db9c8f3fa6e 100644
> > --- a/hw/ppc/sam460ex.c
> > +++ b/hw/ppc/sam460ex.c
> > @@ -234,7 +234,7 @@ static void main_cpu_reset(void *opaque)
> >
> >         /* Create a mapping for the kernel.  */
> >         booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1 << 31);
> > -        env->gpr[6] = tswap32(EPAPR_MAGIC);
> > +        env->gpr[6] = EPAPR_MAGIC;
>
> I don't know how to test this (or if anything actually uses it).

The Linux kernel boot wrapper tests it AFAIKS. Does this mean
they never worked on LE hosts?

> not sure about is what endianness env->gpr is? It's a host array so maybe 
> it needs to match the host endianness which is little endian most of the 
> time as opposed to PPC big endian on this machine. So maybe tswap is wrong 
> but is removing it right? Maybe we need to only swap on LE hosts. I think 
> it's only used by Linux kernels so maybe trying to boot one could test 
> this change but I'm not sure.

Yes env->gpr is host-endian, and emulated target code will see
the same value that the host does. It can't be correct because
the machine running in emulation can't possibly know what endian
the host is.

I think we should just take it, it looks trivially correct
(always dangerous words when dealing with early boot code, lol).

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

>
> Regards,
> BALATON Zoltan
>
> >         env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
> >
> >     } else {
> > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > index f378e5c4a90..6197d31d88f 100644
> > --- a/hw/ppc/virtex_ml507.c
> > +++ b/hw/ppc/virtex_ml507.c
> > @@ -119,7 +119,7 @@ static void main_cpu_reset(void *opaque)
> >     /* Create a mapping spanning the 32bit addr space. */
> >     booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1U << 31);
> >     booke_set_tlb(&env->tlb.tlbe[1], 0x80000000, 0x80000000, 1U << 31);
> > -    env->gpr[6] = tswap32(EPAPR_MAGIC);
> > +    env->gpr[6] = EPAPR_MAGIC;
> >     env->gpr[7] = bi->ima_size;
> > }
> >
> >

Re: [PATCH v3 7/7] hw/ppc/epapr: Do not swap ePAPR magic value

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

On Thu, 19 Dec 2024, Nicholas Piggin wrote:
> On Thu Dec 19, 2024 at 5:18 AM AEST, BALATON Zoltan wrote:
>> On Wed, 18 Dec 2024, Philippe Mathieu-Daudé wrote:
>>> The ePAPR magic value in $r6 doesn't need to be byte swapped.
>>>
>>> See ePAPR-v1.1.pdf chapter 5.4.1 "Boot CPU Initial Register State"
>>> and the following mailing-list thread:
>>> https://lore.kernel.org/qemu-devel/CAFEAcA_NR4XW5DNL4nq7vnH4XRH5UWbhQCxuLyKqYk6_FCBrAA@mail.gmail.com/
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/ppc/sam460ex.c     | 2 +-
>>> hw/ppc/virtex_ml507.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>>> index 78e2a46e753..db9c8f3fa6e 100644
>>> --- a/hw/ppc/sam460ex.c
>>> +++ b/hw/ppc/sam460ex.c
>>> @@ -234,7 +234,7 @@ static void main_cpu_reset(void *opaque)
>>>
>>>         /* Create a mapping for the kernel.  */
>>>         booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1 << 31);
>>> -        env->gpr[6] = tswap32(EPAPR_MAGIC);
>>> +        env->gpr[6] = EPAPR_MAGIC;
>>
>> I don't know how to test this (or if anything actually uses it).
>
> The Linux kernel boot wrapper tests it AFAIKS. Does this mean
> they never worked on LE hosts?

Linux boots on the sam460ex on LE host, I don't know if it works on BE 
host as I don't have any so this could be broken on BE host but should 
work on LE host. If this breaks Linux boot somebody may report it, I don't 
have time to try testing it now.

Regards,
BALATON Zoltan

>> not sure about is what endianness env->gpr is? It's a host array so maybe
>> it needs to match the host endianness which is little endian most of the
>> time as opposed to PPC big endian on this machine. So maybe tswap is wrong
>> but is removing it right? Maybe we need to only swap on LE hosts. I think
>> it's only used by Linux kernels so maybe trying to boot one could test
>> this change but I'm not sure.
>
> Yes env->gpr is host-endian, and emulated target code will see
> the same value that the host does. It can't be correct because
> the machine running in emulation can't possibly know what endian
> the host is.
>
> I think we should just take it, it looks trivially correct
> (always dangerous words when dealing with early boot code, lol).
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>>
>> Regards,
>> BALATON Zoltan
>>
>>>         env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
>>>
>>>     } else {
>>> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
>>> index f378e5c4a90..6197d31d88f 100644
>>> --- a/hw/ppc/virtex_ml507.c
>>> +++ b/hw/ppc/virtex_ml507.c
>>> @@ -119,7 +119,7 @@ static void main_cpu_reset(void *opaque)
>>>     /* Create a mapping spanning the 32bit addr space. */
>>>     booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1U << 31);
>>>     booke_set_tlb(&env->tlb.tlbe[1], 0x80000000, 0x80000000, 1U << 31);
>>> -    env->gpr[6] = tswap32(EPAPR_MAGIC);
>>> +    env->gpr[6] = EPAPR_MAGIC;
>>>     env->gpr[7] = bi->ima_size;
>>> }
>>>
>>>
>
>