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

[David Hildenbrand]

On 12.08.19 15:58, Cornelia Huck wrote:
> On Mon, 12 Aug 2019 15:45:25 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 12.08.19 15:40, Cornelia Huck wrote:
>>> 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?  
>>
>> 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 ...).

-- 

Thanks,

David / dhildenb