Re: [PATCH] Make S390x work in qemu-kvm

[Avi Kivity <avi@redhat.com>]

On 12/15/2009 01:13 PM, Alexander Graf wrote:
>
>> This should be CONFIG_PCI or CONFIG_PCI_MSI.  Also, better to hack it
>> at the Makefile level.
>>      
> That's what I did at first in a hacky way. To be honest, the new
> makefile structure scares me a bit, so I'm not sure I'll easily figure
> out how to do that properly :).
>    

I'm sure Juan will be glad to help.

>>> +
>>> +#ifndef KVM_UPSTREAM
>>> +    kvm_arch_load_regs(env);
>>> +#endif
>>>    }
>>>
>>>        
>> Why isn't cpu_synchronize_state() suitable?
>>      
> Because the KVM fd's are not available yet.
>    

Then kvm_arch_load_regs() will fail as well, no?

>>>    static int get_free_slot(kvm_context_t kvm)
>>>    {
>>> -    int i;
>>> +    int i = 0;
>>>        int tss_ext;
>>>
>>>    #if defined(KVM_CAP_SET_TSS_ADDR)&&   !defined(__s390__)
>>> @@ -171,10 +171,12 @@ static int get_free_slot(kvm_context_t kvm)
>>>         * slot 0 to hold the extended memory, as the vmx will use the
>>> last 3
>>>         * pages of this slot.
>>>         */
>>> +#ifndef __s390x__
>>>        if (tss_ext>   0)
>>>            i = 0;
>>>        else
>>>            i = 1;
>>> +#endif
>>>
>>>        
>> That should conditioned on x86, not s390.  While you're at it, drop
>> the i = 0 assignment please.
>>      
> So it's useless for IA64 as well?
>    

Yes.

>>>        for (; i<   KVM_MAX_NUM_MEM_REGIONS; ++i)
>>>            if (!slots[i].len)
>>> @@ -450,6 +452,14 @@ static void kvm_create_vcpu(CPUState *env, int id)
>>>        env->kvm_fd = r;
>>>        env->kvm_state = kvm_state;
>>>
>>> +#ifdef __s390x__
>>> +    r = kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, 0);
>>> +    if (r<   0) {
>>> +        fprintf(stderr, "kvm_s390_initial_reset: %m\n");
>>> +        exit(1);
>>> +    }
>>> +#endif
>>>
>>>        
>> Isn't there an arch hook for this?
>>
>> TARGET_S390 or similar.
>>      
> Yes, there is. I figured this is just a temporary hack, so who cares :-).
>    

The whole of qemu-kvm.c is a temporary hack.  No reason to make it 
uglier than it needs to be.

>>>                break;
>>> -#if defined(__s390__)
>>> -        case KVM_EXIT_S390_SIEIC:
>>> -            r = kvm_s390_handle_intercept(kvm, env, run);
>>> -            break;
>>> -        case KVM_EXIT_S390_RESET:
>>> -            r = kvm_s390_handle_reset(kvm, env, run);
>>> -            break;
>>> -#endif
>>>
>>>        
>> Ditto.
>>      
> Ditto what? This is code removal.
>    

Um, yes.

>>> +#ifdef __s390x__
>>> +static
>>> +#endif
>>>
>>>        
>> Lovely.  Why?
>>      
> Because it collided with the init function provided by target-s390x/kvm.c.
>    

I'd prefer a separate rename then.

>>>    int kvm_arch_init_vcpu(CPUState *env)
>>>    {
>>> @@ -86,12 +88,27 @@ int kvm_arch_init_vcpu(CPUState *env)
>>>        return ret;
>>>    }
>>>
>>> +#ifdef KVM_UPSTREAM
>>>    void kvm_arch_reset_vcpu(CPUState *env)
>>> +#else
>>> +void kvm_arch_cpu_reset(CPUState *env)
>>> +#endif
>>>
>>>        
>> :(
>>      
> Yeah, feel like getting the naming a bit closer? :)
>    

A renaming patch would be welcome.

>> I thought we no longer include libkvm.h!
>>
>>      
> Some file failed to build without it. IIRC because PAGE_* was not defined.
>    

There's a TARGET_PAGE_SIZE or something, we can use that instead.

-- 
error compiling committee.c: too many arguments to function