| Message ID | 20190812112737.6652-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/mmu: Storage key reference and change bit handling | expand |
On 8/12/19 1:27 PM, David Hildenbrand wrote: > Let's select the ASC before calling the function. This is a prepararion > to remove the ASC magic depending on the access mode from mmu_translate. > > There is currently no way to distinguish if we have code or data access. > For now, we were using code access, because especially when debugging with > the gdbstub, we want to read and disassemble what we single-step. IMHO we should add a "instruction" bit to MemTxAttrs and then use the ...page_attrs_debug() interface instead. But ok, that's likely really something for a separate clean-up, so for the time being: Reviewed-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/helper.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/target/s390x/helper.c b/target/s390x/helper.c > index 13ae9909ad..c5fb8966b6 100644 > --- a/target/s390x/helper.c > +++ b/target/s390x/helper.c > @@ -58,6 +58,11 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) > vaddr &= 0x7fffffff; > } > > + /* We want to read the code (e.g., see what we are single-stepping).*/ > + if (asc != PSW_ASC_HOME) { > + asc = PSW_ASC_PRIMARY; > + } > + > if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { > return -1; > } >
On 12.08.19 17:18, Thomas Huth wrote: > On 8/12/19 1:27 PM, David Hildenbrand wrote: >> Let's select the ASC before calling the function. This is a prepararion >> to remove the ASC magic depending on the access mode from mmu_translate. >> >> There is currently no way to distinguish if we have code or data access. >> For now, we were using code access, because especially when debugging with >> the gdbstub, we want to read and disassemble what we single-step. > > IMHO we should add a "instruction" bit to MemTxAttrs and then use the > ...page_attrs_debug() interface instead. But ok, that's likely really > something for a separate clean-up, so for the time being: > That sounds like a good idea, and then switching over to cc->get_phys_page_attrs() Thanks! > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On 12.08.19 17:28, David Hildenbrand wrote: > On 12.08.19 17:18, Thomas Huth wrote: >> On 8/12/19 1:27 PM, David Hildenbrand wrote: >>> Let's select the ASC before calling the function. This is a prepararion >>> to remove the ASC magic depending on the access mode from mmu_translate. >>> >>> There is currently no way to distinguish if we have code or data access. >>> For now, we were using code access, because especially when debugging with >>> the gdbstub, we want to read and disassemble what we single-step. >> >> IMHO we should add a "instruction" bit to MemTxAttrs and then use the >> ...page_attrs_debug() interface instead. But ok, that's likely really >> something for a separate clean-up, so for the time being: >> > > That sounds like a good idea, and then switching over to > cc->get_phys_page_attrs() But looking at get_phys_page_attrs_debug() again, "MemTxAttrs *attrs" is an output value not an input value. So there wouldn't be a way to specify "what you want" from the caller. At least unless I am missing something :) > > Thanks! > >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> > >
On 8/12/19 5:39 PM, David Hildenbrand wrote: > On 12.08.19 17:28, David Hildenbrand wrote: >> On 12.08.19 17:18, Thomas Huth wrote: >>> On 8/12/19 1:27 PM, David Hildenbrand wrote: >>>> Let's select the ASC before calling the function. This is a prepararion >>>> to remove the ASC magic depending on the access mode from mmu_translate. >>>> >>>> There is currently no way to distinguish if we have code or data access. >>>> For now, we were using code access, because especially when debugging with >>>> the gdbstub, we want to read and disassemble what we single-step. >>> >>> IMHO we should add a "instruction" bit to MemTxAttrs and then use the >>> ...page_attrs_debug() interface instead. But ok, that's likely really >>> something for a separate clean-up, so for the time being: >>> >> >> That sounds like a good idea, and then switching over to >> cc->get_phys_page_attrs() > > But looking at get_phys_page_attrs_debug() again, "MemTxAttrs *attrs" is > an output value not an input value. So there wouldn't be a way to > specify "what you want" from the caller. At least unless I am missing > something :) Oops, you're right. Too bad :-( So never mind that idea ... your patch is certainly the best you can do here right now. Thomas
On Mon, 12 Aug 2019 13:27:32 +0200 David Hildenbrand <david@redhat.com> wrote: > Let's select the ASC before calling the function. This is a prepararion > to remove the ASC magic depending on the access mode from mmu_translate. > > There is currently no way to distinguish if we have code or data access. > For now, we were using code access, because especially when debugging with > the gdbstub, we want to read and disassemble what we single-step. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/helper.c | 5 +++++ > 1 file changed, 5 insertions(+) > Let's go with that for now. Reviewed-by: Cornelia Huck <cohuck@redhat.com>
diff --git a/target/s390x/helper.c b/target/s390x/helper.c index 13ae9909ad..c5fb8966b6 100644 --- a/target/s390x/helper.c +++ b/target/s390x/helper.c @@ -58,6 +58,11 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cs, vaddr vaddr) vaddr &= 0x7fffffff; } + /* We want to read the code (e.g., see what we are single-stepping).*/ + if (asc != PSW_ASC_HOME) { + asc = PSW_ASC_PRIMARY; + } + if (mmu_translate(env, vaddr, MMU_INST_FETCH, asc, &raddr, &prot, false)) { return -1; }
Let's select the ASC before calling the function. This is a prepararion to remove the ASC magic depending on the access mode from mmu_translate. There is currently no way to distinguish if we have code or data access. For now, we were using code access, because especially when debugging with the gdbstub, we want to read and disassemble what we single-step. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/helper.c | 5 +++++ 1 file changed, 5 insertions(+)