Re: [PATCH v2 6/9] tcg: implement mirror mapped JIT for iOS

[BALATON Zoltan via <qemu-devel@nongnu.org>]

On Mon, 19 Oct 2020, Joelle van Dyne wrote:
>> Explicit cast may not be needed here so this could be a macro if caling it
>> differently helps or why don't you just use tcg_mirror_prr_rw directly
>> everywhere?
>
> There are quite a bit of code that depends on tcg_insn_unit * type such as
>
> *tcg_code_ptr_rw(s, code_ptr) = insn;
>
> and
>
> (tcg_code_ptr_rw(s, p))[i] = NOP;

OK that explains it, haven't looked at it at that detail.

> I think it's cleaner to not have to manually cast in every one of 30+
> instances of this. In v1, I used a macro but was told to use an inline
> function instead.

Definitely cleaner to have the cast either in a macro or inline func than 
manually having it everywhere. The static inline in v2 looks better than 
the macro in v1 so unless others disagree it's fine this way, I'm not the 
one who decides, I was just asking if we can avoid having two static 
inlines relying on casting void * but if you also dereference as above 
then returning the right type is needed. Let's see what Richard says who 
suggested the function instead of a macro but it does look more readable 
than the previous macro.

>> Is that !defined or are you missing an implementation and #else here?
> No, `flush_dcache_range` is only needed when mirror mapped (after
> writing to the RW mirror). Now there is no iOS compatible compiler for
> any other arch than x86 and ARM. However, in the slim chance that

But this was in tcg/arm/tcg-target.h which is ARM but maybe you mean only 
x86 and 64bit ARM which is aarch64 but not 32bit ARM. I've noticed this 
only after sending the question.

> Apple decides to change arch again in the future and moves to RISC-V
> or something, then we get a nice compiler error.
>
>> Maybe this patch could be split up some more, making the RW offset
>> handling and cache management separate patches even if they don't work
>> separately just to make it easier to review.
>
> I can probably do that for v3 but imo most of the LOC here is because
> the same change has to be done to every TCG target. No matter how you
> split up the patches, it will look like a lot of changes.

Sure but it's easier to review if a single patch has only similar changes 
even if it touches a lot of files than if it does independent stuff 
intermixed unless it's a small patch (but even then QEMU tends to prefer a 
lot of smaller patches instead of combining changes in a single patch). 
That's also good for bisectability so that's also something to consider 
when splitting patches. Not sure if in this case this can be split up into 
two working changes because RW mapping may not work without cache flushes 
and cache flushes may not be added before having the RW split but having 
two patches for the review that can be squashed in the final series may 
still help. But lets see if this gets reviewed without further splitting 
or what others say.

Not sure you're aware of this: https://wiki.qemu.org/Planning/5.2 but if 
this series does not get merged this week don't be surprised if your next 
opportunity to pick it up will be in December (when most people who can 
review it will be on holyday so it can be easily take longer). So maybe 
you could try pushing it and do everything to make reviewers' job easier 
if you want it in the next release. Otherwise you'll have time to polish 
it until next year.

Also it may be good to fix checkpatch errors (warnings may be OK but 
errors are not) that are reported even if it's not in your code (it could 
be a separate clean up patch before your changes or for small things in 
the same patch) otherwise automated tests may not run which can also delay 
reviews and merging:

https://patchew.org/QEMU/20201019051953.90107-1-j@getutm.app/

and I assume you already know this:

https://wiki.qemu.org/Contribute/SubmitAPatch

It might be overwhelming and off putting sometimes to try getting series 
into QEMU but please don't give up.

Regards,
BALATON Zoltan