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

From: David Hildenbrand <david@redhat.com>
To: Thomas Huth <thuth@redhat.com>, qemu-devel@nongnu.org
Cc: Janosch Frank <frankja@linux.ibm.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Ilya Leoshkevich <iii@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	qemu-s390x@nongnu.org, Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH-for-4.2 v1 1/9] s390x/mmu: Better ASC selection in s390_cpu_get_phys_page_debug()
Date: Mon, 12 Aug 2019 09:52:56 +0200	[thread overview]
Message-ID: <bbf905b3-6f32-c478-4e6e-81c341f5601a@redhat.com> (raw)
In-Reply-To: <e68f8298-8946-37f1-2e65-afa73a45604e@redhat.com>

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.

-- 

Thanks,

David / dhildenb