| Message ID | 20170907201335.13956-9-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x cleanups and CPU hotplug via device_add | expand |
On 07.09.2017 22:13, David Hildenbrand wrote: > Implemented in sclp.c, so let's move it to the right include file. > Fix up one include. Do a forward declaration of CPUS390XState to fix the > two sclp consoles complaining. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/hw/s390x/sclp.h | 2 ++ > target/s390x/cpu.h | 1 - > target/s390x/misc_helper.c | 1 + > 3 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index a72d096081..4b86a8a293 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > +typedef struct CPUS390XState CPUS390XState; > +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); That's dangerous and likely does not work with certain versions of GCC. You can't do a "forward declaration" with typedef in C, I'm afraid. See for example: https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html https://stackoverflow.com/questions/8367646/redefinition-of-typedef All this typedef'ing in QEMU is pretty bad ... we run into this problem again and again. include/qemu/typedefs.h is just a work-around for this. I know people like typedefs for some reasons (I used to do that, too, before I realized the trouble with them), but IMHO we should rather adopt the typedef-related rules from the kernel coding conventions instead: https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs Thomas
Thomas Huth <thuth@redhat.com> writes: > On 07.09.2017 22:13, David Hildenbrand wrote: >> Implemented in sclp.c, so let's move it to the right include file. >> Fix up one include. Do a forward declaration of CPUS390XState to fix the >> two sclp consoles complaining. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> include/hw/s390x/sclp.h | 2 ++ >> target/s390x/cpu.h | 1 - >> target/s390x/misc_helper.c | 1 + >> 3 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index a72d096081..4b86a8a293 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> +typedef struct CPUS390XState CPUS390XState; >> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > That's dangerous and likely does not work with certain versions of GCC. > You can't do a "forward declaration" with typedef in C, I'm afraid. See > for example: > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html > https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html > https://stackoverflow.com/questions/8367646/redefinition-of-typedef > > All this typedef'ing in QEMU is pretty bad ... we run into this problem > again and again. include/qemu/typedefs.h is just a work-around for this. > I know people like typedefs for some reasons (I used to do that, too, > before I realized the trouble with them), but IMHO we should rather > adopt the typedef-related rules from the kernel coding conventions instead: > > https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs I prefer the kernel style for typedefs myself. But it's strictly a matter of style. Excessive typedeffing makes code harder to understand, it isn't wrong. The part that's wrong is defining things more than once, and that applies to everything, not just typedefs. Sometimes you get away with defining something more than once. It's still wrong. You're not supposed to define the same variable more than once, either (although many C compilers let you get away with it, see -fno-common). You define it in *one* place. If you need to declare it, declare it in *one* place, and make sure that place is in scope at the definition, so the compiler can check the two match. Likewise, you're not supposed to define the same struct tag more than once, and you should declare it in just one place. Likewise, you're not supposed to define (with typedef) the same type more than once. There is no such thing as a typedef declaration. qemu/typedefs.h is not a work-around for a typedef-happy style. Its purpose is breaking inclusion cycles. Secondary purpose is reducing the need for non-cyclic includes. If we didn't typedef, we'd still put our struct declarations there.
On 08.09.2017 06:21, Thomas Huth wrote: > On 07.09.2017 22:13, David Hildenbrand wrote: >> Implemented in sclp.c, so let's move it to the right include file. >> Fix up one include. Do a forward declaration of CPUS390XState to fix the >> two sclp consoles complaining. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> include/hw/s390x/sclp.h | 2 ++ >> target/s390x/cpu.h | 1 - >> target/s390x/misc_helper.c | 1 + >> 3 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index a72d096081..4b86a8a293 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> +typedef struct CPUS390XState CPUS390XState; >> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > That's dangerous and likely does not work with certain versions of GCC. > You can't do a "forward declaration" with typedef in C, I'm afraid. See > for example: > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html > https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html > https://stackoverflow.com/questions/8367646/redefinition-of-typedef > > All this typedef'ing in QEMU is pretty bad ... we run into this problem > again and again. include/qemu/typedefs.h is just a work-around for this. > I know people like typedefs for some reasons (I used to do that, too, > before I realized the trouble with them), but IMHO we should rather > adopt the typedef-related rules from the kernel coding conventions instead: > > https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs > > Thomas > Yes, this is really nasty. And I wasn't aware of the involved issues. This seems to be the only feasible solution (including cpu.h sounds wrong and will require a bunch of other includes): diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index a72d096081..ce80915a02 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); void sclp_service_interrupt(uint32_t sccb); void raise_irq_cpu_hotplug(void); +struct CPUS390XState; +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 3aa2e46aac..032d1de2e8 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, /* outside of target/s390x/ */ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); -int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index b142db71c6..8b07535b02 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -35,6 +35,7 @@ #include "sysemu/sysemu.h" #include "hw/s390x/ebcdic.h" #include "hw/s390x/s390-virtio-hcall.h" +#include "hw/s390x/sclp.h" #endif /* #define DEBUG_HELPER */ Opinions?
On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: > On 08.09.2017 06:21, Thomas Huth wrote: > > On 07.09.2017 22:13, David Hildenbrand wrote: > >> Implemented in sclp.c, so let's move it to the right include file. > >> Fix up one include. Do a forward declaration of CPUS390XState to fix the > >> two sclp consoles complaining. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> include/hw/s390x/sclp.h | 2 ++ > >> target/s390x/cpu.h | 1 - > >> target/s390x/misc_helper.c | 1 + > >> 3 files changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > >> index a72d096081..4b86a8a293 100644 > >> --- a/include/hw/s390x/sclp.h > >> +++ b/include/hw/s390x/sclp.h > >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > >> void sclp_service_interrupt(uint32_t sccb); > >> void raise_irq_cpu_hotplug(void); > >> +typedef struct CPUS390XState CPUS390XState; > >> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > > > That's dangerous and likely does not work with certain versions of GCC. > > You can't do a "forward declaration" with typedef in C, I'm afraid. See > > for example: > > > > https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html > > https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html > > https://stackoverflow.com/questions/8367646/redefinition-of-typedef > > > > All this typedef'ing in QEMU is pretty bad ... we run into this problem > > again and again. include/qemu/typedefs.h is just a work-around for this. > > I know people like typedefs for some reasons (I used to do that, too, > > before I realized the trouble with them), but IMHO we should rather > > adopt the typedef-related rules from the kernel coding conventions instead: > > > > https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs > > > > Thomas > > > > Yes, this is really nasty. And I wasn't aware of the involved issues. > > This seems to be the only feasible solution (including cpu.h sounds > wrong and will require a bunch of other includes): > > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index a72d096081..ce80915a02 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -242,5 +242,7 @@ sclpMemoryHotplugDev > *init_sclp_memory_hotplug_dev(void); > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > +struct CPUS390XState; > +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, > uint32_t code); > > #endif Why not use typedefs.h? Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index 4b86a8a293..3512bf8283 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -242,7 +242,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); void sclp_service_interrupt(uint32_t sccb); void raise_irq_cpu_hotplug(void); -typedef struct CPUS390XState CPUS390XState; int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index 39bc8351a3..9c97bffa92 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -21,6 +21,7 @@ typedef struct Chardev Chardev; typedef struct CompatProperty CompatProperty; typedef struct CPUAddressSpace CPUAddressSpace; typedef struct CPUState CPUState; +typedef struct CPUS390XState CPUS390XState; typedef struct DeviceListener DeviceListener; typedef struct DeviceState DeviceState; typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot; diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 032d1de2e8..f99a82cd5e 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -80,7 +80,7 @@ typedef struct MchkQueue { uint16_t type; } MchkQueue; -typedef struct CPUS390XState { +struct CPUS390XState { uint64_t regs[16]; /* GP registers */ /* * The floating point registers are part of the vector registers. @@ -174,7 +174,7 @@ typedef struct CPUS390XState { /* currently processed sigp order */ uint8_t sigp_order; -} CPUS390XState; +}; static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr) { > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 3aa2e46aac..032d1de2e8 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, > uint8_t ar, void *hostbuf, > > /* outside of target/s390x/ */ > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); > -int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > #endif > diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > index b142db71c6..8b07535b02 100644 > --- a/target/s390x/misc_helper.c > +++ b/target/s390x/misc_helper.c > @@ -35,6 +35,7 @@ > #include "sysemu/sysemu.h" > #include "hw/s390x/ebcdic.h" > #include "hw/s390x/s390-virtio-hcall.h" > +#include "hw/s390x/sclp.h" > #endif > > /* #define DEBUG_HELPER */ > > > Opinions? > > > > -- > > Thanks, > > David
On 08.09.2017 14:29, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 07.09.2017 22:13, David Hildenbrand wrote: >>> Implemented in sclp.c, so let's move it to the right include file. >>> Fix up one include. Do a forward declaration of CPUS390XState to fix the >>> two sclp consoles complaining. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> include/hw/s390x/sclp.h | 2 ++ >>> target/s390x/cpu.h | 1 - >>> target/s390x/misc_helper.c | 1 + >>> 3 files changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>> index a72d096081..4b86a8a293 100644 >>> --- a/include/hw/s390x/sclp.h >>> +++ b/include/hw/s390x/sclp.h >>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>> void sclp_service_interrupt(uint32_t sccb); >>> void raise_irq_cpu_hotplug(void); >>> +typedef struct CPUS390XState CPUS390XState; >>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >> >> That's dangerous and likely does not work with certain versions of GCC. >> You can't do a "forward declaration" with typedef in C, I'm afraid. See >> for example: >> >> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html >> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html >> https://stackoverflow.com/questions/8367646/redefinition-of-typedef >> >> All this typedef'ing in QEMU is pretty bad ... we run into this problem >> again and again. include/qemu/typedefs.h is just a work-around for this. >> I know people like typedefs for some reasons (I used to do that, too, >> before I realized the trouble with them), but IMHO we should rather >> adopt the typedef-related rules from the kernel coding conventions instead: >> >> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs > > I prefer the kernel style for typedefs myself. But it's strictly a > matter of style. Excessive typedeffing makes code harder to understand, > it isn't wrong. The part that's wrong is defining things more than > once, and that applies to everything, not just typedefs. > > Sometimes you get away with defining something more than once. It's > still wrong. > > You're not supposed to define the same variable more than once, either > (although many C compilers let you get away with it, see -fno-common). > You define it in *one* place. If you need to declare it, declare it in > *one* place, and make sure that place is in scope at the definition, so > the compiler can check the two match. > > Likewise, you're not supposed to define the same struct tag more than > once, and you should declare it in just one place. AFAIK it's perfect valid C to do a forward declaration of a struct multiple times by just writing e.g. "struct CPUS390XState;" somewhere in your code. This is also what is done in various Linux kernel headers all over the place. > Likewise, you're not supposed to define (with typedef) the same type > more than once. There is no such thing as a typedef declaration. > > qemu/typedefs.h is not a work-around for a typedef-happy style. Its > purpose is breaking inclusion cycles. Secondary purpose is reducing the > need for non-cyclic includes. If we didn't typedef, we'd still put our > struct declarations there. No, since it's not required for struct forward declarations, only to avoid multiple typedef definitions. Thomas
On 10.09.2017 00:07, Eduardo Habkost wrote: > On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: >> On 08.09.2017 06:21, Thomas Huth wrote: >>> On 07.09.2017 22:13, David Hildenbrand wrote: >>>> Implemented in sclp.c, so let's move it to the right include file. >>>> Fix up one include. Do a forward declaration of CPUS390XState to fix the >>>> two sclp consoles complaining. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> include/hw/s390x/sclp.h | 2 ++ >>>> target/s390x/cpu.h | 1 - >>>> target/s390x/misc_helper.c | 1 + >>>> 3 files changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>>> index a72d096081..4b86a8a293 100644 >>>> --- a/include/hw/s390x/sclp.h >>>> +++ b/include/hw/s390x/sclp.h >>>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >>>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>>> void sclp_service_interrupt(uint32_t sccb); >>>> void raise_irq_cpu_hotplug(void); >>>> +typedef struct CPUS390XState CPUS390XState; >>>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >>> >>> That's dangerous and likely does not work with certain versions of GCC. >>> You can't do a "forward declaration" with typedef in C, I'm afraid. See >>> for example: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html >>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html >>> https://stackoverflow.com/questions/8367646/redefinition-of-typedef >>> >>> All this typedef'ing in QEMU is pretty bad ... we run into this problem >>> again and again. include/qemu/typedefs.h is just a work-around for this. >>> I know people like typedefs for some reasons (I used to do that, too, >>> before I realized the trouble with them), but IMHO we should rather >>> adopt the typedef-related rules from the kernel coding conventions instead: >>> >>> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs >>> >>> Thomas >>> >> >> Yes, this is really nasty. And I wasn't aware of the involved issues. >> >> This seems to be the only feasible solution (including cpu.h sounds >> wrong and will require a bunch of other includes): >> >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index a72d096081..ce80915a02 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev >> *init_sclp_memory_hotplug_dev(void); >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> +struct CPUS390XState; >> +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, >> uint32_t code); >> >> #endif > > Why not use typedefs.h? > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index 4b86a8a293..3512bf8283 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -242,7 +242,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > -typedef struct CPUS390XState CPUS390XState; > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > #endif > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 39bc8351a3..9c97bffa92 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h Using include/qemu/typedefs.h here is IMHO really ugly. Do we really want to pollute a common include file with target specific code? My preferences are first to avoid typdefs, but if we really need/want them (do we? There is no comment about this in our coding styles), I think we should rather introduce target-specific typedefs.h headers, too, for everything that is not part of the common code. Thomas
On 10/09/2017 00:07, Eduardo Habkost wrote: > On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: >> On 08.09.2017 06:21, Thomas Huth wrote: >>> On 07.09.2017 22:13, David Hildenbrand wrote: >>>> Implemented in sclp.c, so let's move it to the right include file. >>>> Fix up one include. Do a forward declaration of CPUS390XState to fix the >>>> two sclp consoles complaining. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> include/hw/s390x/sclp.h | 2 ++ >>>> target/s390x/cpu.h | 1 - >>>> target/s390x/misc_helper.c | 1 + >>>> 3 files changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>>> index a72d096081..4b86a8a293 100644 >>>> --- a/include/hw/s390x/sclp.h >>>> +++ b/include/hw/s390x/sclp.h >>>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >>>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>>> void sclp_service_interrupt(uint32_t sccb); >>>> void raise_irq_cpu_hotplug(void); >>>> +typedef struct CPUS390XState CPUS390XState; >>>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >>> >>> That's dangerous and likely does not work with certain versions of GCC. >>> You can't do a "forward declaration" with typedef in C, I'm afraid. See >>> for example: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html >>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html >>> https://stackoverflow.com/questions/8367646/redefinition-of-typedef >>> >>> All this typedef'ing in QEMU is pretty bad ... we run into this problem >>> again and again. include/qemu/typedefs.h is just a work-around for this. >>> I know people like typedefs for some reasons (I used to do that, too, >>> before I realized the trouble with them), but IMHO we should rather >>> adopt the typedef-related rules from the kernel coding conventions instead: >>> >>> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs >>> >>> Thomas >>> >> >> Yes, this is really nasty. And I wasn't aware of the involved issues. >> >> This seems to be the only feasible solution (including cpu.h sounds >> wrong and will require a bunch of other includes): >> >> >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >> index a72d096081..ce80915a02 100644 >> --- a/include/hw/s390x/sclp.h >> +++ b/include/hw/s390x/sclp.h >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev >> *init_sclp_memory_hotplug_dev(void); >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >> void sclp_service_interrupt(uint32_t sccb); >> void raise_irq_cpu_hotplug(void); >> +struct CPUS390XState; >> +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, >> uint32_t code); >> >> #endif > > Why not use typedefs.h? See Markus's reply. But, maybe it's even better to use S390CPU* and include target/s390x/cpu-qom.h, which by design provides as little definitions as needed. > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > index 4b86a8a293..3512bf8283 100644 > --- a/include/hw/s390x/sclp.h > +++ b/include/hw/s390x/sclp.h > @@ -242,7 +242,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > void sclp_service_interrupt(uint32_t sccb); > void raise_irq_cpu_hotplug(void); > -typedef struct CPUS390XState CPUS390XState; > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > #endif > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 39bc8351a3..9c97bffa92 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -21,6 +21,7 @@ typedef struct Chardev Chardev; > typedef struct CompatProperty CompatProperty; > typedef struct CPUAddressSpace CPUAddressSpace; > typedef struct CPUState CPUState; > +typedef struct CPUS390XState CPUS390XState; > typedef struct DeviceListener DeviceListener; > typedef struct DeviceState DeviceState; > typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot; > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 032d1de2e8..f99a82cd5e 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -80,7 +80,7 @@ typedef struct MchkQueue { > uint16_t type; > } MchkQueue; > > -typedef struct CPUS390XState { > +struct CPUS390XState { > uint64_t regs[16]; /* GP registers */ > /* > * The floating point registers are part of the vector registers. > @@ -174,7 +174,7 @@ typedef struct CPUS390XState { > /* currently processed sigp order */ > uint8_t sigp_order; > > -} CPUS390XState; > +}; > > static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr) > { > > > >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index 3aa2e46aac..032d1de2e8 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, >> uint8_t ar, void *hostbuf, >> >> /* outside of target/s390x/ */ >> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); >> -int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >> >> #endif >> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c >> index b142db71c6..8b07535b02 100644 >> --- a/target/s390x/misc_helper.c >> +++ b/target/s390x/misc_helper.c >> @@ -35,6 +35,7 @@ >> #include "sysemu/sysemu.h" >> #include "hw/s390x/ebcdic.h" >> #include "hw/s390x/s390-virtio-hcall.h" >> +#include "hw/s390x/sclp.h" >> #endif >> >> /* #define DEBUG_HELPER */ >> >> >> Opinions? >> >> >> >> -- >> >> Thanks, >> >> David >
On 11.09.2017 12:23, Paolo Bonzini wrote: > On 10/09/2017 00:07, Eduardo Habkost wrote: >> On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: >>> On 08.09.2017 06:21, Thomas Huth wrote: >>>> On 07.09.2017 22:13, David Hildenbrand wrote: >>>>> Implemented in sclp.c, so let's move it to the right include file. >>>>> Fix up one include. Do a forward declaration of CPUS390XState to fix the >>>>> two sclp consoles complaining. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> include/hw/s390x/sclp.h | 2 ++ >>>>> target/s390x/cpu.h | 1 - >>>>> target/s390x/misc_helper.c | 1 + >>>>> 3 files changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>>>> index a72d096081..4b86a8a293 100644 >>>>> --- a/include/hw/s390x/sclp.h >>>>> +++ b/include/hw/s390x/sclp.h >>>>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); >>>>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>>>> void sclp_service_interrupt(uint32_t sccb); >>>>> void raise_irq_cpu_hotplug(void); >>>>> +typedef struct CPUS390XState CPUS390XState; >>>>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); >>>> >>>> That's dangerous and likely does not work with certain versions of GCC. >>>> You can't do a "forward declaration" with typedef in C, I'm afraid. See >>>> for example: >>>> >>>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html >>>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html >>>> https://stackoverflow.com/questions/8367646/redefinition-of-typedef >>>> >>>> All this typedef'ing in QEMU is pretty bad ... we run into this problem >>>> again and again. include/qemu/typedefs.h is just a work-around for this. >>>> I know people like typedefs for some reasons (I used to do that, too, >>>> before I realized the trouble with them), but IMHO we should rather >>>> adopt the typedef-related rules from the kernel coding conventions instead: >>>> >>>> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs >>>> >>>> Thomas >>>> >>> >>> Yes, this is really nasty. And I wasn't aware of the involved issues. >>> >>> This seems to be the only feasible solution (including cpu.h sounds >>> wrong and will require a bunch of other includes): >>> >>> >>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h >>> index a72d096081..ce80915a02 100644 >>> --- a/include/hw/s390x/sclp.h >>> +++ b/include/hw/s390x/sclp.h >>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev >>> *init_sclp_memory_hotplug_dev(void); >>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); >>> void sclp_service_interrupt(uint32_t sccb); >>> void raise_irq_cpu_hotplug(void); >>> +struct CPUS390XState; >>> +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, >>> uint32_t code); >>> >>> #endif >> >> Why not use typedefs.h? > > See Markus's reply. But, maybe it's even better to use S390CPU* and > include target/s390x/cpu-qom.h, which by design provides as little > definitions as needed. I'll go with that approach. I'll drop the dependency from cpu-qom.h to cpu_models.h (by moving typedefs to cpu-qom.h). This makes it compile at hopefully should then be good enough for now. (this approach implies dropping patch "[PATCH v3 05/21] target/s390x: move typedef of S390CPU to its definition"). Thanks! > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
On Mon, Sep 11, 2017 at 12:23:10PM +0200, Paolo Bonzini wrote: > On 10/09/2017 00:07, Eduardo Habkost wrote: > > On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: > >> On 08.09.2017 06:21, Thomas Huth wrote: > >>> On 07.09.2017 22:13, David Hildenbrand wrote: > >>>> Implemented in sclp.c, so let's move it to the right include file. > >>>> Fix up one include. Do a forward declaration of CPUS390XState to fix the > >>>> two sclp consoles complaining. > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> include/hw/s390x/sclp.h | 2 ++ > >>>> target/s390x/cpu.h | 1 - > >>>> target/s390x/misc_helper.c | 1 + > >>>> 3 files changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > >>>> index a72d096081..4b86a8a293 100644 > >>>> --- a/include/hw/s390x/sclp.h > >>>> +++ b/include/hw/s390x/sclp.h > >>>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > >>>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > >>>> void sclp_service_interrupt(uint32_t sccb); > >>>> void raise_irq_cpu_hotplug(void); > >>>> +typedef struct CPUS390XState CPUS390XState; > >>>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > >>> > >>> That's dangerous and likely does not work with certain versions of GCC. > >>> You can't do a "forward declaration" with typedef in C, I'm afraid. See > >>> for example: > >>> > >>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html > >>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html > >>> https://stackoverflow.com/questions/8367646/redefinition-of-typedef > >>> > >>> All this typedef'ing in QEMU is pretty bad ... we run into this problem > >>> again and again. include/qemu/typedefs.h is just a work-around for this. > >>> I know people like typedefs for some reasons (I used to do that, too, > >>> before I realized the trouble with them), but IMHO we should rather > >>> adopt the typedef-related rules from the kernel coding conventions instead: > >>> > >>> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs > >>> > >>> Thomas > >>> > >> > >> Yes, this is really nasty. And I wasn't aware of the involved issues. > >> > >> This seems to be the only feasible solution (including cpu.h sounds > >> wrong and will require a bunch of other includes): > >> > >> > >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > >> index a72d096081..ce80915a02 100644 > >> --- a/include/hw/s390x/sclp.h > >> +++ b/include/hw/s390x/sclp.h > >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev > >> *init_sclp_memory_hotplug_dev(void); > >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > >> void sclp_service_interrupt(uint32_t sccb); > >> void raise_irq_cpu_hotplug(void); > >> +struct CPUS390XState; > >> +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, > >> uint32_t code); > >> > >> #endif > > > > Why not use typedefs.h? > > See Markus's reply. But, maybe it's even better to use S390CPU* and > include target/s390x/cpu-qom.h, which by design provides as little > definitions as needed. I don't see an argument against moving typedef CPUS390XState to typedefs.h in Markus' reply. I see one argument for it (reducing the need for non-cyclic includes). cpu-qom.h includes cpu.h, so I don't know why using S390CPU* would solve any problem. I don't disagree about changing the function to use S390CPU* eventually, but it would still require us make a choice between: a) including the header where the typedef name is declared (cpu.h or cpu-qom.h); or b) moving the typedef name declaration to typedefs.h. > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > > index 4b86a8a293..3512bf8283 100644 > > --- a/include/hw/s390x/sclp.h > > +++ b/include/hw/s390x/sclp.h > > @@ -242,7 +242,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > > void sclp_service_interrupt(uint32_t sccb); > > void raise_irq_cpu_hotplug(void); > > -typedef struct CPUS390XState CPUS390XState; > > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > > > #endif > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > > index 39bc8351a3..9c97bffa92 100644 > > --- a/include/qemu/typedefs.h > > +++ b/include/qemu/typedefs.h > > @@ -21,6 +21,7 @@ typedef struct Chardev Chardev; > > typedef struct CompatProperty CompatProperty; > > typedef struct CPUAddressSpace CPUAddressSpace; > > typedef struct CPUState CPUState; > > +typedef struct CPUS390XState CPUS390XState; > > typedef struct DeviceListener DeviceListener; > > typedef struct DeviceState DeviceState; > > typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot; > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > > index 032d1de2e8..f99a82cd5e 100644 > > --- a/target/s390x/cpu.h > > +++ b/target/s390x/cpu.h > > @@ -80,7 +80,7 @@ typedef struct MchkQueue { > > uint16_t type; > > } MchkQueue; > > > > -typedef struct CPUS390XState { > > +struct CPUS390XState { > > uint64_t regs[16]; /* GP registers */ > > /* > > * The floating point registers are part of the vector registers. > > @@ -174,7 +174,7 @@ typedef struct CPUS390XState { > > /* currently processed sigp order */ > > uint8_t sigp_order; > > > > -} CPUS390XState; > > +}; > > > > static inline CPU_DoubleU *get_freg(CPUS390XState *cs, int nr) > > { > > > > > > > >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > >> index 3aa2e46aac..032d1de2e8 100644 > >> --- a/target/s390x/cpu.h > >> +++ b/target/s390x/cpu.h > >> @@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, > >> uint8_t ar, void *hostbuf, > >> > >> /* outside of target/s390x/ */ > >> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); > >> -int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > >> > >> #endif > >> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c > >> index b142db71c6..8b07535b02 100644 > >> --- a/target/s390x/misc_helper.c > >> +++ b/target/s390x/misc_helper.c > >> @@ -35,6 +35,7 @@ > >> #include "sysemu/sysemu.h" > >> #include "hw/s390x/ebcdic.h" > >> #include "hw/s390x/s390-virtio-hcall.h" > >> +#include "hw/s390x/sclp.h" > >> #endif > >> > >> /* #define DEBUG_HELPER */ > >> > >> > >> Opinions? > >> > >> > >> > >> -- > >> > >> Thanks, > >> > >> David > > >
>>>> >>>> #endif >>> >>> Why not use typedefs.h? >> >> See Markus's reply. But, maybe it's even better to use S390CPU* and >> include target/s390x/cpu-qom.h, which by design provides as little >> definitions as needed. > > I don't see an argument against moving typedef CPUS390XState to > typedefs.h in Markus' reply. I see one argument for it (reducing > the need for non-cyclic includes). > > cpu-qom.h includes cpu.h, so I don't know why using S390CPU* > would solve any problem. I don't disagree about changing the > function to use S390CPU* eventually, but it would still require > us make a choice between: a) including the header where the > typedef name is declared (cpu.h or cpu-qom.h); or b) moving the > typedef name declaration to typedefs.h. It includes qom/cpu.h, not cpu.h. That's why using cpu-qom.h for such typedefs works (see v4). Thanks!
On Mon, Sep 11, 2017 at 07:56:19PM +0200, David Hildenbrand wrote: > > >>>> > >>>> #endif > >>> > >>> Why not use typedefs.h? > >> > >> See Markus's reply. But, maybe it's even better to use S390CPU* and > >> include target/s390x/cpu-qom.h, which by design provides as little > >> definitions as needed. > > > > I don't see an argument against moving typedef CPUS390XState to > > typedefs.h in Markus' reply. I see one argument for it (reducing > > the need for non-cyclic includes). > > > > cpu-qom.h includes cpu.h, so I don't know why using S390CPU* > > would solve any problem. I don't disagree about changing the > > function to use S390CPU* eventually, but it would still require > > us make a choice between: a) including the header where the > > typedef name is declared (cpu.h or cpu-qom.h); or b) moving the > > typedef name declaration to typedefs.h. > > It includes qom/cpu.h, not cpu.h. That's why using cpu-qom.h for such > typedefs works (see v4). > Oops, I was looking at an older tree (before commit 347b1a5c). You're right, sorry for the noise.
On Mon, Sep 11, 2017 at 04:23:09AM +0200, Thomas Huth wrote: > On 10.09.2017 00:07, Eduardo Habkost wrote: > > On Fri, Sep 08, 2017 at 02:46:36PM +0200, David Hildenbrand wrote: > >> On 08.09.2017 06:21, Thomas Huth wrote: > >>> On 07.09.2017 22:13, David Hildenbrand wrote: > >>>> Implemented in sclp.c, so let's move it to the right include file. > >>>> Fix up one include. Do a forward declaration of CPUS390XState to fix the > >>>> two sclp consoles complaining. > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> include/hw/s390x/sclp.h | 2 ++ > >>>> target/s390x/cpu.h | 1 - > >>>> target/s390x/misc_helper.c | 1 + > >>>> 3 files changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > >>>> index a72d096081..4b86a8a293 100644 > >>>> --- a/include/hw/s390x/sclp.h > >>>> +++ b/include/hw/s390x/sclp.h > >>>> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > >>>> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > >>>> void sclp_service_interrupt(uint32_t sccb); > >>>> void raise_irq_cpu_hotplug(void); > >>>> +typedef struct CPUS390XState CPUS390XState; > >>>> +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > >>> > >>> That's dangerous and likely does not work with certain versions of GCC. > >>> You can't do a "forward declaration" with typedef in C, I'm afraid. See > >>> for example: > >>> > >>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01454.html > >>> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03337.html > >>> https://stackoverflow.com/questions/8367646/redefinition-of-typedef > >>> > >>> All this typedef'ing in QEMU is pretty bad ... we run into this problem > >>> again and again. include/qemu/typedefs.h is just a work-around for this. > >>> I know people like typedefs for some reasons (I used to do that, too, > >>> before I realized the trouble with them), but IMHO we should rather > >>> adopt the typedef-related rules from the kernel coding conventions instead: > >>> > >>> https://www.kernel.org/doc/html/v4.13/process/coding-style.html#typedefs > >>> > >>> Thomas > >>> > >> > >> Yes, this is really nasty. And I wasn't aware of the involved issues. > >> > >> This seems to be the only feasible solution (including cpu.h sounds > >> wrong and will require a bunch of other includes): > >> > >> > >> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > >> index a72d096081..ce80915a02 100644 > >> --- a/include/hw/s390x/sclp.h > >> +++ b/include/hw/s390x/sclp.h > >> @@ -242,5 +242,7 @@ sclpMemoryHotplugDev > >> *init_sclp_memory_hotplug_dev(void); > >> sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > >> void sclp_service_interrupt(uint32_t sccb); > >> void raise_irq_cpu_hotplug(void); > >> +struct CPUS390XState; > >> +int sclp_service_call(struct CPUS390XState *env, uint64_t sccb, > >> uint32_t code); > >> > >> #endif > > > > Why not use typedefs.h? > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h > > index 4b86a8a293..3512bf8283 100644 > > --- a/include/hw/s390x/sclp.h > > +++ b/include/hw/s390x/sclp.h > > @@ -242,7 +242,6 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); > > sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); > > void sclp_service_interrupt(uint32_t sccb); > > void raise_irq_cpu_hotplug(void); > > -typedef struct CPUS390XState CPUS390XState; > > int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); > > > > #endif > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > > index 39bc8351a3..9c97bffa92 100644 > > --- a/include/qemu/typedefs.h > > +++ b/include/qemu/typedefs.h > > Using include/qeemu/typedefs.h here is IMHO really ugly. Do we really > want to pollute a common include file with target specific code? My > preferences are first to avoid typdefs, but if we really need/want them > (do we? There is no comment about this in our coding styles), I think we > should rather introduce target-specific typedefs.h headers, too, for > everything that is not part of the common code. I don't see any advantage in splitting typedefs.h into arch-specific files. We don't split typedefs.h into subsystem-specific or device-specific headers, so I don't see we would need a per-architecture split either. typedefs.h is just a global collection of type identifiers that helps us reduce header dependency hell. (Anyway, the current problem is now going solved by using S390CPU* instead of CPUS390XState*, so there's no need to touch typedefs.h this time.) About keeping using typedefs: I don't have an strong opinion for/against them[1], but I would prefer to keep style consistent even if it's not explicitly documented. [1] The fact that it would make typedefs.h completely unnecessary makes me inclined towards the suggestion to stop using them.
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h index a72d096081..4b86a8a293 100644 --- a/include/hw/s390x/sclp.h +++ b/include/hw/s390x/sclp.h @@ -242,5 +242,7 @@ sclpMemoryHotplugDev *init_sclp_memory_hotplug_dev(void); sclpMemoryHotplugDev *get_sclp_memory_hotplug_dev(void); void sclp_service_interrupt(uint32_t sccb); void raise_irq_cpu_hotplug(void); +typedef struct CPUS390XState CPUS390XState; +int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 3aa2e46aac..032d1de2e8 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -721,6 +721,5 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, /* outside of target/s390x/ */ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); -int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code); #endif diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index b142db71c6..8b07535b02 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -35,6 +35,7 @@ #include "sysemu/sysemu.h" #include "hw/s390x/ebcdic.h" #include "hw/s390x/s390-virtio-hcall.h" +#include "hw/s390x/sclp.h" #endif /* #define DEBUG_HELPER */
Implemented in sclp.c, so let's move it to the right include file. Fix up one include. Do a forward declaration of CPUS390XState to fix the two sclp consoles complaining. Signed-off-by: David Hildenbrand <david@redhat.com> --- include/hw/s390x/sclp.h | 2 ++ target/s390x/cpu.h | 1 - target/s390x/misc_helper.c | 1 + 3 files changed, 3 insertions(+), 1 deletion(-)