Re: [Qemu-devel] [PATCH v1 for-2.11 09/10] s390x/kvm: move KVM declarations and stubs to separate files

[David Hildenbrand <david@redhat.com>]

On 17.08.2017 14:48, Cornelia Huck wrote:
> On Thu, 17 Aug 2017 14:35:53 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 17.08.2017 13:40, Philippe Mathieu-Daudé wrote:
>>> Hi David,
>>>
>>> On 08/17/2017 06:22 AM, David Hildenbrand wrote:  
>>>> Let's do it just like the other architectures. Introduce kvm-stub.c
>>>> for stubs and kvm_s390x.h for the declarations.
>>>>
>>>> Add a fake declaration of struct kvm_s390_irq so we don't need other
>>>> ugly CONFIG_KVM checks.  
>>>
>>> You can use an opaque pointer to avoid that ("bridge" design pattern).
>>>
>>> It involves few more changes but looks safer.  
>>
>> There is maybe even a simpler solution than that, see below ...
>>
>>>> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
>>>> new file mode 100644
>>>> index 0000000..35db28c
>>>> --- /dev/null
>>>> +++ b/target/s390x/kvm_s390x.h
>>>> @@ -0,0 +1,51 @@
>>>> +/*
>>>> + * QEMU KVM support -- s390x specific functions.
>>>> + *
>>>> + * Copyright (c) 2009 Ulrich Hecht
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>>> later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#ifndef KVM_S390X_H
>>>> +#define KVM_S390X_H
>>>> +
>>>> +#include "sysemu/kvm.h"
>>>> +
>>>> +#ifndef CONFIG_KVM
>>>> +struct kvm_s390_irq {};
>>>> +#endif /* CONFIG_KVM */  
>>>
>>> change by
>>>
>>> typedef struct kvm_s390_irq kvm_s390_irq_t;  
>>
>> May I suggest to simply use this instead:
>>
>> struct kvm_s390_irq;
>>
>> No need to switch for a typedef here, you can simply use this anonymous
>> struct declaration, I think.
> 
> Yes, I think so.
> 
> I'm wondering if there are more cleanup opportunities...
> 
> We currently have the flic which is supposed to deal with floating
> interrupts (in hw/intc/). It comes in a kvm (backed by a kvm device)
> and a non-kvm flavour. Recent changes have introduced some adapter
> interrupt handling in both the kvm device backing and in the non-kvm
> flic.
> 
> Creation/injection of floating interrupts (like I/O interrupts) is
> currently a bit scattered around the code, and the tcg code (which
> predates the flic) does not have much in common with the kvm code. Can
> we enhance the flic with one or more injection methods, move the tcg
> interrupt creation into the non-kvm flic, and get rid of
> kvm_s390_inject_flic()? Not sure how doable that is.
> 

There are many more possible cleanups, let's limit the scope of this
series. (it all started by wanting to move one function to cpu.h ...)

Thanks!

-- 

Thanks,

David