Re: [Qemu-devel] [PATCH v4 00/36] VMState port of all cpus

[Andreas Färber]

Am 21.03.2012 18:24, schrieb Juan Quintela:
> Andreas Färber <address@hidden> wrote:
>> Am 19.03.2012 23:57, schrieb Juan Quintela:
>>> This repository contains all the changes:
>>>
>>>   git://repo.or.cz/qemu/quintela.git vmstate-cpus-v4
>>>
>>> [v4]
>>> - rebase to top
>>> - adapt to vmstate.h change
>>> - adapt to CPUState -> CPU$archState rename
>>> - integrate arm changes in the meantime
>>> - add QEMU contributors to the copyright notice of ppc & sparc
> 
> [...]
>>
>> Actually I don't see any CCs at all in this series. Which makes me think
>> this is v1 rubbish in the new cover letter. :/
> 
> see the changelog for v4. All patches were already reviewed, only

There's no tags on the patches though, which equals unreviewed.
Especially I see no indication that Alex ack'ed ppc and s390x - do you
have links of where he did?

> changes were: copyright stuff that Blaw didn't liked (and he was cc'd),
> and rebasing (move of  stuff from hw/hw.h to vmstate.h and
> s/CPUState/CPU$archState/).
> 
> Idea here was not to resend to everybody that already reviewed the
> patches another copy.

My point was: Please remember to update the cover letter. In this case,
if no one is cc'ed, don't write they are (in the part you snipped above)
just because they were previously. Has nothing to do with what changes
you actually did in v4 and whether they should be cc'ed again (I'd
expect to be).

>> --cc-cmd="scripts/get_maintainer.pl --nogit-fallback" should work.
> 
> nice trick.

sendemail.cccmd (no dash) is even handier long-term. :)

>> With regards to the ongoing CPU QOM'ification, if we ever arrive in a
>> scenario where we can have multiple targets in one machine, I guess the
>> VMState .name "cpu" would cause problems? In that case it might be
>> better to use the proposed QOM type names, i.e. "arm-cpu", etc. from the
>> start.
> 
> At least for x86 we need to maintain backward compatibility.  For the
> cest of architectures, we can change it after this series.  It is more,
> it would be better to have:  "sparc32-cpu" and "sparc64-cpu", so we
> could be able to read the vmstate section without more external info.
> 
> I would preffer to do this changes after this series goes in.

It's unclear to me where that field is being used - is it being written
into the stream and does changing it mean a breakage of the format? Or
is this just for debugging?
If the former, then I think sparc32-cpu vs. sparc64-cpu is just candy
since I don't believe we'll manage to combine 32- and 64-bit CPUs
anytime soon.
My understanding was that this conversion series would generally
preserve the format and allow interoperability where no target-specific
changes are performed. Am I wrong if that only applies to x86 now?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg