Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/6] target-ppc: Synchronize more SPRs to KVM using ONE_REG interface

[David Gibson]

On Fri, Jan 25, 2013 at 12:24:06PM +0100, Alexander Graf wrote:
> On 25.01.2013, at 03:39, David Gibson wrote:
> > On Thu, Jan 24, 2013 at 05:32:42PM +0100, Alexander Graf wrote:
[snip]
> >>> +static void kvm_get_one_spr(CPUState *cs, uint64_t id, int spr)
> >>> +{
> >>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>> +    CPUPPCState *env = &cpu->env;
> >>> +    union {
> >>> +        uint32_t u32;
> >>> +        uint64_t u64;
> >>> +    } val;
> >>> +    struct kvm_one_reg reg = {
> >>> +        .id = id,
> >>> +        .addr = (uintptr_t) &val,
> >>> +    };
> >>> +    int ret;
> >>> +
> >>> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> >>> +    if (ret != 0) {
> >>> +        fprintf(stderr, "Warning: Unable to retrieve SPR %d from KVM: 
> >>> %s\n",
> >>> +                spr, strerror(errno));
> >> 
> >> Any way you could make this a WARN_ONCE style message?
> > 
> > Uh.. I guess.  Afaict, qemu doesn't have any infrastructure for that,
> > so I'd have to roll my own.
> 
> As I stated in a later patch review, I think it makes sense to just
> not print anything by default.
> 
> Usually, you would do
> 
> if (cap_foo) {
>     get_one_spr(...);
> }
> 
> right? With one_reg I consciously omitted caps for every register,
> because you can just as well read and push the reg itself and thus
> know whether it works.
> 
> So if you translate that to the above code, it would mean
> 
>    get_one_spr(...);
> 
> without error message would be semantically identical to the first
> version.
> 
> It would however make sense to have a debug print version in case
> you want to know why something goes wrong. It might also make sense
> to have an initial get_one_spr() or so for a FULL register push to
> warn the user that live migration will not work on his machine.

So, I realised I need to rework this substantially anyway.  As you
say, I need to not attempt to sync these various SPRs for BookE.  But
more generally, different CPUs will have a different set of supported
SPRs.

I have a draft in the works that adds a ONE_REG id code to the
ppc_spr_t structure, which is (optionally) initialized by
spr_register() calls in the cpu initialization.  Then the kvm call
goes through the SPR table and just syncs all those with registered
ONE_REG ids.  I'm at linux.conf.au this week, so I don't expect to
finish that off until next week.  There are some uglies in that that I
still need to see if can be resolved yet.

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

signature.asc
Description: Digital signature