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

[Peter Crosthwaite]

On Tue, Dec 17, 2013 at 10:54 AM, Peter Maydell
<address@hidden> wrote:
> On 17 December 2013 00:34, Edgar E. Iglesias <address@hidden> 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".)
>

A levelled approach to debugging can help here too rather than having
all-or-none with the QoM casts.

Regards,
Peter

> thanks
> -- PMM
>