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:56, David Gibson <address@hidden> wrote:

> 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.

Because that's the only way to keep the code consistent.

> 
> 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.

Yup. Please double-check all fields. I wouldn't be surprised if there are more 
fields that you would want to save/restore that are hwaddr.

Alex

> 
> -- 
> 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