diff mbox series

[1/1] s390x/s390-virtio-ccw: Fix build on systems without KVM

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

Commit Message

Christian Borntraeger April 6, 2020, 7:59 a.m. UTC
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(-)

Comments

Cornelia Huck April 6, 2020, 9:04 a.m. UTC | #1
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;
David Hildenbrand April 6, 2020, 9:07 a.m. UTC | #2
>  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.
Christian Borntraeger April 6, 2020, 9:27 a.m. UTC | #3
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;
>
Christian Borntraeger April 6, 2020, 9:29 a.m. UTC | #4
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?
David Hildenbrand April 6, 2020, 9:32 a.m. UTC | #5
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 :)
Cornelia Huck April 6, 2020, 9:40 a.m. UTC | #6
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.
Cornelia Huck April 6, 2020, 9:41 a.m. UTC | #7
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 {  
> >
Christian Borntraeger April 6, 2020, 9:51 a.m. UTC | #8
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.
Cornelia Huck April 6, 2020, 10 a.m. UTC | #9
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 mbox series

Patch

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 */