Re: [Qemu-devel] [PATCH v4] target-s390x: Implement stfl and stfle

[Thomas Huth <thuth@redhat.com>]

On 01.03.2017 20:30, Richard Henderson wrote:
> On 03/01/2017 07:00 PM, Thomas Huth wrote:
>>> Primarily, it does not raise an exception for error.
>>
>> Protection and page faults should be generated properly via
>> trigger_access_exception() there ... or did I miss something?
> 
> So why does s390_cpu_virt_mem_rw document that it returns non-zero if
> there was an error?

The non-zero return code is needed for the callers to decide whether to
continue or not (this is used in non-TCG code, too, so we need something
that is independent from TCG magic here). See the usage of
s390_cpu_virt_mem_read() / ..._write() in target/s390x/ioinst.c for example.

> I see you do raise an exception on the TCG path (via translate_pages),
> but not the KVM path.  That is very confusing.

The KVM_S390_MEM_OP ioctl can raise an exception, too. I konw, it's
confusing, but that ioctl was necessary since you need to hold a special
lock in KVM while walking the page tables.

>> AR = Access Register ... they are needed when the CPU runs in access
>> register mode (i.e. the AS PSW bits are 01). AFAIK we don't emulate this
>> in QEMU yet, but the KVM_S390_MEM_OP ioctl already supports it.
> 
> Hmm.  I guess I don't know how to read that then.

The access-register mode is described in the "Access-Register
Translation" sub-chapter in Chapter 5 ("Program Execution") of the POP.
Make sure to have some good painkillers ready first, though ... that's
IMHO really one of the ugliest parts of the architecture ;-)

>>> Your writes need to look a lot more like fast_memmove in mem_helper.c,
>>> except that of course only the destination needs translation.
>>
>> I still think that s390_cpu_virt_mem_write() should be fine here, too.
> 
> Ideally you'd interact with the TCG TLB in some way so that you didn't
> have to manage your own translation and your own exceptions.  That's
> what fast_memmove does.

Well, all the code from s390_cpu_virt_mem_rw() can also run with KVM -
in case the KVM_S390_MEM_OP is not available (which also has just been
introduced two years ago). So all the code that can be called by
s390_cpu_virt_mem_rw() has to work also without TCG.

But I agree, since the original problem here (TCG emulation of stfle) is
not related to KVM, it's maybe better to use a function a la
fast_memmove there instead.

>>> Of course, in practice we could reduce this to just one cpu_stl_data for
>>> STFL and one or two cpu_stq_data for STFLE.
>>
>> I think STFLE can store more than two 64-bit words, can't it?
> 
> Technically, yes.  But there are less than 128 bits defined.  Certainly
> much less than the 4k bits that Michal prepares for.

True. But maybe we should then also have an assert() or something
similar here in case the bits exceed the 128 limit one day...?

 Thomas