diff mbox series

[v9,11/15] s390x: protvirt: Disable address checks for PV guest IO emulation

Message ID 20200311132151.172389-12-frankja@linux.ibm.com
State New
Headers show
Series s390x: Protected Virtualization support | expand

Commit Message

Janosch Frank March 11, 2020, 1:21 p.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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Christian Borntraeger March 12, 2020, 3:41 p.m. UTC | #1 
On 11.03.20 14:21, 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.

Maybe also mention that the data structures for  the I/O instrutions
start at offset 0 of the SIDA?
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>


Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>



> ---
>  target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6afed80..481d789de9e09a04 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -16,6 +16,23 @@
>  #include "hw/s390x/ioinst.h"
>  #include "trace.h"
>  #include "hw/s390x/s390-pci-bus.h"
> +#include "hw/s390x/pv.h"
> +
> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
> +                                      uint8_t *ar)
> +{
> +    /*
> +     * Addresses for protected guests are all offsets into the
> +     * satellite block which holds the IO control structures. Those
> +     * control structures are always aligned and accessible, so we can
> +     * return 0 here which will pass the following address checks.
> +     */
> +    if (s390_is_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 +131,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 +188,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 +220,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 +251,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 +320,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 +618,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 +627,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 (!s390_is_pv()) {
> +        addr = env->regs[reg];
> +    }
>      /* Page boundary? */
>      if (addr & 0xfff) {
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>
Janosch Frank March 12, 2020, 4:07 p.m. UTC | #2 
On 3/12/20 4:41 PM, Christian Borntraeger wrote:
> On 11.03.20 14:21, 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.
> 
> Maybe also mention that the data structures for  the I/O instrutions
> start at offset 0 of the SIDA?

You mean in the comment for get_address_from_regs() or in the commit
message? It would certainly make sense for get_address_from_regs().

>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
> 
>> ---
>>  target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
>>  1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>> index c437a1d8c6afed80..481d789de9e09a04 100644
>> --- a/target/s390x/ioinst.c
>> +++ b/target/s390x/ioinst.c
>> @@ -16,6 +16,23 @@
>>  #include "hw/s390x/ioinst.h"
>>  #include "trace.h"
>>  #include "hw/s390x/s390-pci-bus.h"
>> +#include "hw/s390x/pv.h"
>> +
>> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
>> +                                      uint8_t *ar)
>> +{
>> +    /*
>> +     * Addresses for protected guests are all offsets into the
>> +     * satellite block which holds the IO control structures. Those
>> +     * control structures are always aligned and accessible, so we can
>> +     * return 0 here which will pass the following address checks.
>> +     */
>> +    if (s390_is_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 +131,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 +188,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 +220,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 +251,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 +320,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 +618,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 +627,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 (!s390_is_pv()) {
>> +        addr = env->regs[reg];
>> +    }
>>      /* Page boundary? */
>>      if (addr & 0xfff) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>
Christian Borntraeger March 12, 2020, 4:10 p.m. UTC | #3 
On 12.03.20 17:07, Janosch Frank wrote:
> On 3/12/20 4:41 PM, Christian Borntraeger wrote:
>> On 11.03.20 14:21, 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.
>>
>> Maybe also mention that the data structures for  the I/O instrutions
>> start at offset 0 of the SIDA?
> 
> You mean in the comment for get_address_from_regs() or in the commit
> message? It would certainly make sense for get_address_from_regs().

Maybe both?
Janosch Frank March 12, 2020, 4:17 p.m. UTC | #4 
On 3/12/20 5:10 PM, Christian Borntraeger wrote:
> 
> 
> On 12.03.20 17:07, Janosch Frank wrote:
>> On 3/12/20 4:41 PM, Christian Borntraeger wrote:
>>> On 11.03.20 14:21, 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.
>>>
>>> Maybe also mention that the data structures for  the I/O instrutions
>>> start at offset 0 of the SIDA?
>>
>> You mean in the comment for get_address_from_regs() or in the commit
>> message? It would certainly make sense for get_address_from_regs().
> 
> Maybe both?
> 

This is what I have now:

s390x: protvirt: Disable address checks for PV guest IO emulation

IO instruction data is routed through SIDAD for protected guests, so
adresses do not need to be checked, as this is kernel memory which is
always available.

Also the instruction data always starts at offset 0 of the SIDAD.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

and

+    /*


+     * Addresses for protected guests are all offsets into the


+     * satellite block which holds the IO control structures. Those


+     * control structures are always starting at offset 0 and are


+     * always aligned and accessible. So we can return 0 here which


+     * will pass the following address checks.


+     */
Christian Borntraeger March 13, 2020, 7:38 a.m. UTC | #5 
On 12.03.20 17:17, Janosch Frank wrote:
> On 3/12/20 5:10 PM, Christian Borntraeger wrote:
>>
>>
>> On 12.03.20 17:07, Janosch Frank wrote:
>>> On 3/12/20 4:41 PM, Christian Borntraeger wrote:
>>>> On 11.03.20 14:21, 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.
>>>>
>>>> Maybe also mention that the data structures for  the I/O instrutions
>>>> start at offset 0 of the SIDA?
>>>
>>> You mean in the comment for get_address_from_regs() or in the commit
>>> message? It would certainly make sense for get_address_from_regs().
>>
>> Maybe both?
>>
> 
> This is what I have now:
> 
> s390x: protvirt: Disable address checks for PV guest IO emulation
> 
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory which is
> always available.
> 
> Also the instruction data always starts at offset 0 of the SIDAD.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> and
> 
> +    /*
> 
> 
> +     * Addresses for protected guests are all offsets into the
> 
> 
> +     * satellite block which holds the IO control structures. Those
> 
> 
> +     * control structures are always starting at offset 0 and are
> 
> 
> +     * always aligned and accessible. So we can return 0 here which
> 
> 
> +     * will pass the following address checks.
> 
> 
> +     */
> 

Looks perfect without the empty lines.
Claudio Imbrenda March 13, 2020, 12:57 p.m. UTC | #6 
On Wed, 11 Mar 2020 09:21:47 -0400
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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6afed80..481d789de9e09a04 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -16,6 +16,23 @@
>  #include "hw/s390x/ioinst.h"
>  #include "trace.h"
>  #include "hw/s390x/s390-pci-bus.h"
> +#include "hw/s390x/pv.h"
> +
> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t
> ipb,
> +                                      uint8_t *ar)
> +{
> +    /*
> +     * Addresses for protected guests are all offsets into the
> +     * satellite block which holds the IO control structures. Those
> +     * control structures are always aligned and accessible, so we
> can
> +     * return 0 here which will pass the following address checks.
> +     */
> +    if (s390_is_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 +131,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 +188,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 +220,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 +251,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 +320,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 +618,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 +627,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 (!s390_is_pv()) {
> +        addr = env->regs[reg];
> +    }
>      /* Page boundary? */
>      if (addr & 0xfff) {
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cornelia Huck March 17, 2020, 11:36 a.m. UTC | #7 
On Wed, 11 Mar 2020 09:21:47 -0400
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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6afed80..481d789de9e09a04 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -16,6 +16,23 @@
>  #include "hw/s390x/ioinst.h"
>  #include "trace.h"
>  #include "hw/s390x/s390-pci-bus.h"
> +#include "hw/s390x/pv.h"
> +

Maybe add

/* all I/O instructions but chsc use the s format */

to emphasize that this is not a generic decoder?

> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
> +                                      uint8_t *ar)
> +{
> +    /*
> +     * Addresses for protected guests are all offsets into the
> +     * satellite block which holds the IO control structures. Those
> +     * control structures are always aligned and accessible, so we can
> +     * return 0 here which will pass the following address checks.
> +     */
> +    if (s390_is_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)

With the other comment updates,

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Janosch Frank March 17, 2020, noon UTC | #8 
On 3/17/20 12:36 PM, Cornelia Huck wrote:
> On Wed, 11 Mar 2020 09:21:47 -0400
> 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>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
>>  1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>> index c437a1d8c6afed80..481d789de9e09a04 100644
>> --- a/target/s390x/ioinst.c
>> +++ b/target/s390x/ioinst.c
>> @@ -16,6 +16,23 @@
>>  #include "hw/s390x/ioinst.h"
>>  #include "trace.h"
>>  #include "hw/s390x/s390-pci-bus.h"
>> +#include "hw/s390x/pv.h"
>> +
> 
> Maybe add
> 
> /* all I/O instructions but chsc use the s format *>
> to emphasize that this is not a generic decoder?

Ack

> 
>> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
>> +                                      uint8_t *ar)
>> +{
>> +    /*
>> +     * Addresses for protected guests are all offsets into the
>> +     * satellite block which holds the IO control structures. Those
>> +     * control structures are always aligned and accessible, so we can
>> +     * return 0 here which will pass the following address checks.
>> +     */
>> +    if (s390_is_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)
> 
> With the other comment updates,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> 
Thanks
diff mbox series

Patch

diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index c437a1d8c6afed80..481d789de9e09a04 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -16,6 +16,23 @@ 
 #include "hw/s390x/ioinst.h"
 #include "trace.h"
 #include "hw/s390x/s390-pci-bus.h"
+#include "hw/s390x/pv.h"
+
+static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
+                                      uint8_t *ar)
+{
+    /*
+     * Addresses for protected guests are all offsets into the
+     * satellite block which holds the IO control structures. Those
+     * control structures are always aligned and accessible, so we can
+     * return 0 here which will pass the following address checks.
+     */
+    if (s390_is_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 +131,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 +188,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 +220,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 +251,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 +320,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 +618,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 +627,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 (!s390_is_pv()) {
+        addr = env->regs[reg];
+    }
     /* Page boundary? */
     if (addr & 0xfff) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);