[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements
From: |
Jin Guojie |
Subject: |
Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements |
Date: |
Mon, 5 Dec 2016 17:41:19 +0800 |
------------------ Original ------------------
From: "Richard Henderson";<address@hidden>;
Send time: Thursday, Dec 1, 2016 11:52 PM
To: "Jin Guojie"<address@hidden>; "qemu-devel"<address@hidden>;
Cc: "Aurelien Jarno"<address@hidden>; "James Hogan"<address@hidden>;
Subject: Re: [PATCH v5 00/10] tcg mips64 and mips r6 improvements
Thanks a lot for Richard's careful review.
I have some different opinions for discussion:
> @@ -1233,7 +1235,8 @@ static void tcg_out_tlb_load(TCGContext *s, TCGReg
> base,
> - mask = (target_ulong)TARGET_PAGE_MASK | ((1 << a_bits) - 1);
> + mask = TARGET_PAGE_MASK | ((1 << a_bits) - 1);
> You need the target_ulong cast here for mips64.
> See patch ebb90a005da67147245cd38fb04a965a87a961b7.
Since mask is defined as a C type "target_ulong" at the beginning of the
function,
I guess an implicit type cast should be already done by the compiler.
So your change is functionally the same with v5 patch.
To test my idea, I wrote a small case and compiled it on an x86_64 host:
main()
{
int a_bits = 2;
int page_mask = 0xfffff000; /* X86 4KB page*/
unsigned int mask = page_mask | ((1 << a_bits) - 1);
unsigned long m = mask;
printf("mask=%lx\n", m);
}
$ gcc a.c
$ file a.out
a.out: Mach-O 64-bit executable x86_64
$ ./a.out
mask=fffff003
The output is exactly an unsigned 32-bit quantity.
I didn't see a wrong result generated.
Could you teach me where is my mistake?
> if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
> - tcg_out_ext32u(s, base, addrl);
> - addrl = base;
> + tcg_out_ext32u(s, TCG_TMP0, TCG_TMP0);
> }
> We should be zero-extending the guest address, not the address that we just
> loaded from CMP_OFF. This is because we need to add an unsigned 32-bit
> quantity to the full 64-bit host pointer that we load from ADD_OFF.
> Did you notice a compare problem between TMP1 and TMP0 below? If so, I
> believe
> that a partial solution is the TARGET_PAGE_MASK change above. I guess we also
> need to do
> tcg_out_ldst(s, (TARGET_LONG_BITS == 64 ? OPC_LD
> TCG_TARGET_REG_BITS == 64 : OPC_LWU : OPC_LW),
> TCG_TMP0, TCG_REG_A0, cmp_off);
> in the else section of the tlb comparator load above, since mask will be
> 32-bit
> unsigned for TARGET_LONG_BITS == 32, and we need an unsigned quantity to
> compare that against.
In v5 patch, TMP0 is first loaded via LD, and then it is zero-extended. This is
just
the same behavior as LWU.
After OPC_AND, TMP1 also contains unsigned 32-bit quantity.
So in theory, there should not exist compare problem between TMP1 and TMP0.
I agree with your opinion that addrl should be zero-extended, but It's really
strange
that the last ALIAS_PADD doesn't generate a bad host address in v5 patch.
If v5 patch really lacks the zero-extension operation of addrl, the subsequent
load
or store will access wrong memory space.
However, all the tests so far did not crash at this point.
To further verify my idea, I will do more debug work and test the tlb miss rate
to see if your analysis should be implemented.
At the meantime, I am waiting for Aurelien's test results. I think it's better
to collect
all people's feedback before I submit the v6 patch.
Jin Guojie
- [Qemu-devel] [PATCH v5 02/10] tcg-mips: Add mips64 opcodes, (continued)
- [Qemu-devel] [PATCH v5 02/10] tcg-mips: Add mips64 opcodes, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 01/10] tcg-mips: Move bswap code to a subroutine, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 05/10] tcg-mips: Adjust move functions for mips64, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 06/10] tcg-mips: Adjust load/store functions for mips64, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 07/10] tcg-mips: Adjust prologue for mips64, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 03/10] tcg-mips: Support 64-bit opcodes, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 08/10] tcg-mips: Add tcg unwind info, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 09/10] tcg-mips: Adjust calling conventions for mips64, Jin Guojie, 2016/12/01
- [Qemu-devel] [PATCH v5 10/10] tcg-mips: Adjust qemu_ld/st for mips64, Jin Guojie, 2016/12/01
- Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements, Richard Henderson, 2016/12/01
- Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements,
Jin Guojie <=
- Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements, James Hogan, 2016/12/02
- Re: [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements, Aurelien Jarno, 2016/12/02
- [Qemu-devel] [PATCH v5 00/10] tcg mips64 and mips r6 improvements, YunQiang Su, 2016/12/02