Re: [PATCH 2/2] target/ppc: ppc_store_fpscr doesn't update bit 52

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 10/20/21 09:57, Lucas Mateus Castro (alqotel) wrote:
> From: "Lucas Mateus Castro (alqotel)" <lucas.castro@eldorado.org.br>
> 
> This commit fixes the difference reported in the bug in the reserved
> bit 52, it does this by adding this bit to the mask of bits to not be
> directly altered in the ppc_store_fpscr function (the hardware used to
> compare to QEMU was a Power9).

IIUC, "bug" here is related to https://gitlab.com/qemu-project/qemu/-/issues/266,
the bug mentioned in the commit msg of the first patch. In that case, you
should mention it again in this commit message explicitly.

In fact, I also believe that the "Resolves:" tag from the first patch should
be moved to this patch instead, given that the bug is only fully fixed after
both patches are applied.


> 
> Although this is a difference reported in the bug, since it's a reserved
> bit it may be a "don't care" case, as put in the bug report. Looking at
> the ISA it doesn't explicitly mentions this bit can't be set, like it
> does for FEX and VX, so I'm unsure if this is necessary.
> 
> Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.castro@eldorado.org.br>
> ---
>   target/ppc/cpu.c | 2 +-
>   target/ppc/cpu.h | 3 +++
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index 7ad9bd6044..5c411b32ff 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -112,7 +112,7 @@ static inline void fpscr_set_rounding_mode(CPUPPCState *env)
>   
>   void ppc_store_fpscr(CPUPPCState *env, target_ulong val)
>   {
> -    val &= ~(FP_VX | FP_FEX);
> +    val &= FPSCR_MTFS_MASK;
>       if (val & FPSCR_IX) {
>           val |= FP_VX;
>       }
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index baa4e7c34d..4b42b281ed 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -736,6 +736,9 @@ enum {
>                             FP_VXZDZ  | FP_VXIMZ  | FP_VXVC   | FP_VXSOFT | \
>                             FP_VXSQRT | FP_VXCVI)
>   
> +/* FPSCR bits that can be set by mtfsf, mtfsfi and mtfsb1 */
> +#define FPSCR_MTFS_MASK ~((1ull << 11) | FP_VX | FP_FEX)


./scripts/checkpatch.pl is not happy about this line:


ERROR: Macros with complex values should be enclosed in parenthesis
#44: FILE: target/ppc/cpu.h:763:
+#define FPSCR_MTFS_MASK ~((1ull << 11) | FP_VX | FP_FEX)

total: 1 errors, 0 warnings, 17 lines checked




Thanks,



Daniel


> +
>   /*****************************************************************************/
>   /* Vector status and control register */
>   #define VSCR_NJ         16 /* Vector non-java */
>