Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()

[Cornelia Huck]

On Mon, 12 Aug 2019 09:52:56 +0200
David Hildenbrand <address@hidden> 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 <address@hidden>
> >> ---
> >>  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.