diff mbox series

[v2,12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

Message ID 20191129094809.26684-13-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: Protected Virtualization support | expand

Commit Message

Janosch Frank Nov. 29, 2019, 9:48 a.m. UTC 
IO instruction data is routed through SIDAD for protected guests, so
adresses do not need to be checked, as this is kernel memory.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/ioinst.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Comments

David Hildenbrand Nov. 29, 2019, 11:42 a.m. UTC | #1 
On 29.11.19 10:48, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.

"do not need" - is it actually evil to check? I would have assumed the
kernel checks and these checks are just superfluous but not dangerous?

IOW, some overhead we can ignore for now easily.

Same comment regarding doing the SIDAD access differently ...
Thomas Huth Dec. 4, 2019, 12:16 p.m. UTC | #2 
On 29/11/2019 10.48, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/ioinst.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>
Cornelia Huck Dec. 5, 2019, 5:44 p.m. UTC | #3 
On Fri, 29 Nov 2019 04:48:08 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/ioinst.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6..e4102430aa 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -17,6 +17,16 @@
>  #include "trace.h"
>  #include "hw/s390x/s390-pci-bus.h"
>  
> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
> +                                      uint8_t *ar)

Would like to keep the ioinst_* pattern here (even though this is an
internal function). ioinst_decode_addr()?

> +{
> +    if (env->pv) {
> +        *ar = 0;
> +        return 0;
> +    }
> +    return decode_basedisp_s(env, ipb, ar);
> +}
> +
>  int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
>                                   int *schid)
>  {

(...)

> @@ -601,7 +611,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
>  {
>      ChscReq *req;
>      ChscResp *res;
> -    uint64_t addr;
> +    uint64_t addr = 0;
>      int reg;
>      uint16_t len;
>      uint16_t command;
> @@ -610,7 +620,9 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
>  
>      trace_ioinst("chsc");
>      reg = (ipb >> 20) & 0x00f;
> -    addr = env->regs[reg];
> +    if (!env->pv) {
> +        addr = env->regs[reg];
> +    }

addr = env->pv ? 0 : env->regs[reg];

?

>      /* Page boundary? */
>      if (addr & 0xfff) {
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
diff mbox series

Patch

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index c437a1d8c6..e4102430aa 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -17,6 +17,16 @@ 
 #include "trace.h"
 #include "hw/s390x/s390-pci-bus.h"
 
+static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
+                                      uint8_t *ar)
+{
+    if (env->pv) {
+        *ar = 0;
+        return 0;
+    }
+    return decode_basedisp_s(env, ipb, ar);
+}
+
 int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
                                  int *schid)
 {
@@ -114,7 +124,7 @@  void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -171,7 +181,7 @@  void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -203,7 +213,7 @@  void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -234,7 +244,7 @@  void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
     CPUS390XState *env = &cpu->env;
     uint8_t ar;
 
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
@@ -303,7 +313,7 @@  int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
         return -EIO;
     }
     trace_ioinst_sch_id("tsch", cssid, ssid, schid);
-    addr = decode_basedisp_s(env, ipb, &ar);
+    addr = get_address_from_regs(env, ipb, &ar);
     if (addr & 3) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return -EIO;
@@ -601,7 +611,7 @@  void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
 {
     ChscReq *req;
     ChscResp *res;
-    uint64_t addr;
+    uint64_t addr = 0;
     int reg;
     uint16_t len;
     uint16_t command;
@@ -610,7 +620,9 @@  void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
 
     trace_ioinst("chsc");
     reg = (ipb >> 20) & 0x00f;
-    addr = env->regs[reg];
+    if (!env->pv) {
+        addr = env->regs[reg];
+    }
     /* Page boundary? */
     if (addr & 0xfff) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);