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

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 17 Aug 2017 14:53:08 +0200
David Hildenbrand <david@redhat.com> wrote:

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

What, you don't want to rewrite everything? :)

No, that was just an idea for the future, no need to expand your series
further.