On 21.11.2012, at 02:56, David Gibson <da...@gibson.dropbear.id.au> 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 <ag...@suse.de> 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
