| Message ID | 20170817092225.4264-10-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
>> +#include "qemu/osdep.h" >> +#include "qemu-common.h" >> +#include "cpu.h" >> +#include "kvm_s390x.h" > > here goes the: > > struct kvm_s390_irq {}; As we don't have a stub using kvm_s390_irq, I guess I can just drop this for now. [...] > > change by > > typedef struct kvm_s390_irq kvm_s390_irq_t; > >> + >> +void kvm_s390_floating_interrupt(struct kvm_s390_irq *irq); >> +void kvm_s390_service_interrupt(uint32_t parm); >> +void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq); > > change these to use 'kvm_s390_irq_t *irq' arg > >> +void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code); >> +int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, >> + int len, bool is_write); >> +void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code); >> +void kvm_s390_io_interrupt(uint16_t subchannel_id, >> + uint16_t subchannel_nr, uint32_t io_int_parm, >> + uint32_t io_int_word); >> +void kvm_s390_crw_mchk(void); >> +int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state); >> +void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu); >> +int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu); >> +int kvm_s390_get_ri(void); >> +int kvm_s390_get_gs(void); >> +int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock); >> +int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock); >> +void kvm_s390_enable_css_support(S390CPU *cpu); >> +int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, >> + int vq, bool assign); >> +int kvm_s390_cpu_restart(S390CPU *cpu); >> +int kvm_s390_get_memslot_count(KVMState *s); >> +int kvm_s390_cmma_active(void); >> +void kvm_s390_cmma_reset(void); >> +void kvm_s390_reset_vcpu(S390CPU *cpu); >> +int kvm_s390_set_mem_limit(KVMState *s, uint64_t new_limit, uint64_t *hw_limit); >> +void kvm_s390_crypto_reset(void); >> + >> +/* implemented outside of target/s390x/ */ >> +int kvm_s390_inject_flic(struct kvm_s390_irq *irq); > > also change this to use 'kvm_s390_irq_t *irq' arg here and in hw/intc/ > Yes, this seems to compile!
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 ... [...] >> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index 74d5b35..aeb730c 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -41,6 +41,7 @@ >> #include "exec/cpu-all.h" >> #include "fpu/softfloat.h" >> +#include "kvm_s390x.h" Do we still need that? cpu.h should theoretically be independent from kvm now, shouldn't it? And for the .c files, it's likely better to include kvm_s390x.h directly there if they require it. [...] >> 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. Thomas
On 08/17/2017 09:35 AM, Thomas Huth 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 ... > > [...] >>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>> index 74d5b35..aeb730c 100644 >>> --- a/target/s390x/cpu.h >>> +++ b/target/s390x/cpu.h >>> @@ -41,6 +41,7 @@ >>> #include "exec/cpu-all.h" >>> #include "fpu/softfloat.h" >>> +#include "kvm_s390x.h" > > Do we still need that? cpu.h should theoretically be independent from > kvm now, shouldn't it? And for the .c files, it's likely better to > include kvm_s390x.h directly there if they require it. Oh you totally right, I missed this include. [...] >>> +#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.
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.
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!
On 17.08.2017 14:35, Thomas Huth 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 ... > > [...] >>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>> index 74d5b35..aeb730c 100644 >>> --- a/target/s390x/cpu.h >>> +++ b/target/s390x/cpu.h >>> @@ -41,6 +41,7 @@ >>> #include "exec/cpu-all.h" >>> #include "fpu/softfloat.h" >>> +#include "kvm_s390x.h" > > Do we still need that? cpu.h should theoretically be independent from > kvm now, shouldn't it? And for the .c files, it's likely better to > include kvm_s390x.h directly there if they require it. It should work if: a) we include "sysemu/kvm.h" in hw/s390x/s390-virtio-ccw.c b) we include "target/s390x/kvm_s390x.h" in hw/intc/s390_flic_kvm.c c) we include "kvm_s390x.h" in "internal.h" d) we drop the "KVMState" parameter from kvm_s390_get_memslot_count() (separate patch) > > May I suggest to simply use this instead: > > struct kvm_s390_irq; That also seems to compile just fine. > > No need to switch for a typedef here, you can simply use this anonymous > struct declaration, I think. > > Thomas >
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.
On 17.08.2017 14:55, David Hildenbrand wrote: > On 17.08.2017 14:35, Thomas Huth wrote: >> On 17.08.2017 13:40, Philippe Mathieu-Daudé wrote: >>> On 08/17/2017 06:22 AM, David Hildenbrand wrote: [...] >>>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>>> index 74d5b35..aeb730c 100644 >>>> --- a/target/s390x/cpu.h >>>> +++ b/target/s390x/cpu.h >>>> @@ -41,6 +41,7 @@ >>>> #include "exec/cpu-all.h" >>>> #include "fpu/softfloat.h" >>>> +#include "kvm_s390x.h" >> >> Do we still need that? cpu.h should theoretically be independent from >> kvm now, shouldn't it? And for the .c files, it's likely better to >> include kvm_s390x.h directly there if they require it. > > It should work if: > > a) we include "sysemu/kvm.h" in hw/s390x/s390-virtio-ccw.c > b) we include "target/s390x/kvm_s390x.h" in hw/intc/s390_flic_kvm.c > c) we include "kvm_s390x.h" in "internal.h" > d) we drop the "KVMState" parameter from kvm_s390_get_memslot_count() > (separate patch) Ok, that's rather a lot of changes already. Maybe that's rather something for a later patch instead, so I'm also fine if you keep in #include "kvm_s390x.h" in cpu.h here. Thomas
On Thu, 17 Aug 2017 15:06:10 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 17.08.2017 14:55, David Hildenbrand wrote: > > On 17.08.2017 14:35, Thomas Huth wrote: > >> On 17.08.2017 13:40, Philippe Mathieu-Daudé wrote: > >>> On 08/17/2017 06:22 AM, David Hildenbrand wrote: > [...] > >>>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ > >>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > >>>> index 74d5b35..aeb730c 100644 > >>>> --- a/target/s390x/cpu.h > >>>> +++ b/target/s390x/cpu.h > >>>> @@ -41,6 +41,7 @@ > >>>> #include "exec/cpu-all.h" > >>>> #include "fpu/softfloat.h" > >>>> +#include "kvm_s390x.h" > >> > >> Do we still need that? cpu.h should theoretically be independent from > >> kvm now, shouldn't it? And for the .c files, it's likely better to > >> include kvm_s390x.h directly there if they require it. > > > > It should work if: > > > > a) we include "sysemu/kvm.h" in hw/s390x/s390-virtio-ccw.c > > b) we include "target/s390x/kvm_s390x.h" in hw/intc/s390_flic_kvm.c > > c) we include "kvm_s390x.h" in "internal.h" > > d) we drop the "KVMState" parameter from kvm_s390_get_memslot_count() > > (separate patch) > > Ok, that's rather a lot of changes already. Maybe that's rather > something for a later patch instead, so I'm also fine if you keep in > #include "kvm_s390x.h" in cpu.h here. Yup, let's defer it. It's not like that is the last series that will ever go in.
On 17.08.2017 15:07, Cornelia Huck wrote: > On Thu, 17 Aug 2017 15:06:10 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 17.08.2017 14:55, David Hildenbrand wrote: >>> On 17.08.2017 14:35, Thomas Huth wrote: >>>> On 17.08.2017 13:40, Philippe Mathieu-Daudé wrote: >>>>> On 08/17/2017 06:22 AM, David Hildenbrand wrote: >> [...] >>>>>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >>>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>>>>> index 74d5b35..aeb730c 100644 >>>>>> --- a/target/s390x/cpu.h >>>>>> +++ b/target/s390x/cpu.h >>>>>> @@ -41,6 +41,7 @@ >>>>>> #include "exec/cpu-all.h" >>>>>> #include "fpu/softfloat.h" >>>>>> +#include "kvm_s390x.h" >>>> >>>> Do we still need that? cpu.h should theoretically be independent from >>>> kvm now, shouldn't it? And for the .c files, it's likely better to >>>> include kvm_s390x.h directly there if they require it. >>> >>> It should work if: >>> >>> a) we include "sysemu/kvm.h" in hw/s390x/s390-virtio-ccw.c >>> b) we include "target/s390x/kvm_s390x.h" in hw/intc/s390_flic_kvm.c >>> c) we include "kvm_s390x.h" in "internal.h" >>> d) we drop the "KVMState" parameter from kvm_s390_get_memslot_count() >>> (separate patch) >> >> Ok, that's rather a lot of changes already. Maybe that's rather >> something for a later patch instead, so I'm also fine if you keep in >> #include "kvm_s390x.h" in cpu.h here. > > Yup, let's defer it. It's not like that is the last series that will > ever go in. > Sorry guys, already creating patches :)
On 17.08.2017 15:10, David Hildenbrand wrote: > On 17.08.2017 15:07, Cornelia Huck wrote: >> On Thu, 17 Aug 2017 15:06:10 +0200 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> On 17.08.2017 14:55, David Hildenbrand wrote: >>>> On 17.08.2017 14:35, Thomas Huth wrote: >>>>> On 17.08.2017 13:40, Philippe Mathieu-Daudé wrote: >>>>>> On 08/17/2017 06:22 AM, David Hildenbrand wrote: >>> [...] >>>>>>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >>>>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>>>>>> index 74d5b35..aeb730c 100644 >>>>>>> --- a/target/s390x/cpu.h >>>>>>> +++ b/target/s390x/cpu.h >>>>>>> @@ -41,6 +41,7 @@ >>>>>>> #include "exec/cpu-all.h" >>>>>>> #include "fpu/softfloat.h" >>>>>>> +#include "kvm_s390x.h" >>>>> >>>>> Do we still need that? cpu.h should theoretically be independent from >>>>> kvm now, shouldn't it? And for the .c files, it's likely better to >>>>> include kvm_s390x.h directly there if they require it. >>>> >>>> It should work if: >>>> >>>> a) we include "sysemu/kvm.h" in hw/s390x/s390-virtio-ccw.c >>>> b) we include "target/s390x/kvm_s390x.h" in hw/intc/s390_flic_kvm.c >>>> c) we include "kvm_s390x.h" in "internal.h" >>>> d) we drop the "KVMState" parameter from kvm_s390_get_memslot_count() >>>> (separate patch) >>> >>> Ok, that's rather a lot of changes already. Maybe that's rather >>> something for a later patch instead, so I'm also fine if you keep in >>> #include "kvm_s390x.h" in cpu.h here. >> >> Yup, let's defer it. It's not like that is the last series that will >> ever go in. >> > > Sorry guys, already creating patches :) > s/guys/folks/ ;)
On 08/17/2017 10:06 AM, Thomas Huth wrote: > On 17.08.2017 14:55, David Hildenbrand wrote: >> On 17.08.2017 14:35, Thomas Huth wrote: >>> On 17.08.2017 13:40, Philippe Mathieu-Daudé wrote: >>>> On 08/17/2017 06:22 AM, David Hildenbrand wrote: > [...] >>>>> feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ >>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>>>> index 74d5b35..aeb730c 100644 >>>>> --- a/target/s390x/cpu.h >>>>> +++ b/target/s390x/cpu.h >>>>> @@ -41,6 +41,7 @@ >>>>> #include "exec/cpu-all.h" >>>>> #include "fpu/softfloat.h" >>>>> +#include "kvm_s390x.h" >>> >>> Do we still need that? cpu.h should theoretically be independent from >>> kvm now, shouldn't it? And for the .c files, it's likely better to >>> include kvm_s390x.h directly there if they require it. >> >> It should work if: >> >> a) we include "sysemu/kvm.h" in hw/s390x/s390-virtio-ccw.c >> b) we include "target/s390x/kvm_s390x.h" in hw/intc/s390_flic_kvm.c that sounds odd... but git grep gives: hw/i386/kvm/apic.c:19:#include "target/i386/kvm_i386.h" hw/ppc/spapr_cpu_core.c:17:#include "target/ppc/kvm_ppc.h" >> c) we include "kvm_s390x.h" in "internal.h" >> d) we drop the "KVMState" parameter from kvm_s390_get_memslot_count() >> (separate patch) > > Ok, that's rather a lot of changes already. Maybe that's rather > something for a later patch instead, so I'm also fine if you keep in > #include "kvm_s390x.h" in cpu.h here. there is still time until 2.11 :)
On 08/17/2017 02: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. > > Change license to GPL2+ and keep copyright notice. > > Suggested-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/Makefile.objs | 1 + > target/s390x/cpu.h | 121 +-------------------------------------------- > target/s390x/kvm-stub.c | 111 +++++++++++++++++++++++++++++++++++++++++ > target/s390x/kvm_s390x.h | 51 +++++++++++++++++++ > 4 files changed, 164 insertions(+), 120 deletions(-) > create mode 100644 target/s390x/kvm-stub.c > create mode 100644 target/s390x/kvm_s390x.h Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/s390x/Makefile.objs b/target/s390x/Makefile.objs index f42cd1f..9615256 100644 --- a/target/s390x/Makefile.objs +++ b/target/s390x/Makefile.objs @@ -3,6 +3,7 @@ obj-$(CONFIG_TCG) += translate.o cc_helper.o excp_helper.o fpu_helper.o obj-$(CONFIG_TCG) += int_helper.o mem_helper.o misc_helper.o obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o diag.o obj-$(CONFIG_KVM) += kvm.o +obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o # build and run feature list generator feat-src = $(SRC_PATH)/target/$(TARGET_BASE_ARCH)/ diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 74d5b35..aeb730c 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -41,6 +41,7 @@ #include "exec/cpu-all.h" #include "fpu/softfloat.h" +#include "kvm_s390x.h" #define NB_MMU_MODES 3 #define TARGET_INSN_START_EXTRA_WORDS 1 @@ -213,8 +214,6 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env) extern const struct VMStateDescription vmstate_s390_cpu; #endif -#include "sysemu/kvm.h" - /* distinguish between 24 bit and 31 bit addressing */ #define HIGH_ORDER_BIT 0x80000000 @@ -407,39 +406,6 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void s390_enable_css_support(S390CPU *cpu); int s390_virtio_hypercall(CPUS390XState *env); -#ifdef CONFIG_KVM -void kvm_s390_service_interrupt(uint32_t parm); -void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq); -void kvm_s390_floating_interrupt(struct kvm_s390_irq *irq); -int kvm_s390_inject_flic(struct kvm_s390_irq *irq); -void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code); -int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, - int len, bool is_write); -int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock); -int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock); -#else -static inline void kvm_s390_service_interrupt(uint32_t parm) -{ -} -static inline int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low) -{ - return -ENOSYS; -} -static inline int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low) -{ - return -ENOSYS; -} -static inline int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, - void *hostbuf, int len, bool is_write) -{ - return -ENOSYS; -} -static inline void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, - uint64_t te_code) -{ -} -#endif - int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low); int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low); @@ -706,91 +672,6 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, #define ILEN_AUTO 0xff void program_interrupt(CPUS390XState *env, uint32_t code, int ilen); -#ifdef CONFIG_KVM -void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code); -void kvm_s390_io_interrupt(uint16_t subchannel_id, - uint16_t subchannel_nr, uint32_t io_int_parm, - uint32_t io_int_word); -void kvm_s390_crw_mchk(void); -void kvm_s390_enable_css_support(S390CPU *cpu); -int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, - int vq, bool assign); -int kvm_s390_cpu_restart(S390CPU *cpu); -int kvm_s390_get_memslot_count(KVMState *s); -int kvm_s390_cmma_active(void); -void kvm_s390_cmma_reset(void); -int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state); -void kvm_s390_reset_vcpu(S390CPU *cpu); -int kvm_s390_set_mem_limit(KVMState *s, uint64_t new_limit, uint64_t *hw_limit); -void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu); -int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu); -int kvm_s390_get_ri(void); -int kvm_s390_get_gs(void); -void kvm_s390_crypto_reset(void); -#else -static inline void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code) -{ -} -static inline void kvm_s390_io_interrupt(uint16_t subchannel_id, - uint16_t subchannel_nr, - uint32_t io_int_parm, - uint32_t io_int_word) -{ -} -static inline void kvm_s390_crw_mchk(void) -{ -} -static inline void kvm_s390_enable_css_support(S390CPU *cpu) -{ -} -static inline int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, - uint32_t sch, int vq, - bool assign) -{ - return -ENOSYS; -} -static inline int kvm_s390_cpu_restart(S390CPU *cpu) -{ - return -ENOSYS; -} -static inline void kvm_s390_cmma_reset(void) -{ -} -static inline int kvm_s390_get_memslot_count(KVMState *s) -{ - return MAX_AVAIL_SLOTS; -} -static inline int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) -{ - return -ENOSYS; -} -static inline void kvm_s390_reset_vcpu(S390CPU *cpu) -{ -} -static inline int kvm_s390_set_mem_limit(KVMState *s, uint64_t new_limit, - uint64_t *hw_limit) -{ - return 0; -} -static inline void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) -{ -} -static inline int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) -{ - return 0; -} -static inline int kvm_s390_get_ri(void) -{ - return 0; -} -static inline int kvm_s390_get_gs(void) -{ - return 0; -} -static inline void kvm_s390_crypto_reset(void) -{ -} -#endif int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit); void s390_cmma_reset(void); diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c new file mode 100644 index 0000000..29a89ba --- /dev/null +++ b/target/s390x/kvm-stub.c @@ -0,0 +1,111 @@ +/* + * QEMU KVM support -- s390x specific function stubs. + * + * 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. + */ + +#include "qemu/osdep.h" +#include "qemu-common.h" +#include "cpu.h" +#include "kvm_s390x.h" + +void kvm_s390_service_interrupt(uint32_t parm) +{ +} + +void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code) +{ +} + +int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, + int len, bool is_write) +{ + return -ENOSYS; +} + +void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code) +{ +} + +void kvm_s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, + uint32_t io_int_parm, uint32_t io_int_word) +{ +} + +void kvm_s390_crw_mchk(void) +{ +} + +int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) +{ + return -ENOSYS; +} + +void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) +{ +} + +int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) +{ + return 0; +} + +int kvm_s390_get_ri(void) +{ + return 0; +} + +int kvm_s390_get_gs(void) +{ + return 0; +} + +int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_low) +{ + return -ENOSYS; +} + +int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low) +{ + return -ENOSYS; +} + +void kvm_s390_enable_css_support(S390CPU *cpu) +{ +} + +int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, + int vq, bool assign) +{ + return -ENOSYS; +} + +int kvm_s390_cpu_restart(S390CPU *cpu) +{ + return -ENOSYS; +} + +void kvm_s390_cmma_reset(void) +{ +} + +int kvm_s390_get_memslot_count(KVMState *s) +{ + return MAX_AVAIL_SLOTS; +} + +void kvm_s390_reset_vcpu(S390CPU *cpu) +{ +} + +int kvm_s390_set_mem_limit(KVMState *s, uint64_t new_limit, uint64_t *hw_limit) +{ + return 0; +} + +void kvm_s390_crypto_reset(void) +{ +} 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 */ + +void kvm_s390_floating_interrupt(struct kvm_s390_irq *irq); +void kvm_s390_service_interrupt(uint32_t parm); +void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq); +void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code); +int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, + int len, bool is_write); +void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code); +void kvm_s390_io_interrupt(uint16_t subchannel_id, + uint16_t subchannel_nr, uint32_t io_int_parm, + uint32_t io_int_word); +void kvm_s390_crw_mchk(void); +int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state); +void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu); +int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu); +int kvm_s390_get_ri(void); +int kvm_s390_get_gs(void); +int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock); +int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock); +void kvm_s390_enable_css_support(S390CPU *cpu); +int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, + int vq, bool assign); +int kvm_s390_cpu_restart(S390CPU *cpu); +int kvm_s390_get_memslot_count(KVMState *s); +int kvm_s390_cmma_active(void); +void kvm_s390_cmma_reset(void); +void kvm_s390_reset_vcpu(S390CPU *cpu); +int kvm_s390_set_mem_limit(KVMState *s, uint64_t new_limit, uint64_t *hw_limit); +void kvm_s390_crypto_reset(void); + +/* implemented outside of target/s390x/ */ +int kvm_s390_inject_flic(struct kvm_s390_irq *irq); + +#endif /* KVM_S390X_H */
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. Change license to GPL2+ and keep copyright notice. Suggested-by: Thomas Huth <thuth@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/Makefile.objs | 1 + target/s390x/cpu.h | 121 +-------------------------------------------- target/s390x/kvm-stub.c | 111 +++++++++++++++++++++++++++++++++++++++++ target/s390x/kvm_s390x.h | 51 +++++++++++++++++++ 4 files changed, 164 insertions(+), 120 deletions(-) create mode 100644 target/s390x/kvm-stub.c create mode 100644 target/s390x/kvm_s390x.h