Re: [Qemu-devel] [PATCH 3/8] migration: support todetectcompressionand decompression errors

[jiang.biao2]

> On 03/28/2018 12:20 PM, Peter Xu wrote:
>> On Wed, Mar 28, 2018 at 12:08:19PM +0800, jiang.bi...@zte.com.cn wrote:

 On Tue, Mar 27, 2018 at 10:35:29PM +0800, Xiao Guangrong wrote:

>>> No, we can't make the assumption that "error _must_ be caused by page 
>>> update".
>>> No document/ABI about compress/decompress promised it. :)

 Indeed, I found no good documents about below errors that jiang.biao
 pointed out.
>>> Hi, Peter
>>> The description about the errors comes from here,
>>> http://www.zlib.net/manual.html
>>> And about the error codes returned by inflate(), they are described as，
>>> ** inflate() returns
>>> Z_OK if some progress has been made (more input processed or more output 
>>> produced),
>>> Z_STREAM_END if the end of the compressed data has been reached and all 
>>> uncompressed output has been produced,
>>> Z_NEED_DICT if a preset dictionary is needed at this point,
>>> Z_DATA_ERROR if the input data was corrupted (input stream not conforming 
>>> to the zlib format or incorrect check value, in which case strm->msg points 
>>> to a string with a >more specific error),
>>> Z_STREAM_ERROR if the stream structure was inconsistent (for example 
>>> next_in or next_out was Z_NULL, or the state was inadvertently written over 
>>> by the application),
>>> Z_MEM_ERROR if there was not enough memory,
>>> Z_BUF_ERROR if no progress was possible or if there was not enough room in 
>>> the output buffer when Z_FINISH is used. ...
>>> **
>>
>> Ah yes.  My bad to be so uncareful. :)
>>
>>> According to the above description, the error caused by page update looks
>>> more like tend to return Z_DATA_ERROR, but I do not have env to verify 
>>> that. :)
>
> No, still lack info to confirm the case of compressing the data being
> updated is the only one to return Z_DATA_ERROR. And nothing provided
> that no other error condition causes data corrupted will be squeezed
> into this error code.
>
>>> As I understand it, the real compress/decompress error cases other than that
>>> caused by page update should be rare, maybe the error code is enough to
>>> distinguish those if we can verify the the error codes returned by page 
>>> update
>>> and other silent failures by test. If so, we can cut the cost of memcpy.
>
> Please note, compare with other operations, e.g, compression, detect zero 
> page,
> etc., memcpy() is not a hot function at all.

Just out of curiousity, what's level of memory num need to be copied for 
normal cases? KBs, MBs? 

>>> If not, I agree with Guangrong's idea too. I never read the zlib code and 
>>> all my
>>> information comes from the manual, so if anything inaccurate, pls ignore my
>>> option. :)
>>
>> So I suppose all of us know that alternative now, we just need a solid
>> way to confirm the uncertainty.  I'll leave this to Guangrong.
>
> Yes, i still prefer to memcpy() to make it safe enough to protect our 
> production
> unless we get enough certainty to figure out the error conditions.

Indeed, no garantee for that currently, so for safe, we need memcpy(). 
Never mind, pls just ignore my opinion. :)  
Thanks!

Regards,
Jiang

Re: [Qemu-devel] [PATCH 3/8] migration: support todetectcompression and decompression errors

[jiang.biao2]

> 
> On Tue, Mar 27, 2018 at 10:35:29PM +0800, Xiao Guangrong wrote:
>
>> > > No, we can't make the assumption that "error _must_ be caused by page 
>> > > update".
>> > > No document/ABI about compress/decompress promised it. :)
>
> Indeed, I found no good documents about below errors that jiang.biao
> pointed out.
Hi, Peter
The description about the errors comes from here,
http://www.zlib.net/manual.html
And about the error codes returned by inflate(), they are described as，
** inflate() returns 
Z_OK if some progress has been made (more input processed or more output 
produced),
Z_STREAM_END if the end of the compressed data has been reached and all 
uncompressed output has been produced, 
Z_NEED_DICT if a preset dictionary is needed at this point, 
Z_DATA_ERROR if the input data was corrupted (input stream not conforming to 
the zlib format or incorrect check value, in which case strm->msg points to a 
string with a more specific error), 
Z_STREAM_ERROR if the stream structure was inconsistent (for example next_in or 
next_out was Z_NULL, or the state was inadvertently written over by the 
application), 
Z_MEM_ERROR if there was not enough memory, 
Z_BUF_ERROR if no progress was possible or if there was not enough room in the 
output buffer when Z_FINISH is used. ... 
**
According to the above description, the error caused by page update looks 
more like tend to return Z_DATA_ERROR, but I do not have env to verify that. :)
As I understand it, the real compress/decompress error cases other than that 
caused by page update should be rare, maybe the error code is enough to
distinguish those if we can verify the the error codes returned by page update
and other silent failures by test. If so, we can cut the cost of memcpy.  
If not, I agree with Guangrong's idea too. I never read the zlib code and all my
information comes from the manual, so if anything inaccurate, pls ignore my
option. :)

Regards,
Jiang

Re: [Qemu-devel] [PATCH 3/8] migration: support to detectcompression and decompression errors

[jiang.biao2]

> On 03/27/2018 07:17 PM, Peter Xu wrote:
>> On Tue, Mar 27, 2018 at 03:42:32AM +0800, Xiao Guangrong wrote:
>> 
>> [...]
>> 
 It'll be understandable to me if the problem is that the compress()
 API does not allow the input buffer to be changed during the whole
 period of the call.  If that is a must, this patch for sure helps.
>>>
>>> Yes, that is exactly what i want to say. :)
>> 
>> So I think now I know what this patch is for. :) And yeah, it makes
>> sense.
>> 
>> Though another question would be: if the buffer is updated during
>> compress() and compress() returned error, would that pollute the whole
>> z_stream or it only fails the compress() call?
>> 
>
> I guess deflateReset() can recover everything, i.e, keep z_stream as
> it is init'ed by deflate_init().
>
>> (Same question applies to decompress().)
>> 
>> If it's only a compress() error and it won't pollute z_stream (or say,
>> it can be recovered after a deflateReset() and then we can continue to
>> call deflate() without problem), then we'll actually have two
>> alternatives to solve this "buffer update" issue:
>> 
>> 1. Use the approach of current patch: we copy the page every time, so
>> deflate() never fails because update never happens.  But it's slow
>> since we copy the pages every time.
>> 
>> 2. Use the old approach, and when compress() fail, we just ignore that
>> page (since now we know that error _must_ be caused by page update,
>> then we are 100% sure that we'll send that page again so it'll be
>> perfectly fine).
>> 
>
> No, we can't make the assumption that "error _must_ be caused by page 
> update". 
> No document/ABI about compress/decompress promised it. :)
So, as I metioned before, can we just distingush the decompress/compress errors 
from errors caused by page update by the return code of inflate/deflate?
According to the zlib manual, there seems to be several error codes for 
different 
cases,
#define Z_ERRNO(-1) 
#define Z_STREAM_ERROR (-2) 
#define Z_DATA_ERROR   (-3) 
#define Z_MEM_ERROR(-4)
#define Z_BUF_ERROR(-5)
#define Z_VERSION_ERROR (-6)
Did you check the return code when silent failure(not caused by page update) 
happened before? :)

Jiang
Regards

Re: [Qemu-devel] [PATCH 3/8] migration: support to detect compressionand decompression errors

[jiang.biao2]

Hi, guangrong
> @@ -1051,11 +1052,13 @@ static int do_compress_ram_page(QEMUFile *f, z_stream 
> *stream, RAMBlock *block,
> {
> RAMState *rs = ram_state;
> int bytes_sent, blen;
> -uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
> +uint8_t buf[TARGET_PAGE_SIZE], *p;

> +p = block->host + (offset & TARGET_PAGE_MASK);
> bytes_sent = save_page_header(rs, f, block, offset |
> RAM_SAVE_FLAG_COMPRESS_PAGE);
> -blen = qemu_put_compression_data(f, stream, p, TARGET_PAGE_SIZE);
> +memcpy(buf, p, TARGET_PAGE_SIZE);
> +blen = qemu_put_compression_data(f, stream, buf, TARGET_PAGE_SIZE);
Memory copy operation for every page to be compressed is not cheap, especially
when the page number is huge, and it may be not necessary for pages never 
updated during migration. 
Is there any possibility that we can distinguish the real compress/decompress 
errors from those being caused by source VM updating? Such as the return 
value of qemu_uncompress(distinguish Z_DATA_ERROR and other error codes 
returned by inflate())? 

Jiang
Regards,

Re: [Qemu-devel] [PATCH 2/8] migration: stop allocating andfreeingmemory frequently

[jiang.biao2]

>>> +err = deflate(stream, Z_FINISH);
>>> +if (err != Z_STREAM_END) {
>>> +return -1;
>>> +}
>>> +
>>> +return stream->next_out - dest;
>>> +}
>>> +
>>>
>>> @@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const 
>>> uint8_t *p, size_t size,
>>> return -1;
>>> }
>>> }
>>> -if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *),
>>> -  (Bytef *)p, size, level) != Z_OK) {
>>> +
>>> +blen = qemu_compress_data(stream, f->buf + f->buf_index + 
>>> sizeof(int32_t),
>>> +  blen, p, size);
>> The "level" parameter is never used after the patch, could we just removed 
>> it?
>> On the other hand, deflate() of zlib supports compression level too(by
>> deflateInit(stream, level)), should we just reuse the level properly?  If 
>> not, the
>> *migrate parameter compress_level* will be useless.
> 
> The 'level' has been pushed to @stream:
> +if (deflateInit(_param[i].stream,
> +   migrate_compress_level()) != Z_OK) {
> +goto exit;
> +}
Indeed, I missed that. 
Reviewed-by: Jiang Biao 

Re: [Qemu-devel] [PATCH 2/8] migration: stop allocating and freeingmemory frequently

[jiang.biao2]

Hi, guangrong
> 
> +/* return the size after compression, or negative value on error */
> +static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t 
> dest_len,
> +  const uint8_t *source, size_t source_len)
> +{
> +int err;
> +
> +err = deflateReset(stream);
> +if (err != Z_OK) {
> +return -1;
> +}
> +
> +stream->avail_in = source_len;
> +stream->next_in = (uint8_t *)source;
> +stream->avail_out = dest_len;
> +stream->next_out = dest;
>+
duplicated code with qemu_uncompress(), would initializing stream outside 
of qemu_compress_data() be better? In that case, we could pass much less
parameters down, and avoid the duplicated code. Or could we encapsulate 
some struct to ease the case?
> +err = deflate(stream, Z_FINISH);
> +if (err != Z_STREAM_END) {
> +return -1;
> +}
> +
> +return stream->next_out - dest;
> +}
> +
> 
> @@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const 
> uint8_t *p, size_t size,
> return -1;
> }
> }
> -if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *),
> -  (Bytef *)p, size, level) != Z_OK) {
> +
> +blen = qemu_compress_data(stream, f->buf + f->buf_index + 
> sizeof(int32_t),
> +  blen, p, size);
The "level" parameter is never used after the patch, could we just removed it? 
On the other hand, deflate() of zlib supports compression level too(by 
deflateInit(stream, level)), should we just reuse the level properly?  If not, 
the 
*migrate parameter compress_level* will be useless. 
> +if (blen < 0) {
> error_report("Compress Failed!");
> return 0;
> }
>
> +/* return the size after decompression, or negative value on error */
> +static int qemu_uncompress(z_stream *stream, uint8_t *dest, size_t dest_len,
> +   uint8_t *source, size_t source_len)
The name of *qemu_uncompress* does not quite match *qemu_compress_data*,
would *qemu_uncompress_data* be better?
Besides, the prototype is not consistent with  *qemu_compress_data* either, 
should the -*source- be -const- also here?
> +{
> +int err;
> +
> +err = inflateReset(stream);
> +if (err != Z_OK) {
> +return -1;
> +}
> +
> +stream->avail_in = source_len;
> +stream->next_in = source;
> +stream->avail_out = dest_len;
> +stream->next_out = dest;
> +
> +err = inflate(stream, Z_NO_FLUSH);
> +if (err != Z_STREAM_END) {
> +return -1;
> +}
> +
> +return stream->total_out;
> +}
> +

Jiang
Regards,

Re: [Qemu-devel] [PATCH] tcg/mips: make some functions inline.

[jiang.biao2]

> On 11/02/2017 12:23 PM, jiang.bi...@zte.com.cn wrote:
> > No, :) Just found some of these functions are inline and others are not 
> > when reading
> > the code, the style looks inconsistent.
> >
> My preference would be to remove them all.
Should I commit another patch to do that? :) 

> > Besides, The compiler may make default inline choices for some functions, 
> > but may
> > not for some cases(some longer ones). Adding inline manually can guarantee 
> > and
> > not hurt. :)
> The longer ones in particular should *not* be inline, so that we don't expand
> the size of the code unnecessarily.  Which *can* hurt.
Got that, thanks for the reply.

Regards,

Re: [Qemu-devel] [PATCH] tcg/mips: make some functions inline.

[jiang.biao2]

> On 11/02/2017 11:26 AM, Jiang Biao wrote:> > Make some functions inline.
> 
> Why?
> 
> Do you have evidence that the compiler isn't making reasonable default choices
> with respect to inlining?
No, :) Just found some of these functions are inline and others are not when 
reading 
the code, the style looks inconsistent. 
Besides, The compiler may make default inline choices for some functions, but 
may 
not for some cases(some longer ones). Adding inline manually can guarantee and 
not hurt. :)
Thanks.

r~

Re: [Qemu-devel] [PATCH] checkpatch: replace ERROR with WARN forextern checking.

[jiang.biao2]

> On 11 October 2017 at 05:33, Jiang Biao  wrote:
> 
> I'd rather not drop this to a warning for the sake of a single
> use case that's already in the tree (and which if you really
> cared about you could work around by putting the link_error()
> declaration in a header file I suppose, though I wouldn't
> bother personally.)



Neither would I :). Thanks for the reply. 


Regards, Jiang

Re: [Qemu-devel] [PATCH] scripts/checkpatch.pl: fix false error ofexterns checking.

[jiang.biao2]

> On 10 October 2017 at 08:54, Jiang Biao  wrote:


> > When adding a function declaration in a .c file without extern
> >keywork decoration, checkpatch.pl will report *externs should be
> > avoided in .c files* false error. This patch fixes the bug.
> 
> I don't think this is a bug. "extern int foo(void);" and
> "int foo(void);" have the same effect: they declare a function
> prototype that can be called from outside the file.
> We don't want to permit that in QEMU. Either:
>  (1) the function is for use only within this file: the
>  declaration should be "static int foo(void);"
>  (2) the function is for use by other files too: the
>  declaration should be in a header file, not a .c file,
>  so the other files can use it
> 
> Do you have an example of code where this warning is a problem?
Hi Peter,

I understand external functions should be put in header files. But there

is some rare exceptions, take the code segment in 


tcg/mips/tcg-target.inc.c for example,

#if TCG_TARGET_REG_BITS == 32
# define LO_OFF  (MIPS_BE * 4)
# define HI_OFF  (4 - LO_OFF)
#else
/* To assert at compile-time that these values are never used
   for TCG_TARGET_REG_BITS == 64.  */
/* extern */ int link_error(void);
# define LO_OFF  link_error()
# define HI_OFF  link_error()
#endif

In this case, the external funciton is just used to assert at complie-time.


If I make a patch to delete the commented out extern before *int link_error()*,

the checkpatch.pl will complain the  *externs should be avoided in .c files* 


error.

The same usage can also be found in tcg/tcg-op.c,

/* Reduce the number of ifdefs below.  This assumes that all uses of
   TCGV_HIGH and TCGV_LOW are properly protected by a conditional that
   the compiler can eliminate.  */
#if TCG_TARGET_REG_BITS == 64
extern TCGv_i32 TCGV_LOW_link_error(TCGv_i64);
extern TCGv_i32 TCGV_HIGH_link_error(TCGv_i64);
#define TCGV_LOW  TCGV_LOW_link_error
#define TCGV_HIGH TCGV_HIGH_link_error
#endif

I'm not sure if the checkpatch.pl should take these case into consideration. :)

Thanks.





Regards.

Re: [Qemu-devel] [PATCH v2 00/45] tcg: support for multipleTCGcontexts

[jiang.biao2]

> For this specific case, the primary problem will be the implicit memory 
> barriers that the i386 guest requires, but that the mips64 host does not 
> provide.
> 
> For tcg/mips/, TCG_TARGET_DEFAULT_MO should be 0, because the mips 
> architecture 
> does not have any implicit memory ordering.  All memory barriers are explicit 
> via SYNC instructions.
> 
> For target/i386, TCG_GUEST_DEFAULT_MO is TCG_MO_ALL & ~TCG_MO_ST_LD, or
> 
> TCG_MO_LD_LD | TCG_MO_LD_ST | TCG_MO_ST_ST
> 
> which means that implicit memory barriers exist between load/load, 
> load/store, 
> store/store, but not store/load.
> 
> In order to fix this, we need to add the missing barriers to the opcode 
> stream.
> 
> The simplest fix for this is to put a call
> 
> tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC)
> 
> at the end of gen_ldst_i32 and gen_ldst_i64.  That should be good enough to 
> make your specific case operate correctly.
> 
> A proper fix will involve (1) emitting those barriers only if the barrier is 
> not implied by the host and (2) optimizing away redundant barriers.
> 
Thanks very much. That really helps a lot.

I'll dig deeper into that and maybe feedback some patches.

Re: [Qemu-devel] [PATCH v2 00/45] tcg: support for multiple TCGcontexts

[jiang.biao2]

> On 07/18/2017 02:22 PM, jiang.bi...@zte.com.cn wrote:



> > Seeing your work on multiple TCG, it seems that it has some kind of 
> > connection 
> > with the  MTTCG feature,
> > 
> > but I do not figure out how they are connected in detail.
> > 
> > Could you pls help to confirm the following questions:
> > 
> >  1.
> > 
> > what is the relationship between your patches and the MTTCG feature
> > mentioned by https://lwn.net/Articles/697265/?
> 
> The current MTTCG feature is in QEMU mainline.  It allows parallel execution 
> of 
> translated code in both system mode.  It does *not* allow parallel 
> translation 
> -- all translation is done with tb_lock held.
> 
> Note that we *always* have parallel execution in user mode.  However, this 
> can 
> and does lead to problems.  See below.
> 
> This patch set allows parallel translation in system mode.  This is shown to 
> improve the overall throughput.  It does *not* allow parallel translation in 
> user mode.  Firstly because user mode already shares more translations 
> between 
> threads (because it is running a single executable), and so the translation 
> routines are not high in the profile.  Secondly because there are additional 
> locking problems due to the fact that we have no bound on the number of user 
> threads.

Does that mean both the MTTCG feature and this patch set are all about system 


mode, and have nothing to do with linux-user mode?




>
>  2.
> 
> What is the current status of the development of the MTTCG feature?
> 
> MTTCG has only been enabled on a few targets: alpha, arm, ppc64.
> Look for "mttcg=yes" in configure.
> 
> In order for MTTCG to be enabled, the target must be adjusted so that
> (1) all atomic instructions are implemented with atomic tcg operations,
> (2) define TCG_GUEST_DEFAULT_MO to indicate any barriers implied by
> normal memory operations by the target architecture.
> 
> For target/mips, neither of these things are complete.
> 
> MTTCG has only been enabled on one host: i386.
> Look for TCG_TARGET_DEFAULT_MO in tcg/*/tcg-target.h.
> 
> In order for MTTCG to be enabled, the target memory order must not be 
> stronger 
> than the host memory order.  Since i386 has a very strong host memory order, 
> it 
> is easy for it to emulate any guest.  When the host has a weak memory order, 
> we 
> need to add the additional barriers that are implied by the target.  This is 
> work that has not been done.
> 
> I am not sure why we have not already added this definition to all of the 
> other 
> tcg hosts.  I think this is just oversight, since almost everyone uses x86_64 
> linux as the host for testing tcg.  However, since all of the supported 
> targets 
> have weak memory orders we ought to be able to support them with any host.
In my case, I use Mips64 host and i386 target, does that mean I can not enable 


the MTTCG?




> >  3.
> > 
> > Is there any problem with the multithread programme running with 
> > linux-user
> > qemu mode? would the situation be improved with  the MTTCG feature?
> > 
> > We need to use linux-user mode qemu to run multithread app, but there 
> > seems
> > to be many problem.
> 
> For user mode, we should still follow the rules for MTTCG, but we do not. 
> Instead we take it on faith that they have been and execute the code in 
> parallel anyway.  This faith is often misplaced and it does mean that 
> unsupported targets execute user mode code incorrectly.
What do you exactly mean about the *unsupported targets*? mips? arm? i386? 


What is the main reason for the incorrectly execution of multithread app for 
user mode?

Is MTTCG helpful for that?

Specificly for my case(i386 target on Mips64 host in user mode),  how to 
improved the situation?




Thanks a lot for your detailed explaination.

Re: [Qemu-devel] [PATCH v2 00/45] tcg: support for multiple TCG contexts

[jiang.biao2]

Hi,

Seeing your work on multiple TCG, it seems that it has some kind of connection 
with the  MTTCG feature,

but I do not figure out how they are connected in detail.

Could you pls help to confirm the following questions:

what is the relationship between your patches and the MTTCG feature mentioned 
by https://lwn.net/Articles/697265/?

What is the current status of the development of the MTTCG feature?

Is
 there any problem with the multithread programme running with 
linux-user qemu mode? would the situation be improved with  the MTTCG 
feature?

We need to use linux-user mode qemu to run multithread app, but there seems to 
be many problem.





Thanks a lot.

[Qemu-devel] 答复: Re: [PATCH] tcg/mips: reserve a register for the guest_base.

[jiang.biao2]

> On 07/09/2017 11:12 PM, Jiang Biao wrote:
> > Reserve a register for the guest_base using ppc code for reference.
> > By doing so, we do not have to recompute it for every memory load.
> > 
> > Signed-off-by: Jiang Biao
> > Signed-off-by: Richard Henderson
> > ---
> >   tcg/mips/tcg-target.inc.c | 17 +
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> For future reference, you should not add a Signed-off-by for someone else. 
> That is like writing someone else's signature.
> 
> You may acknowledge my suggestion with a Suggested-by tag.
> 
> QEMU uses the same rules for this as does the linux kernel.  Please read
> 
> > https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst
> 
> or at least section 11 from
> 
>> https://github.com/torvalds/linux/blob/master/Documentation/translations/zh_CN/SubmittingPatches

> (The translation may be be out of date.  I don't see any of the other tags, 
> like Suggested-by, within the Chinese text.)
> 
> That said, applied to tcg-next.
Got that:-). Thanks for your advice.

Re: [Qemu-devel] [PATCH] tcg/mips: Bugfix for crashwhenrunningprogram with qemu-i386.

[jiang.biao2]

> > this section of code is to extend the addr_regl to 64bit, and use *base* as 
> > temp





> > intermedia. The real intention could be to extend addr_regl into base, and 
> > then
> > move base back to addr_regl for later use, but it wrongly assigning  base to
> > addr_regl directly, which will cause crash for every use of tcg_out_qemu_st.
> 
> The intent is to zero-extend addr_regl into a temporary base = A0, and at 
> this 
> point is known to be unused.  Afterward, whenever we talk about the low part 
> of 
> the address, we want to use the temporary and not the original input.
> 
> For example, if addr_regl = S0, and guest_base = S1, then we want
> 
> dexta0, s0, 31, 0
>dadda0, s1, a0
> 
> instead of
> 
> dexta0, s0, 31, 0
> movs0, a0
> dadda0, s1, s0
Got what you mean finally :-), it is better indeed. 


I'll send a new version patch for that, and send another patch trying 


to optimize the use of register by guest_base .

Re: [Qemu-devel] [PATCH] tcg/mips: Bugfix for crash whenrunningprogram with qemu-i386.

[jiang.biao2]

> On 07/09/2017 04:04 PM, jiang.bi...@zte.com.cn wrote:
> >  >>   if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
> >  >>   tcg_out_ext32u(s, base, addr_regl)
> >  >> -addr_regl = base
> >  >> +tcg_out_mov(s, TCG_TYPE_PTR, addr_regl, base)
> >  >>   }
> >  >>   if (guest_base == 0 && data_regl != addr_regl) {
> >  >>   base = addr_regl
> >  >
> >  > This is wrong, because you're not allowed to modify the input operands.
> >  >
> >  > Try this, just a few lines lower in the function:
> > 
> >  > -tcg_out_movi(s, TCG_TYPE_PTR, base, guest_base)
> >  > -tcg_out_opc_reg(s, ALIAS_PADD, base, base, addr_regl)
> >  > +tcg_out_movi(s, TCG_TYPE_PTR, TCG_TMP0, guest_base)
> >  > +tcg_out_opc_reg(s, ALIAS_PADD, base, TCG_TMP0, addr_regl)
> > 
> >  >
> > 
> > Got it, but the real problem is for addr_regl instead of guest_base.
> 
> Guest base is a problem simply because we require a temporary for it, and we 
> were trying to put two temporaries into the same register.
> 
> If we retain guest_base in a register all of the time, then (1) we do not 
> have 
> to recompute it for every memory load and (2) we do not need a temporary for 
> it.
> 


> > >  > Better would be to reserve a register for the guest_base, like we do 
> > > for ppc.
> >  > See all of the uses of TCG_GUEST_BASE_REG in tcg/ppc/tcg-target.inc.c.
> > 
> > It uses base(TCG_REG_A0) for temperary use for guest_base in this case.
> 
> No it doesn't.  It computes guest_base into a register in the prologue:
> 
> > #ifndef CONFIG_SOFTMMU
> > if (guest_base) {
> > tcg_out_movi(s, TCG_TYPE_PTR, TCG_GUEST_BASE_REG, guest_base)
> > tcg_regset_set_reg(s->reserved_regs, TCG_GUEST_BASE_REG)
> > }
> > #endif
> 
> and then it uses a reg+reg addressing mode during qemu_ld/st:
> 
> > rbase = guest_base ? TCG_GUEST_BASE_REG : 0
> 
> > insn = qemu_ldx_opc[opc & (MO_SIZE | MO_BSWAP)]
> > tcg_out32(s, insn | TAB(datalo, rbase, addrlo))
> 
> Obviously mips doesn't have a reg+reg addressing mode, so a PADD instruction 
> is 
> required, but otherwise you can use the same scheme.  Using TCG_REG_S1 on 
> mips 
> for TCG_GUEST_BASE_REG would be fine.



Understood. guest_base is a problem indeed, and it would be better to be like 
that.

However, the bug I mentioned here seems to have no direct connect with the 


guest_base problem. It lies in the following code, 


> if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
> tcg_out_ext32u(s, base, addr_regl)
>addr_regl = base //problem is here. 
>}

this section of code is to extend the addr_regl to 64bit, and use *base* as 
temp 


intermedia. The real intention could be to extend addr_regl into base, and then 


move base back to addr_regl for later use, but it wrongly assigning  base to 


addr_regl directly, which will cause crash for every use of tcg_out_qemu_st.

My intention is to fix this bug, and intend to use TCG_TMP0 for temporary use 


for addr_regl.




As for the guest_base problem, I'll try to optimize it later, taking ppc code 
for 


reference.





thanks for your advise and help.

Re: [Qemu-devel] [PATCH] tcg/mips: Bugfix for crash when runningprogram with qemu-i386.

[jiang.biao2]

>>   if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {



>>   tcg_out_ext32u(s, base, addr_regl)
>> -addr_regl = base
>> +tcg_out_mov(s, TCG_TYPE_PTR, addr_regl, base)
>>   }
>>   if (guest_base == 0 && data_regl != addr_regl) {
>>   base = addr_regl
> 
> This is wrong, because you're not allowed to modify the input operands.
> 
> Try this, just a few lines lower in the function:

> -tcg_out_movi(s, TCG_TYPE_PTR, base, guest_base)
> -tcg_out_opc_reg(s, ALIAS_PADD, base, base, addr_regl)
> +tcg_out_movi(s, TCG_TYPE_PTR, TCG_TMP0, guest_base)
> +tcg_out_opc_reg(s, ALIAS_PADD, base, TCG_TMP0, addr_regl)
>

Got it, but the real problem is for addr_regl instead of guest_base.


Using your code for reference, I'll use TCG_TMP0 for addr_regl instead 


of modifying it, and then send patch of new version later.





>


> and you'll need the same change within tcg_out_qemu_st.
I'll write a test demo to produce crash in tcg_out_qemu_st flow, and send

another patch for that.





> Better would be to reserve a register for the guest_base, like we do for ppc. 
> See all of the uses of TCG_GUEST_BASE_REG in tcg/ppc/tcg-target.inc.c.
It uses base(TCG_REG_A0) for temperary use for guest_base in this case.




Thanks a lot.

[Qemu-devel] 答复: Re: [PATCH] tcg/mips: Bugfix for crash when runningprogram with qemu-i386.

[jiang.biao2]

>



> This is wrong, because you're not allowed to modify the input operands.
> 
> Try this, just a few lines lower in the function:
> 
> -tcg_out_movi(s, TCG_TYPE_PTR, base, guest_base)
> -tcg_out_opc_reg(s, ALIAS_PADD, base, base, addr_regl)
> +tcg_out_movi(s, TCG_TYPE_PTR, TCG_TMP0, guest_base)
> +tcg_out_opc_reg(s, ALIAS_PADD, base, TCG_TMP0, addr_regl)
> 
> and you'll need the same change within tcg_out_qemu_st.
> 
> Better would be to reserve a register for the guest_base, like we do for ppc. 
> See all of the uses of TCG_GUEST_BASE_REG in tcg/ppc/tcg-target.inc.c.
Thanks a lot for the reviewing, I'll check on these.