[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH v2 6/9] tcg: implement mirror mapped JIT for iOS
Date: Tue, 20 Oct 2020 01:45:52 +0200 (CEST)

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

There are quite a bit of code that depends on tcg_insn_unit * type such as

*tcg_code_ptr_rw(s, code_ptr) = insn;


(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:


and I assume you already know this:


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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]