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

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Tue, Dec 17, 2013 at 12:54:19AM +0000, Peter Maydell wrote:
> 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".)

Sounds reasonable to me, I'll move the AS back into CPUState for v2.

Thanks,
Edgar