Re: [Qemu-devel] [PATCH v1 08/22] cpu: Add per-cpu address space

[Peter Maydell <peter.maydell@linaro.org>]

On 17 December 2013 00:34, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Mon, Dec 16, 2013 at 01:11:47PM +0100, Andreas Färber wrote:
>> Why are you adding this field here rather than in CPUState alongside the
>> other field? This being a pointer I can't imagine problems for
>> non-softmmu, and I had posted patches a while ago to move the
>> surrounding fields there. Having it in CPUState would avoid some of the
>> env_ptr accesses above, which were supposed to be an interim solution only.

> This was discussed when I posted the RFC. My first try had this member in
> CPUState but some of the hot paths for mmio accesses needed to do
> ENV_GET_CPU() to get hold of the CS and AS. ENV_GET_CPU does a runtime type
> check that would slow things down. That's the reason I moved it to env.
>
> I'm not against moving the field back if it doesnt cost too much. Maybe we
> should consider removing the CPU() around ENV_GET_CPU()?
>
> See:
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02889.html

I think that's the wrong way round. We should put the field in the right
place in the code, which is CPUState. To the extent that that means we
discover that our dynamic cast macros are not fast enough for hot
paths we should make the actual error checking be debug builds
only. (There's been pushback on dynamic-casts in fastpaths before
and the preferred approach so far has been "optimise the cast".
I think we should take "...and do it only in debug builds" over "do
the wrong thing because a purely-debug-purposes type check
isn't as fast as we'd like".)

thanks
-- PMM