diff mbox

[10/11] s390x/sic: realize SIC handling

Message ID 1499864265-144136-11-git-send-email-borntraeger@de.ibm.com
State New
Headers show

Commit Message

Christian Borntraeger July 12, 2017, 12:57 p.m. UTC 
From: Fei Li <sherrylf@linux.vnet.ibm.com>

Currently, we do nothing for the SIC instruction, but we need to
implement it properly. Let's add proper handling in the backend code.

Co-authored-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
Signed-off-by: Fei Li <sherrylf@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/css.c         | 26 ++++++++++++++++++++++++++
 hw/s390x/trace-events  |  1 +
 include/hw/s390x/css.h |  2 ++
 target/s390x/kvm.c     | 16 +++++++++++++++-
 4 files changed, 44 insertions(+), 1 deletion(-)

Comments

Thomas Huth July 12, 2017, 1:40 p.m. UTC | #1 
On 12.07.2017 14:57, Christian Borntraeger wrote:
> From: Fei Li <sherrylf@linux.vnet.ibm.com>
> 
> Currently, we do nothing for the SIC instruction, but we need to
> implement it properly. Let's add proper handling in the backend code.
> 
> Co-authored-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> Signed-off-by: Fei Li <sherrylf@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/css.c         | 26 ++++++++++++++++++++++++++
>  hw/s390x/trace-events  |  1 +
>  include/hw/s390x/css.h |  2 ++
>  target/s390x/kvm.c     | 16 +++++++++++++++-
>  4 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 1fcbc84..7b82176 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -521,6 +521,32 @@ void css_conditional_io_interrupt(SubchDev *sch)
>      }
>  }
>  
> +int css_do_sic(CPUS390XState *env, uint8_t isc, uint16_t mode)
> +{
> +    S390FLICState *fs = s390_get_flic();
> +    S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);
> +    int r;
> +
> +    if (env->psw.mask & PSW_MASK_PSTATE) {
> +        r = -PGM_PRIVILEGED;
> +        goto out;

Why not simply:

           return -PGM_PRIVILEGED;

?

Same for the other goto below ... and then you also do not need the "r"
variable anymore.

> +    }
> +
> +    trace_css_do_sic(mode, isc);
> +    switch (mode) {
> +    case SIC_IRQ_MODE_ALL:
> +    case SIC_IRQ_MODE_SINGLE:
> +        break;
> +    default:
> +        r = -PGM_OPERAND;
> +        goto out;
> +    }
> +
> +    r = fsc->modify_ais_mode(fs, isc, mode) ? -PGM_OPERATION : 0;
> +out:
> +    return r;
> +}
> +
>  void css_adapter_interrupt(uint8_t isc)
>  {
>      uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;
> diff --git a/hw/s390x/trace-events b/hw/s390x/trace-events
> index 84ea964..f07e974 100644
> --- a/hw/s390x/trace-events
> +++ b/hw/s390x/trace-events
> @@ -8,6 +8,7 @@ css_new_image(uint8_t cssid, const char *default_cssid) "CSS: add css image %02x
>  css_assign_subch(const char *do_assign, uint8_t cssid, uint8_t ssid, uint16_t schid, uint16_t devno) "CSS: %s %x.%x.%04x (devno %04x)"
>  css_io_interrupt(int cssid, int ssid, int schid, uint32_t intparm, uint8_t isc, const char *conditional) "CSS: I/O interrupt on sch %x.%x.%04x (intparm %08x, isc %x) %s"
>  css_adapter_interrupt(uint8_t isc) "CSS: adapter I/O interrupt (isc %x)"
> +css_do_sic(uint16_t mode, uint8_t isc) "CSS: set interruption mode %x on isc %x"
>  
>  # hw/s390x/virtio-ccw.c
>  virtio_ccw_interpret_ccw(int cssid, int ssid, int schid, int cmd_code) "VIRTIO-CCW: %x.%x.%04x: interpret command %x"
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index e028960..5ee6d52 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -12,6 +12,7 @@
>  #ifndef CSS_H
>  #define CSS_H
>  
> +#include "cpu.h"

Not sure, but it's a little bit strange that the channel sub-system now
depends on the CPU ... maybe the check for problem state should rather
be done in kvm.c instead?
Or should css_do_sic() rather reside in s390-pci-inst.c or in ioinst.c
instead?

>  #include "hw/s390x/adapter.h"
>  #include "hw/s390x/s390_flic.h"
>  #include "hw/s390x/ioinst.h"
> @@ -165,6 +166,7 @@ typedef enum {
>      CSS_IO_ADAPTER_TYPE_NUMS,
>  } CssIoAdapterType;
>  
> +int css_do_sic(CPUS390XState *env, uint8_t isc, uint16_t mode);
>  uint32_t css_get_adapter_id(CssIoAdapterType type, uint8_t isc);
>  void css_register_io_adapters(CssIoAdapterType type, bool swap, bool maskable,
>                                uint8_t flags, Error **errp);
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 7a2a7c0..78ebe83 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1206,7 +1206,21 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>  
>  static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
>  {
> -    /* NOOP */
> +    CPUS390XState *env = &cpu->env;
> +    uint8_t r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +    uint8_t r3 = run->s390_sieic.ipa & 0x000f;
> +    uint8_t isc;
> +    uint16_t mode;
> +    int r;
> +
> +    cpu_synchronize_state(CPU(cpu));
> +    mode = env->regs[r1] & 0xffff;
> +    isc = (env->regs[r3] >> 27) & 0x7;
> +    r = css_do_sic(env, isc, mode);
> +    if (r) {
> +        enter_pgmcheck(cpu, -r);
> +    }
> +
>      return 0;
>  }

 Thomas
Cornelia Huck July 12, 2017, 3:40 p.m. UTC | #2 
On Wed, 12 Jul 2017 15:40:02 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 12.07.2017 14:57, Christian Borntraeger wrote:
> > From: Fei Li <sherrylf@linux.vnet.ibm.com>
> > 
> > Currently, we do nothing for the SIC instruction, but we need to
> > implement it properly. Let's add proper handling in the backend code.
> > 
> > Co-authored-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> > Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> > Signed-off-by: Fei Li <sherrylf@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  hw/s390x/css.c         | 26 ++++++++++++++++++++++++++
> >  hw/s390x/trace-events  |  1 +
> >  include/hw/s390x/css.h |  2 ++
> >  target/s390x/kvm.c     | 16 +++++++++++++++-
> >  4 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 1fcbc84..7b82176 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -521,6 +521,32 @@ void css_conditional_io_interrupt(SubchDev *sch)
> >      }
> >  }
> >  
> > +int css_do_sic(CPUS390XState *env, uint8_t isc, uint16_t mode)
> > +{
> > +    S390FLICState *fs = s390_get_flic();
> > +    S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);
> > +    int r;
> > +
> > +    if (env->psw.mask & PSW_MASK_PSTATE) {
> > +        r = -PGM_PRIVILEGED;
> > +        goto out;  
> 
> Why not simply:
> 
>            return -PGM_PRIVILEGED;
> 
> ?
> 
> Same for the other goto below ... and then you also do not need the "r"
> variable anymore.

I'm slightly in favour of the single exit style.

> 
> > +    }
> > +
> > +    trace_css_do_sic(mode, isc);
> > +    switch (mode) {
> > +    case SIC_IRQ_MODE_ALL:
> > +    case SIC_IRQ_MODE_SINGLE:
> > +        break;
> > +    default:
> > +        r = -PGM_OPERAND;
> > +        goto out;
> > +    }
> > +
> > +    r = fsc->modify_ais_mode(fs, isc, mode) ? -PGM_OPERATION : 0;
> > +out:
> > +    return r;
> > +}
> > +
> >  void css_adapter_interrupt(uint8_t isc)
> >  {
> >      uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;

(...)

> > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> > index e028960..5ee6d52 100644
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h
> > @@ -12,6 +12,7 @@
> >  #ifndef CSS_H
> >  #define CSS_H
> >  
> > +#include "cpu.h"  
> 
> Not sure, but it's a little bit strange that the channel sub-system now
> depends on the CPU ... maybe the check for problem state should rather
> be done in kvm.c instead?

This would mean that tcg would need to duplicate those checks should we
want to wire it up in the future. (FWIW, the sclp backend code in
hw/s390x/sclp.c also does the respective checks, probably for exactly
that reason.)

> Or should css_do_sic() rather reside in s390-pci-inst.c or in ioinst.c
> instead?

s390-pci-inst.c is only a good choice if we are sure that sic will be
applicable for pci only even in the future - and I don't think we can
be. (The ais stuff is tied to the adapter, who knows which adapter
types may arrive in the future...)

I/O interrupt injection is currently triggered from the css code (where
it belongs IMO), so I think the code should stay where it is now.
diff mbox

Patch

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 1fcbc84..7b82176 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -521,6 +521,32 @@  void css_conditional_io_interrupt(SubchDev *sch)
     }
 }
 
+int css_do_sic(CPUS390XState *env, uint8_t isc, uint16_t mode)
+{
+    S390FLICState *fs = s390_get_flic();
+    S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);
+    int r;
+
+    if (env->psw.mask & PSW_MASK_PSTATE) {
+        r = -PGM_PRIVILEGED;
+        goto out;
+    }
+
+    trace_css_do_sic(mode, isc);
+    switch (mode) {
+    case SIC_IRQ_MODE_ALL:
+    case SIC_IRQ_MODE_SINGLE:
+        break;
+    default:
+        r = -PGM_OPERAND;
+        goto out;
+    }
+
+    r = fsc->modify_ais_mode(fs, isc, mode) ? -PGM_OPERATION : 0;
+out:
+    return r;
+}
+
 void css_adapter_interrupt(uint8_t isc)
 {
     uint32_t io_int_word = (isc << 27) | IO_INT_WORD_AI;
diff --git a/hw/s390x/trace-events b/hw/s390x/trace-events
index 84ea964..f07e974 100644
--- a/hw/s390x/trace-events
+++ b/hw/s390x/trace-events
@@ -8,6 +8,7 @@  css_new_image(uint8_t cssid, const char *default_cssid) "CSS: add css image %02x
 css_assign_subch(const char *do_assign, uint8_t cssid, uint8_t ssid, uint16_t schid, uint16_t devno) "CSS: %s %x.%x.%04x (devno %04x)"
 css_io_interrupt(int cssid, int ssid, int schid, uint32_t intparm, uint8_t isc, const char *conditional) "CSS: I/O interrupt on sch %x.%x.%04x (intparm %08x, isc %x) %s"
 css_adapter_interrupt(uint8_t isc) "CSS: adapter I/O interrupt (isc %x)"
+css_do_sic(uint16_t mode, uint8_t isc) "CSS: set interruption mode %x on isc %x"
 
 # hw/s390x/virtio-ccw.c
 virtio_ccw_interpret_ccw(int cssid, int ssid, int schid, int cmd_code) "VIRTIO-CCW: %x.%x.%04x: interpret command %x"
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index e028960..5ee6d52 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -12,6 +12,7 @@ 
 #ifndef CSS_H
 #define CSS_H
 
+#include "cpu.h"
 #include "hw/s390x/adapter.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"
@@ -165,6 +166,7 @@  typedef enum {
     CSS_IO_ADAPTER_TYPE_NUMS,
 } CssIoAdapterType;
 
+int css_do_sic(CPUS390XState *env, uint8_t isc, uint16_t mode);
 uint32_t css_get_adapter_id(CssIoAdapterType type, uint8_t isc);
 void css_register_io_adapters(CssIoAdapterType type, bool swap, bool maskable,
                               uint8_t flags, Error **errp);
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 7a2a7c0..78ebe83 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1206,7 +1206,21 @@  static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
 
 static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
 {
-    /* NOOP */
+    CPUS390XState *env = &cpu->env;
+    uint8_t r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
+    uint8_t r3 = run->s390_sieic.ipa & 0x000f;
+    uint8_t isc;
+    uint16_t mode;
+    int r;
+
+    cpu_synchronize_state(CPU(cpu));
+    mode = env->regs[r1] & 0xffff;
+    isc = (env->regs[r3] >> 27) & 0x7;
+    r = css_do_sic(env, isc, mode);
+    if (r) {
+        enter_pgmcheck(cpu, -r);
+    }
+
     return 0;
 }