Re: [Qemu-devel] [PATCH] kvm: ppc: fixes for KVM_SET_SREGS on init

[Alexander Graf]

On 11.04.2011, at 18:15, Scott Wood wrote:

> On Sat, 9 Apr 2011 02:18:34 +0200
> Alexander Graf <address@hidden> wrote:
> 
>>> -int kvm_arch_init_vcpu(CPUState *cenv)
>>> +static int kvm_arch_sync_sregs(CPUState *cenv)
>> 
>> huh? So what about the previous caller of this?
> 
> It's a new function.  kvm_arch_init_vcpu still exists as a public
> function, "introduced" later in the patch.  Diff doesn't know why this line
> is more important than the sregs definition.
> 
>>> {
>>> -    int ret = 0;
>>>    struct kvm_sregs sregs;
>>> +    int ret;
>> 
>> Eh - this makes the patch less readable :)
> 
> I can flip them around in the new function if you want, though having the
> longer declaration first looks a bit nicer to me.
> 
>>> +#ifdef TARGET_PPC
>>> +#ifdef KVM_CAP_PPC_SEGSTATE
>> 
>> This code never gets compiled without TARGET_PPC?
> 
> Hmm, thought I checked that TARGET_PPC wasn't set in a TARGET_PPCEMB build,
> but now I see it is.  Would be nice if we had a define specifically for
> non-PPCEMB.
> 
>>> +    if (!kvm_check_extension(cenv->kvm_state, KVM_CAP_PPC_SEGSTATE)) {
>>> +        return 0;
>>> +    }
>>> +#else
>>> +    return 0;
>> 
>> Doing a simple return 0 might lead to warnings (which become errors with 
>> -Werror) due to unused variables. I'm not sure how to handle this well. 
>> Maybe define KVM_CAP_PPC_SEGSTATE to something invalid when it's not 
>> defined? That way the capability check would fail and we don't need the 
>> duplicate code paths.
> 
> Which variables would be unused?  sregs/ret are used, just in a dead
> portion of the function.  If the rest of the function had been ifdeffed out
> instead, it would be an issue.

I've had gcc be overly clever in situations like this before, where it detects 
the return is unconditional and just removes all the code after it, generating 
unused warnings.

> 
>>> +#endif
>>> +#else /* TARGET_PPCEMB */
>> 
>> I guess you were #ifdefing on PPCEMB before? I don't think you really need 
>> to care about PPCEMB. The only reason it exists is for page size < 4k, which 
>> you don't hit on e500 IIUC.
> 
> PPCEMB is how we've been running this so far... it also involves a larger
> target_phys_addr_t.  I didn't know it was supposed to be supported at all
> under plain PPC.
> 
> If that really is supposed to be supported, then we'll need a dynamic check
> here instead (based on excp_model?), but I don't see the value in
> supporting that.  I did find it odd that all ppc platforms are being built
> for both PPC and PPCEMB.

I don't disagree that it's odd, but that's the way it is. We even support 
32-bit CPUs in the PPC64 target. This patch surely isn't in the scope of 
changing it :). So yes, all the code has to work with qemu-system-ppc.


Alex