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

[Lucas Mateus Martins Araujo e Castro <lucas.araujo@eldorado.org.br>]

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


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 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Estagiario
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 5714 bytes --]