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

[Lucas Mateus Martins Araujo e Castro]

On 09/11/2021 13:44, Daniel Henrique Barboza wrote:

>       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.
>       
>       
>     
    Ok I'll change that, originally I put it in the first patch as that
    patch resolved the part of the bug that could cause problem.
>       
> >         
> >         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
>       
>       
>     
    Ok I'll put it in parenthesis and send a next version with this and
    the gen_helper_reset_fpstatus removed.
>       
>       
>       
>       Thanks,
>       
>       
>       
>       
>       Daniel
>       
>       
>       
> > +
> >         
> >          
> > /*****************************************************************************/
> >           /* Vector status and control register */
> >         
> >           #define VSCR_NJ         16 /* Vector non-java */
> >         
> >         
> >       
>

--
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Estagiario
Aviso Legal - Disclaimer