Re: [Qemu-ppc] [Qemu-devel] [PATCH 08/12] target-ppc: Convert ppcemb_tlb_t to use fixed 64-bit RPN

[David Gibson]

On Wed, Nov 21, 2012 at 02:48:07AM +0100, Alexander Graf wrote:
> 
> On 21.11.2012, at 02:14, David Gibson wrote:
> 
> > On Tue, Nov 20, 2012 at 10:55:50AM +0100, Alexander Graf wrote:
> >> 
> >> On 20.11.2012, at 10:53, Peter Maydell wrote:
> >> 
> >>> On 20 November 2012 09:29, Alexander Graf <address@hidden> wrote:
> >>>> On 19.11.2012, at 23:48, David Gibson wrote:
> >>>>> On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote:
> >>>>>> On 13.11.2012, at 03:46, David Gibson wrote:
> >>>>>>> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit 
> >>>>>>> integer
> >>>>>>> which we know is sufficient for all the machines which use this 
> >>>>>>> structure.
> >>>>>> 
> >>>>>> hwaddr is always defined to 64bit by now.
> >>>>> 
> >>>>> I know, but there aren't state save helpers for hwaddr, and there are
> >>>>> objections to creating them.
> >>> 
> >>> (previous discussion on this point:
> >>> https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg01456.html )
> >>> 
> >>>> Sure, but you can just use the 64bit save helpers now that hwaddr == 
> >>>> uint64_t, no?
> >>> 
> >>> That would be one approach. I'm a bit sceptical about putting hwaddr
> >>> fields in CPU state, though -- it's suggestive that something's
> >>> not modelled right. hwaddr is conceptually "big enough for the
> >>> biggest bus in the system", and no single component should have
> >>> internal state whose size depends on that.
> > 
> > Right, that's the reason I was given for not adding VMSTATE helpers
> > for hwaddr too.
> > 
> > But more directly, as long as hwaddr is a different type from
> > uint64_t, to me that at least admits the possibility that it could be
> > changed again some day.  And if we're using a uint64_t based VMSTATE
> > helper on a type that could change, that could go badly wrong.
> > Basically it's a subtle and ungreppable dependency on the fact that a
> > hwaddr is actually a uint64_t, which seems like a bad idea.
> > 
> >> *shrug* I'm more than happy to get a patch that just converts all
> >> *the hwaddr fields in CPUState to uint64_t.
> > 
> > So.. does that mean you'll apply this one or not?
> 
> It means I'll wait for one that converts more than just this one
> field :). According to the above rationale, there shouldn't be any
> hwaddr fields in CPUState, right?

Grah.  Why is that qemu people always seem to insist on not fixing
something that needs fixing unless everything else that needs fixing
is done at the same time.

In any case, I don't think that's strictly correct.  The point is that
fields which represent architected CPU state should never be hwaddr,
but it's at least possible that it could be appropriate for some
anciliary data.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson