| Message ID | 20190805152947.28536-2-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: MMU changes and extensions | expand |
On Mon, 5 Aug 2019 17:29:39 +0200 David Hildenbrand <david@redhat.com> wrote: > Let's select the ASC before calling the function and use MMU_DATA_LOAD. > This is a preparation to: > - Remove the ASC magic depending on the access mode from mmu_translate > - Implement IEP support, where we could run into access exceptions 'IEP' was instruction execution protection? > trying to fetch instructions > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/helper.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/target/s390x/helper.c b/target/s390x/helper.c > index 13ae9909ad..08166558a0 100644 > --- a/target/s390x/helper.c > +++ b/target/s390x/helper.c > @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) > vaddr &= 0x7fffffff; > } > > - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { > + /* > + * We want to read the code, however, not run into access exceptions > + * (especially, IEP). > + */ > + if (asc != PSW_ASC_HOME) { > + asc = PSW_ASC_PRIMARY; > + } Previously, if we'd go in here specifying access register mode, we'd hw_error() out. Now, that would be rewritten to using primary. Could that be a problem, or do we filter out access register mode even earlier? (As an aside, I'm not sure the guest crashing qemu by specifying access register mode was a good idea. Or do we get to slap the guest before that happens?) > + > + if (mmu_translate(env, vaddr, MMU_DATA_LOAD, asc, &raddr, &prot, false)) { > return -1; > } > return raddr;
On 08.08.19 14:57, Cornelia Huck wrote: > On Mon, 5 Aug 2019 17:29:39 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> Let's select the ASC before calling the function and use MMU_DATA_LOAD. >> This is a preparation to: >> - Remove the ASC magic depending on the access mode from mmu_translate >> - Implement IEP support, where we could run into access exceptions > > 'IEP' was instruction execution protection? > >> trying to fetch instructions >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/helper.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/target/s390x/helper.c b/target/s390x/helper.c >> index 13ae9909ad..08166558a0 100644 >> --- a/target/s390x/helper.c >> +++ b/target/s390x/helper.c >> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) >> vaddr &= 0x7fffffff; >> } >> >> - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { >> + /* >> + * We want to read the code, however, not run into access exceptions >> + * (especially, IEP). >> + */ >> + if (asc != PSW_ASC_HOME) { >> + asc = PSW_ASC_PRIMARY; >> + } > > Previously, if we'd go in here specifying access register mode, we'd > hw_error() out. Now, that would be rewritten to using primary. Could > that be a problem, or do we filter out access register mode even > earlier? As this is just a debug interface it's not an issue (it actually makes sure that we really read code and not data). Any guest that would be trying to read data while in AR mode would immediately crash. > > (As an aside, I'm not sure the guest crashing qemu by specifying access > register mode was a good idea. Or do we get to slap the guest before > that happens?) I guess there isn't too much we can do - continue running the guest in a wrong mode would lead to data corruption :/ And as it's core ISA, we cannot really forbid switching to it. The only solution is to implement AR mode :D
On 8/5/19 5:29 PM, David Hildenbrand wrote: > Let's select the ASC before calling the function and use MMU_DATA_LOAD. > This is a preparation to: > - Remove the ASC magic depending on the access mode from mmu_translate > - Implement IEP support, where we could run into access exceptions > trying to fetch instructions > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/helper.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/target/s390x/helper.c b/target/s390x/helper.c > index 13ae9909ad..08166558a0 100644 > --- a/target/s390x/helper.c > +++ b/target/s390x/helper.c > @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) > vaddr &= 0x7fffffff; > } > > - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { > + /* > + * We want to read the code, however, not run into access exceptions Is this really a safe assumption here that we always use this to translate code addresses and not data addresses? ... I don't think so. For example with the "gva2gpa" HMP command, I'd rather expect that it also works with the secondary space mode...? So maybe we need a proper MemTxAttrs bit or something similar for distinguishing instruction accesses from data accesses here? Thomas
On 12.08.19 09:12, Thomas Huth wrote: > On 8/5/19 5:29 PM, David Hildenbrand wrote: >> Let's select the ASC before calling the function and use MMU_DATA_LOAD. >> This is a preparation to: >> - Remove the ASC magic depending on the access mode from mmu_translate >> - Implement IEP support, where we could run into access exceptions >> trying to fetch instructions >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/helper.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/target/s390x/helper.c b/target/s390x/helper.c >> index 13ae9909ad..08166558a0 100644 >> --- a/target/s390x/helper.c >> +++ b/target/s390x/helper.c >> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) >> vaddr &= 0x7fffffff; >> } >> >> - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { >> + /* >> + * We want to read the code, however, not run into access exceptions > > Is this really a safe assumption here that we always use this to > translate code addresses and not data addresses? ... I don't think so. > For example with the "gva2gpa" HMP command, I'd rather expect that it > also works with the secondary space mode...? Well, it's what current code does. I am not changing that behavior. While it is in general broken to have a single interface to debug code+data (which is only a problem on s390x), it makes a lot of sense if you think about single-stepping through disassembled code using the gdbstub. Or dumping code where you crashed. In Linux, code+data will luckily usually have the same virtual->physical tables, so it's not a real issue. > > So maybe we need a proper MemTxAttrs bit or something similar for > distinguishing instruction accesses from data accesses here? There would first have to be a way to ask "get_phys_page_debug" to get code or data for this to make sense. Right now we used it to get code. Thanks.
On Mon, 12 Aug 2019 09:52:56 +0200 David Hildenbrand <david@redhat.com> wrote: > On 12.08.19 09:12, Thomas Huth wrote: > > On 8/5/19 5:29 PM, David Hildenbrand wrote: > >> Let's select the ASC before calling the function and use MMU_DATA_LOAD. > >> This is a preparation to: > >> - Remove the ASC magic depending on the access mode from mmu_translate > >> - Implement IEP support, where we could run into access exceptions > >> trying to fetch instructions > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/helper.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/target/s390x/helper.c b/target/s390x/helper.c > >> index 13ae9909ad..08166558a0 100644 > >> --- a/target/s390x/helper.c > >> +++ b/target/s390x/helper.c > >> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) > >> vaddr &= 0x7fffffff; > >> } > >> > >> - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { > >> + /* > >> + * We want to read the code, however, not run into access exceptions > > > > Is this really a safe assumption here that we always use this to > > translate code addresses and not data addresses? ... I don't think so. > > For example with the "gva2gpa" HMP command, I'd rather expect that it > > also works with the secondary space mode...? > > Well, it's what current code does. I am not changing that behavior. Agreed, that is not actively breaking something. > > While it is in general broken to have a single interface to debug > code+data (which is only a problem on s390x), it makes a lot of sense if > you think about single-stepping through disassembled code using the > gdbstub. Or dumping code where you crashed. What about the memsave interface? > > In Linux, code+data will luckily usually have the same virtual->physical > tables, so it's not a real issue. > > > > > So maybe we need a proper MemTxAttrs bit or something similar for > > distinguishing instruction accesses from data accesses here? > > There would first have to be a way to ask "get_phys_page_debug" to get > code or data for this to make sense. Right now we used it to get code. I'm wondering if we're able to do better; but if the code/data distinction is not considered in architecture independent code, probably not easily.
On 12.08.19 15:40, Cornelia Huck wrote: > On Mon, 12 Aug 2019 09:52:56 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 12.08.19 09:12, Thomas Huth wrote: >>> On 8/5/19 5:29 PM, David Hildenbrand wrote: >>>> Let's select the ASC before calling the function and use MMU_DATA_LOAD. >>>> This is a preparation to: >>>> - Remove the ASC magic depending on the access mode from mmu_translate >>>> - Implement IEP support, where we could run into access exceptions >>>> trying to fetch instructions >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> target/s390x/helper.c | 10 +++++++++- >>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c >>>> index 13ae9909ad..08166558a0 100644 >>>> --- a/target/s390x/helper.c >>>> +++ b/target/s390x/helper.c >>>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) >>>> vaddr &= 0x7fffffff; >>>> } >>>> >>>> - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { >>>> + /* >>>> + * We want to read the code, however, not run into access exceptions >>> >>> Is this really a safe assumption here that we always use this to >>> translate code addresses and not data addresses? ... I don't think so. >>> For example with the "gva2gpa" HMP command, I'd rather expect that it >>> also works with the secondary space mode...? >> >> Well, it's what current code does. I am not changing that behavior. > > Agreed, that is not actively breaking something. > >> >> While it is in general broken to have a single interface to debug >> code+data (which is only a problem on s390x), it makes a lot of sense if >> you think about single-stepping through disassembled code using the >> gdbstub. Or dumping code where you crashed. > > What about the memsave interface? I guess the same problem: "save to disk virtual memory dump starting at @var{addr} of size @var{size}" - which virtual memory (code vs. data)? These old interface are really x86 specific (meaning: it made sense this way for x86) I'd like to note that if our KVM guest is in AR mode, we would now no longer be able to crash it :) (well, a nice side-effect of instruction fetches not going via AR mode).
On Mon, 12 Aug 2019 15:45:25 +0200 David Hildenbrand <david@redhat.com> wrote: > On 12.08.19 15:40, Cornelia Huck wrote: > > On Mon, 12 Aug 2019 09:52:56 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 12.08.19 09:12, Thomas Huth wrote: > >>> On 8/5/19 5:29 PM, David Hildenbrand wrote: > >>>> Let's select the ASC before calling the function and use MMU_DATA_LOAD. > >>>> This is a preparation to: > >>>> - Remove the ASC magic depending on the access mode from mmu_translate > >>>> - Implement IEP support, where we could run into access exceptions > >>>> trying to fetch instructions > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> target/s390x/helper.c | 10 +++++++++- > >>>> 1 file changed, 9 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c > >>>> index 13ae9909ad..08166558a0 100644 > >>>> --- a/target/s390x/helper.c > >>>> +++ b/target/s390x/helper.c > >>>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) > >>>> vaddr &= 0x7fffffff; > >>>> } > >>>> > >>>> - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { > >>>> + /* > >>>> + * We want to read the code, however, not run into access exceptions > >>> > >>> Is this really a safe assumption here that we always use this to > >>> translate code addresses and not data addresses? ... I don't think so. > >>> For example with the "gva2gpa" HMP command, I'd rather expect that it > >>> also works with the secondary space mode...? > >> > >> Well, it's what current code does. I am not changing that behavior. > > > > Agreed, that is not actively breaking something. > > > >> > >> While it is in general broken to have a single interface to debug > >> code+data (which is only a problem on s390x), it makes a lot of sense if > >> you think about single-stepping through disassembled code using the > >> gdbstub. Or dumping code where you crashed. > > > > What about the memsave interface? > > I guess the same problem: > > "save to disk virtual memory dump starting at @var{addr} of size > @var{size}" - which virtual memory (code vs. data)? These old interface > are really x86 specific (meaning: it made sense this way for x86) So, the general verdict is "gnarly interface, but at least not broken for Linux guests", I guess. > > I'd like to note that if our KVM guest is in AR mode, we would now no > longer be able to crash it :) (well, a nice side-effect of instruction > fetches not going via AR mode). Heh :) Q: What do we need to consider beyond Linux guests? Probably not much, given that they would be broken already...
On 12.08.19 15:58, Cornelia Huck wrote: > On Mon, 12 Aug 2019 15:45:25 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 12.08.19 15:40, Cornelia Huck wrote: >>> On Mon, 12 Aug 2019 09:52:56 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> On 12.08.19 09:12, Thomas Huth wrote: >>>>> On 8/5/19 5:29 PM, David Hildenbrand wrote: >>>>>> Let's select the ASC before calling the function and use MMU_DATA_LOAD. >>>>>> This is a preparation to: >>>>>> - Remove the ASC magic depending on the access mode from mmu_translate >>>>>> - Implement IEP support, where we could run into access exceptions >>>>>> trying to fetch instructions >>>>>> >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>> --- >>>>>> target/s390x/helper.c | 10 +++++++++- >>>>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c >>>>>> index 13ae9909ad..08166558a0 100644 >>>>>> --- a/target/s390x/helper.c >>>>>> +++ b/target/s390x/helper.c >>>>>> @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) >>>>>> vaddr &= 0x7fffffff; >>>>>> } >>>>>> >>>>>> - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { >>>>>> + /* >>>>>> + * We want to read the code, however, not run into access exceptions >>>>> >>>>> Is this really a safe assumption here that we always use this to >>>>> translate code addresses and not data addresses? ... I don't think so. >>>>> For example with the "gva2gpa" HMP command, I'd rather expect that it >>>>> also works with the secondary space mode...? >>>> >>>> Well, it's what current code does. I am not changing that behavior. >>> >>> Agreed, that is not actively breaking something. >>> >>>> >>>> While it is in general broken to have a single interface to debug >>>> code+data (which is only a problem on s390x), it makes a lot of sense if >>>> you think about single-stepping through disassembled code using the >>>> gdbstub. Or dumping code where you crashed. >>> >>> What about the memsave interface? >> >> I guess the same problem: >> >> "save to disk virtual memory dump starting at @var{addr} of size >> @var{size}" - which virtual memory (code vs. data)? These old interface >> are really x86 specific (meaning: it made sense this way for x86) > > So, the general verdict is "gnarly interface, but at least not broken > for Linux guests", I guess. > >> >> I'd like to note that if our KVM guest is in AR mode, we would now no >> longer be able to crash it :) (well, a nice side-effect of instruction >> fetches not going via AR mode). > > Heh :) > > Q: What do we need to consider beyond Linux guests? Probably not much, > given that they would be broken already... > I think Linux guests only use AR mode for some instances of vDSO, but I don't recall the details (and why it never was an issue for TCG, where we fail hard early). Apart from that, I don't think we have to consider other guests (kvm-unit-tests ...).
diff --git a/target/s390x/helper.c b/target/s390x/helper.c index 13ae9909ad..08166558a0 100644 --- a/target/s390x/helper.c +++ b/target/s390x/helper.c @@ -58,7 +58,15 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) vaddr &= 0x7fffffff; } - if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { + /* + * We want to read the code, however, not run into access exceptions + * (especially, IEP). + */ + if (asc != PSW_ASC_HOME) { + asc = PSW_ASC_PRIMARY; + } + + if (mmu_translate(env, vaddr, MMU_DATA_LOAD, asc, &raddr, &prot, false)) { return -1; } return raddr;
Let's select the ASC before calling the function and use MMU_DATA_LOAD. This is a preparation to: - Remove the ASC magic depending on the access mode from mmu_translate - Implement IEP support, where we could run into access exceptions trying to fetch instructions Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/helper.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)