| Message ID | 20201021134345.110173-3-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: pv: Diag318 fixes | expand |
On 21.10.20 15:43, Janosch Frank wrote: > Diag318 fencing needs to be determined on the current VM PV state and > not on the state that the VM has when we create the CPU model. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Fixes: fabdada935 ("s390: guest support for diagnose 0x318") > Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> > --- > hw/s390x/sclp.c | 10 ++++++++++ > target/s390x/kvm.c | 3 +-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 0cf2290826..69aba402d3 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -22,6 +22,7 @@ > #include "hw/s390x/event-facility.h" > #include "hw/s390x/s390-pci-bus.h" > #include "hw/s390x/ipl.h" > +#include "hw/s390x/pv.h" > > static inline SCLPDevice *get_sclp_device(void) > { > @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) > if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) { > s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134, > &read_info->fac134); > + /* > + * Diag318 is not available in protected mode and will result > + * in an operation exception. As we can dynamically move in > + * and out of protected mode, we need to fence the feature > + * here rather than when creating the CPU model. > + */ > + if (s390_is_pv()) { > + read_info->fac134 &= ~0x80; > + } Hmm, I thought firmware would handle exposing cpu features and similar, so we can't temper with it .... Can we move that into s390_get_feat_block instead and check against the feature bit instead?
On 21.10.20 16:14, David Hildenbrand wrote: > On 21.10.20 15:43, Janosch Frank wrote: >> Diag318 fencing needs to be determined on the current VM PV state and >> not on the state that the VM has when we create the CPU model. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Fixes: fabdada935 ("s390: guest support for diagnose 0x318") >> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 10 ++++++++++ >> target/s390x/kvm.c | 3 +-- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 0cf2290826..69aba402d3 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -22,6 +22,7 @@ >> #include "hw/s390x/event-facility.h" >> #include "hw/s390x/s390-pci-bus.h" >> #include "hw/s390x/ipl.h" >> +#include "hw/s390x/pv.h" >> >> static inline SCLPDevice *get_sclp_device(void) >> { >> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) { >> s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134, >> &read_info->fac134); >> + /* >> + * Diag318 is not available in protected mode and will result >> + * in an operation exception. As we can dynamically move in >> + * and out of protected mode, we need to fence the feature >> + * here rather than when creating the CPU model. >> + */ >> + if (s390_is_pv()) { >> + read_info->fac134 &= ~0x80; >> + } > > Hmm, I thought firmware would handle exposing cpu features and similar, > so we can't temper with it .... Only the stfle bits. > > Can we move that into s390_get_feat_block instead and check against the > feature bit instead? No because we want to have this active for !pv and disabled for pv but we switch this multiple times when doing boot/reboot.
On 10/21/20 4:14 PM, David Hildenbrand wrote: > On 21.10.20 15:43, Janosch Frank wrote: >> Diag318 fencing needs to be determined on the current VM PV state and >> not on the state that the VM has when we create the CPU model. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Fixes: fabdada935 ("s390: guest support for diagnose 0x318") >> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> >> --- >> hw/s390x/sclp.c | 10 ++++++++++ >> target/s390x/kvm.c | 3 +-- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 0cf2290826..69aba402d3 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -22,6 +22,7 @@ >> #include "hw/s390x/event-facility.h" >> #include "hw/s390x/s390-pci-bus.h" >> #include "hw/s390x/ipl.h" >> +#include "hw/s390x/pv.h" >> >> static inline SCLPDevice *get_sclp_device(void) >> { >> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >> if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) { >> s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134, >> &read_info->fac134); >> + /* >> + * Diag318 is not available in protected mode and will result >> + * in an operation exception. As we can dynamically move in >> + * and out of protected mode, we need to fence the feature >> + * here rather than when creating the CPU model. >> + */ >> + if (s390_is_pv()) { >> + read_info->fac134 &= ~0x80; >> + } > > Hmm, I thought firmware would handle exposing cpu features and similar, > so we can't temper with it .... STFLE data is indeed provided by the Ultravisor, but SCLP is still done by QEMU as that data is often not machine specific as visible in this case > > Can we move that into s390_get_feat_block instead and check against the > feature bit instead? > You mean fence inside the s390_get_feat_block() function?
>> Can we move that into s390_get_feat_block instead and check against the >> feature bit instead? >> > You mean fence inside the s390_get_feat_block() function? Yes, in s390_fill_feat_block() and to make it clean even in s390_has_feat() based on s390_is_pv().
On 21.10.20 16:18, Christian Borntraeger wrote: > > > On 21.10.20 16:14, David Hildenbrand wrote: >> On 21.10.20 15:43, Janosch Frank wrote: >>> Diag318 fencing needs to be determined on the current VM PV state and >>> not on the state that the VM has when we create the CPU model. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com> >>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318") >>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> >>> --- >>> hw/s390x/sclp.c | 10 ++++++++++ >>> target/s390x/kvm.c | 3 +-- >>> 2 files changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >>> index 0cf2290826..69aba402d3 100644 >>> --- a/hw/s390x/sclp.c >>> +++ b/hw/s390x/sclp.c >>> @@ -22,6 +22,7 @@ >>> #include "hw/s390x/event-facility.h" >>> #include "hw/s390x/s390-pci-bus.h" >>> #include "hw/s390x/ipl.h" >>> +#include "hw/s390x/pv.h" >>> >>> static inline SCLPDevice *get_sclp_device(void) >>> { >>> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) >>> if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) { >>> s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134, >>> &read_info->fac134); >>> + /* >>> + * Diag318 is not available in protected mode and will result >>> + * in an operation exception. As we can dynamically move in >>> + * and out of protected mode, we need to fence the feature >>> + * here rather than when creating the CPU model. >>> + */ >>> + if (s390_is_pv()) { >>> + read_info->fac134 &= ~0x80; >>> + } >> >> Hmm, I thought firmware would handle exposing cpu features and similar, >> so we can't temper with it .... > > Only the stfle bits. >> >> Can we move that into s390_get_feat_block instead and check against the >> feature bit instead? > > No because we want to have this active for !pv and disabled for pv but we switch > this multiple times when doing boot/reboot. Keeping the s390_is_pv() condition of course.
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index 0cf2290826..69aba402d3 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -22,6 +22,7 @@ #include "hw/s390x/event-facility.h" #include "hw/s390x/s390-pci-bus.h" #include "hw/s390x/ipl.h" +#include "hw/s390x/pv.h" static inline SCLPDevice *get_sclp_device(void) { @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) { s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134, &read_info->fac134); + /* + * Diag318 is not available in protected mode and will result + * in an operation exception. As we can dynamically move in + * and out of protected mode, we need to fence the feature + * here rather than when creating the CPU model. + */ + if (s390_is_pv()) { + read_info->fac134 &= ~0x80; + } } read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO | diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index f13eff688c..baa070fdf7 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -2498,8 +2498,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) */ set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features); - /* DIAGNOSE 0x318 is not supported under protected virtualization */ - if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) { + if (kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) { set_bit(S390_FEAT_DIAG_318, model->features); }