| Message ID | 20200304114231.23493-14-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Protected Virtualization support | expand |
On 04.03.20 12:42, 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> > Reviewed-by: Thomas Huth <thuth@redhat.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) > +{ Please add a comment here why this is done. (e.g., make all address checks - like alignment checks - in the caller succeed, and we don't need the address). > + if (env->pv) { > + *ar = 0; > + return 0; > + } > + return decode_basedisp_s(env, ipb, ar); > +} > + Reviewed-by: David Hildenbrand <david@redhat.com>
On 3/4/20 6:55 PM, David Hildenbrand wrote: > On 04.03.20 12:42, 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> >> Reviewed-by: Thomas Huth <thuth@redhat.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) >> +{ > > Please add a comment here why this is done. (e.g., make all address > checks - like alignment checks - in the caller succeed, and we don't > need the address). * 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 (env->pv) { >> + *ar = 0; >> + return 0; >> + } >> + return decode_basedisp_s(env, ipb, ar); >> +} >> + > > Reviewed-by: David Hildenbrand <david@redhat.com> > >
On 05.03.20 10:42, Janosch Frank wrote: > On 3/4/20 6:55 PM, David Hildenbrand wrote: >> On 04.03.20 12:42, 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> >>> Reviewed-by: Thomas Huth <thuth@redhat.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) >>> +{ >> >> Please add a comment here why this is done. (e.g., make all address >> checks - like alignment checks - in the caller succeed, and we don't >> need the address). > > * Addresses for protected guests are all offsets into the > > > * satellite block which holds the IO control structures. Those maybe mention SIDA as well > > > * control structures are always aligned and accessible, so we can > > > * return 0 here which will pass the following address checks. > > ? Sounds good!
On 3/5/20 11:00 AM, David Hildenbrand wrote: > On 05.03.20 10:42, Janosch Frank wrote: >> On 3/4/20 6:55 PM, David Hildenbrand wrote: >>> On 04.03.20 12:42, 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> >>>> Reviewed-by: Thomas Huth <thuth@redhat.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) >>>> +{ >>> >>> Please add a comment here why this is done. (e.g., make all address >>> checks - like alignment checks - in the caller succeed, and we don't >>> need the address). >> >> * Addresses for protected guests are all offsets into the >> >> >> * satellite block which holds the IO control structures. Those > > maybe mention SIDA as well huh? SIDA is the satellite block > >> >> >> * control structures are always aligned and accessible, so we can >> >> >> * return 0 here which will pass the following address checks. >> >> ? > > > Sounds good! > >
On 05.03.20 12:26, Janosch Frank wrote: > On 3/5/20 11:00 AM, David Hildenbrand wrote: >> On 05.03.20 10:42, Janosch Frank wrote: >>> On 3/4/20 6:55 PM, David Hildenbrand wrote: >>>> On 04.03.20 12:42, 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> >>>>> Reviewed-by: Thomas Huth <thuth@redhat.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) >>>>> +{ >>>> >>>> Please add a comment here why this is done. (e.g., make all address >>>> checks - like alignment checks - in the caller succeed, and we don't >>>> need the address). >>> >>> * Addresses for protected guests are all offsets into the >>> >>> >>> * satellite block which holds the IO control structures. Those >> >> maybe mention SIDA as well > > huh? SIDA is the satellite block Yes, please stick to a consistent terminology. Mix and matching "SIDA" and "satellite block" does not improve readability IMHO.
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);