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

[Alexander Graf]

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?


Alex