| Message ID | 20200406075931.26232-2-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/next: build fix for non-KVM platforms | expand |
On Mon, 6 Apr 2020 03:59:31 -0400 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > linux/kvm.h is not available on all platforms. Let us move > s390_machine_inject_pv_error into pv.c as it uses KVM structures. > > Fixes: 49fc3220175e ("s390x: protvirt: Support unpack facility") > Reported-by: Bruce Rogers <brogers@suse.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > hw/s390x/ipl.h | 1 + > hw/s390x/pv.c | 11 +++++++++++ > hw/s390x/s390-virtio-ccw.c | 10 ---------- > include/hw/s390x/pv.h | 3 +++ > 4 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index 89b3044d7a..53cc9eb5ac 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -14,6 +14,7 @@ > #define HW_S390_IPL_H > > #include "cpu.h" > +#include "exec/address-spaces.h" Hm, what is now requiring including this? (No objection, but I don't see it.) > #include "hw/qdev-core.h" > > struct IPLBlockPVComp { (...) > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index b268907395..0e8b0c63a1 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -45,7 +45,6 @@ > #include "sysemu/sysemu.h" > #include "sysemu/balloon.h" > #include "hw/s390x/pv.h" > -#include <linux/kvm.h> In hindsight, that should have been obvious :) > #include "migration/blocker.h" > > static Error *pv_mig_blocker;
> static inline bool s390_is_pv(void) > @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); > void s390_pv_perf_clear_reset(void); > int s390_pv_verify(void); > void s390_pv_unshare(void); > +void s390_machine_inject_pv_error(CPUState *cs); > #else /* CONFIG_KVM */ > static inline bool s390_is_pv(void) { return false; } > static inline int s390_pv_vm_enable(void) { return 0; } > @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { > static inline void s390_pv_perf_clear_reset(void) {} > static inline int s390_pv_verify(void) { return 0; } > static inline void s390_pv_unshare(void) {} > +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; I'd suggest renaming that to s390_pv_inject_error() or similar right away.
On 06.04.20 11:04, Cornelia Huck wrote: > On Mon, 6 Apr 2020 03:59:31 -0400 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> linux/kvm.h is not available on all platforms. Let us move >> s390_machine_inject_pv_error into pv.c as it uses KVM structures. >> >> Fixes: 49fc3220175e ("s390x: protvirt: Support unpack facility") >> Reported-by: Bruce Rogers <brogers@suse.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> hw/s390x/ipl.h | 1 + >> hw/s390x/pv.c | 11 +++++++++++ >> hw/s390x/s390-virtio-ccw.c | 10 ---------- >> include/hw/s390x/pv.h | 3 +++ >> 4 files changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >> index 89b3044d7a..53cc9eb5ac 100644 >> --- a/hw/s390x/ipl.h >> +++ b/hw/s390x/ipl.h >> @@ -14,6 +14,7 @@ >> #define HW_S390_IPL_H >> >> #include "cpu.h" >> +#include "exec/address-spaces.h" > > Hm, what is now requiring including this? (No objection, but I don't > see it.) ipl.h has static inline bool ipl_valid_pv_header(IplParameterBlock *iplb) { [..] if (!address_space_access_valid(&address_space_memory, and if included alone this fails to build without the include. > >> #include "hw/qdev-core.h" >> >> struct IPLBlockPVComp { > > (...) > >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index b268907395..0e8b0c63a1 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -45,7 +45,6 @@ >> #include "sysemu/sysemu.h" >> #include "sysemu/balloon.h" >> #include "hw/s390x/pv.h" >> -#include <linux/kvm.h> > > In hindsight, that should have been obvious :) Yes, it is. It was added pretty late (v10 I think). >> #include "migration/blocker.h" >> >> static Error *pv_mig_blocker; >
On 06.04.20 11:07, David Hildenbrand wrote: > >> static inline bool s390_is_pv(void) >> @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); >> void s390_pv_perf_clear_reset(void); >> int s390_pv_verify(void); >> void s390_pv_unshare(void); >> +void s390_machine_inject_pv_error(CPUState *cs); >> #else /* CONFIG_KVM */ >> static inline bool s390_is_pv(void) { return false; } >> static inline int s390_pv_vm_enable(void) { return 0; } >> @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { >> static inline void s390_pv_perf_clear_reset(void) {} >> static inline int s390_pv_verify(void) { return 0; } >> static inline void s390_pv_unshare(void) {} >> +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; > > I'd suggest renaming that to s390_pv_inject_error() or similar right away. Makes sense. Conny any preference?
On 06.04.20 11:29, Christian Borntraeger wrote: > > > On 06.04.20 11:07, David Hildenbrand wrote: >> >>> static inline bool s390_is_pv(void) >>> @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); >>> void s390_pv_perf_clear_reset(void); >>> int s390_pv_verify(void); >>> void s390_pv_unshare(void); >>> +void s390_machine_inject_pv_error(CPUState *cs); >>> #else /* CONFIG_KVM */ >>> static inline bool s390_is_pv(void) { return false; } >>> static inline int s390_pv_vm_enable(void) { return 0; } >>> @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { >>> static inline void s390_pv_perf_clear_reset(void) {} >>> static inline int s390_pv_verify(void) { return 0; } >>> static inline void s390_pv_unshare(void) {} >>> +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; >> >> I'd suggest renaming that to s390_pv_inject_error() or similar right away. > Me again: I guess "s390_pv_inject_reset_error()" is what it's really doing :)
On Mon, 6 Apr 2020 11:29:21 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06.04.20 11:07, David Hildenbrand wrote: > > > >> static inline bool s390_is_pv(void) > >> @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); > >> void s390_pv_perf_clear_reset(void); > >> int s390_pv_verify(void); > >> void s390_pv_unshare(void); > >> +void s390_machine_inject_pv_error(CPUState *cs); > >> #else /* CONFIG_KVM */ > >> static inline bool s390_is_pv(void) { return false; } > >> static inline int s390_pv_vm_enable(void) { return 0; } > >> @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { > >> static inline void s390_pv_perf_clear_reset(void) {} > >> static inline int s390_pv_verify(void) { return 0; } > >> static inline void s390_pv_unshare(void) {} > >> +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; > > > > I'd suggest renaming that to s390_pv_inject_error() or similar right away. > > Makes sense. > Conny any preference? > I think s390_pv_inject_error() fits in a bit better.
On Mon, 6 Apr 2020 11:27:13 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06.04.20 11:04, Cornelia Huck wrote: > > On Mon, 6 Apr 2020 03:59:31 -0400 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> linux/kvm.h is not available on all platforms. Let us move > >> s390_machine_inject_pv_error into pv.c as it uses KVM structures. > >> > >> Fixes: 49fc3220175e ("s390x: protvirt: Support unpack facility") > >> Reported-by: Bruce Rogers <brogers@suse.com> > >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> hw/s390x/ipl.h | 1 + > >> hw/s390x/pv.c | 11 +++++++++++ > >> hw/s390x/s390-virtio-ccw.c | 10 ---------- > >> include/hw/s390x/pv.h | 3 +++ > >> 4 files changed, 15 insertions(+), 10 deletions(-) > >> > >> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > >> index 89b3044d7a..53cc9eb5ac 100644 > >> --- a/hw/s390x/ipl.h > >> +++ b/hw/s390x/ipl.h > >> @@ -14,6 +14,7 @@ > >> #define HW_S390_IPL_H > >> > >> #include "cpu.h" > >> +#include "exec/address-spaces.h" > > > > Hm, what is now requiring including this? (No objection, but I don't > > see it.) > > ipl.h has > > static inline bool ipl_valid_pv_header(IplParameterBlock *iplb) > { > [..] > if (!address_space_access_valid(&address_space_memory, > > and if included alone this fails to build without the include. > Ah, makes sense. > > > > >> #include "hw/qdev-core.h" > >> > >> struct IPLBlockPVComp { > >
On 06.04.20 11:32, David Hildenbrand wrote: > On 06.04.20 11:29, Christian Borntraeger wrote: >> >> >> On 06.04.20 11:07, David Hildenbrand wrote: >>> >>>> static inline bool s390_is_pv(void) >>>> @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); >>>> void s390_pv_perf_clear_reset(void); >>>> int s390_pv_verify(void); >>>> void s390_pv_unshare(void); >>>> +void s390_machine_inject_pv_error(CPUState *cs); >>>> #else /* CONFIG_KVM */ >>>> static inline bool s390_is_pv(void) { return false; } >>>> static inline int s390_pv_vm_enable(void) { return 0; } >>>> @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { >>>> static inline void s390_pv_perf_clear_reset(void) {} >>>> static inline int s390_pv_verify(void) { return 0; } >>>> static inline void s390_pv_unshare(void) {} >>>> +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; >>> >>> I'd suggest renaming that to s390_pv_inject_error() or similar right away. >> > > Me again: I guess "s390_pv_inject_reset_error()" is what it's really > doing :) I will use that unless Conny complains.
On Mon, 6 Apr 2020 11:51:24 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06.04.20 11:32, David Hildenbrand wrote: > > On 06.04.20 11:29, Christian Borntraeger wrote: > >> > >> > >> On 06.04.20 11:07, David Hildenbrand wrote: > >>> > >>>> static inline bool s390_is_pv(void) > >>>> @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); > >>>> void s390_pv_perf_clear_reset(void); > >>>> int s390_pv_verify(void); > >>>> void s390_pv_unshare(void); > >>>> +void s390_machine_inject_pv_error(CPUState *cs); > >>>> #else /* CONFIG_KVM */ > >>>> static inline bool s390_is_pv(void) { return false; } > >>>> static inline int s390_pv_vm_enable(void) { return 0; } > >>>> @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { > >>>> static inline void s390_pv_perf_clear_reset(void) {} > >>>> static inline int s390_pv_verify(void) { return 0; } > >>>> static inline void s390_pv_unshare(void) {} > >>>> +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; > >>> > >>> I'd suggest renaming that to s390_pv_inject_error() or similar right away. > >> > > > > Me again: I guess "s390_pv_inject_reset_error()" is what it's really > > doing :) > > I will use that unless Conny complains. > Go ahead.
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index 89b3044d7a..53cc9eb5ac 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -14,6 +14,7 @@ #define HW_S390_IPL_H #include "cpu.h" +#include "exec/address-spaces.h" #include "hw/qdev-core.h" struct IPLBlockPVComp { diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c index d3333d6e13..18f785b09e 100644 --- a/hw/s390x/pv.c +++ b/hw/s390x/pv.c @@ -13,8 +13,10 @@ #include <linux/kvm.h> +#include "cpu.h" #include "qemu/error-report.h" #include "sysemu/kvm.h" +#include "hw/s390x/ipl.h" #include "hw/s390x/pv.h" static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data) @@ -100,3 +102,12 @@ void s390_pv_unshare(void) { s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL); } + +void s390_machine_inject_pv_error(CPUState *cs) +{ + int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4; + CPUS390XState *env = &S390_CPU(cs)->env; + + /* Report that we are unable to enter protected mode */ + env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV; +} diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index b268907395..0e8b0c63a1 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -45,7 +45,6 @@ #include "sysemu/sysemu.h" #include "sysemu/balloon.h" #include "hw/s390x/pv.h" -#include <linux/kvm.h> #include "migration/blocker.h" static Error *pv_mig_blocker; @@ -390,15 +389,6 @@ out_err: return rc; } -static void s390_machine_inject_pv_error(CPUState *cs) -{ - int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4; - CPUS390XState *env = &S390_CPU(cs)->env; - - /* Report that we are unable to enter protected mode */ - env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV; -} - static void s390_pv_prepare_reset(S390CcwMachineState *ms) { CPUState *cs; diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h index c6cb360f2f..b0fbed9bae 100644 --- a/include/hw/s390x/pv.h +++ b/include/hw/s390x/pv.h @@ -13,6 +13,7 @@ #define HW_S390_PV_H #ifdef CONFIG_KVM +#include "cpu.h" #include "hw/s390x/s390-virtio-ccw.h" static inline bool s390_is_pv(void) @@ -41,6 +42,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak); void s390_pv_perf_clear_reset(void); int s390_pv_verify(void); void s390_pv_unshare(void); +void s390_machine_inject_pv_error(CPUState *cs); #else /* CONFIG_KVM */ static inline bool s390_is_pv(void) { return false; } static inline int s390_pv_vm_enable(void) { return 0; } @@ -50,6 +52,7 @@ static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { static inline void s390_pv_perf_clear_reset(void) {} static inline int s390_pv_verify(void) { return 0; } static inline void s390_pv_unshare(void) {} +static inline void s390_machine_inject_pv_error(CPUState *cs) {}; #endif /* CONFIG_KVM */ #endif /* HW_S390_PV_H */
linux/kvm.h is not available on all platforms. Let us move s390_machine_inject_pv_error into pv.c as it uses KVM structures. Fixes: 49fc3220175e ("s390x: protvirt: Support unpack facility") Reported-by: Bruce Rogers <brogers@suse.com> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- hw/s390x/ipl.h | 1 + hw/s390x/pv.c | 11 +++++++++++ hw/s390x/s390-virtio-ccw.c | 10 ---------- include/hw/s390x/pv.h | 3 +++ 4 files changed, 15 insertions(+), 10 deletions(-)