[Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD

David Hildenbrand posted 5 patches 8 years, 2 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by David Hildenbrand 8 years, 2 months ago
We somehow missed that, new kernels require it.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.h      | 1 +
 target/s390x/insn-data.def | 1 +
 target/s390x/misc_helper.c | 9 +++++++++
 target/s390x/translate.c   | 8 ++++++++
 4 files changed, 19 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 0281c286b8..2ce57edc14 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -166,6 +166,7 @@ DEF_HELPER_3(msch, void, env, i64, i64)
 DEF_HELPER_2(rchp, void, env, i64)
 DEF_HELPER_2(rsch, void, env, i64)
 DEF_HELPER_3(ssch, void, env, i64, i64)
+DEF_HELPER_2(stcrw, void, env, i64)
 DEF_HELPER_3(stsch, void, env, i64, i64)
 DEF_HELPER_3(tsch, void, env, i64, i64)
 DEF_HELPER_2(chsc, void, env, i64)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 8c2541f545..43ab1963c8 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1055,6 +1055,7 @@
     C(0xb23b, RCHP,    S,     Z,   0, 0, 0, 0, rchp, 0)
     C(0xb238, RSCH,    S,     Z,   0, 0, 0, 0, rsch, 0)
     C(0xb233, SSCH,    S,     Z,   0, insn, 0, 0, ssch, 0)
+    C(0xb239, STCRW,   S,     Z,   0, insn, 0, 0, stcrw, 0)
     C(0xb234, STSCH,   S,     Z,   0, insn, 0, 0, stsch, 0)
     C(0xb235, TSCH,    S,     Z,   0, insn, 0, 0, tsch, 0)
     /* ??? Not listed in PoO ninth edition, but there's a linux driver that
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 2c6ab329fb..55e78c56d2 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -385,6 +385,15 @@ void HELPER(ssch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
     qemu_mutex_unlock_iothread();
 }
 
+void HELPER(stcrw)(CPUS390XState *env, uint64_t inst)
+{
+    S390CPU *cpu = s390_env_get_cpu(env);
+
+    qemu_mutex_lock_iothread();
+    ioinst_handle_stcrw(cpu, inst >> 16, GETPC());
+    qemu_mutex_unlock_iothread();
+}
+
 void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 8da8610839..5e051fdd03 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4071,6 +4071,14 @@ static ExitStatus op_stsch(DisasContext *s, DisasOps *o)
     return NO_EXIT;
 }
 
+static ExitStatus op_stcrw(DisasContext *s, DisasOps *o)
+{
+    check_privileged(s);
+    gen_helper_stcrw(cpu_env, o->in2);
+    set_cc_static(s);
+    return NO_EXIT;
+}
+
 static ExitStatus op_tsch(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-- 
2.14.3


Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by Cornelia Huck 8 years, 2 months ago
On Mon,  4 Dec 2017 13:55:05 +0100
David Hildenbrand <david@redhat.com> wrote:

> We somehow missed that, new kernels require it.

Why _new_ kernels? I think we have unconditionally issued stcrw since
back in 2.2?

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.h      | 1 +
>  target/s390x/insn-data.def | 1 +
>  target/s390x/misc_helper.c | 9 +++++++++
>  target/s390x/translate.c   | 8 ++++++++
>  4 files changed, 19 insertions(+)

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by David Hildenbrand 8 years, 2 months ago
On 04.12.2017 18:22, Cornelia Huck wrote:
> On Mon,  4 Dec 2017 13:55:05 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> We somehow missed that, new kernels require it.
> 
> Why _new_ kernels? I think we have unconditionally issued stcrw since
> back in 2.2?

Okay, the problem is then rather related to my setup. No ccw devices ->
no stcrw.

How could we ever add ccw devices to a TCG guest then?

> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.h      | 1 +
>>  target/s390x/insn-data.def | 1 +
>>  target/s390x/misc_helper.c | 9 +++++++++
>>  target/s390x/translate.c   | 8 ++++++++
>>  4 files changed, 19 insertions(+)


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by Cornelia Huck 8 years, 2 months ago
On Mon, 4 Dec 2017 18:34:36 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.12.2017 18:22, Cornelia Huck wrote:
> > On Mon,  4 Dec 2017 13:55:05 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> We somehow missed that, new kernels require it.  
> > 
> > Why _new_ kernels? I think we have unconditionally issued stcrw since
> > back in 2.2?  
> 
> Okay, the problem is then rather related to my setup. No ccw devices ->
> no stcrw.
> 
> How could we ever add ccw devices to a TCG guest then?

Hotplug never worked before your patches. Coldplug does not create
machine checks and thus does not trigger the guest to do STCRW. (Only
STSCH, in keeping with "all channel I/O acronyms look the same".)

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by David Hildenbrand 8 years, 2 months ago
On 04.12.2017 18:53, Cornelia Huck wrote:
> On Mon, 4 Dec 2017 18:34:36 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.12.2017 18:22, Cornelia Huck wrote:
>>> On Mon,  4 Dec 2017 13:55:05 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> We somehow missed that, new kernels require it.  
>>>
>>> Why _new_ kernels? I think we have unconditionally issued stcrw since
>>> back in 2.2?  
>>
>> Okay, the problem is then rather related to my setup. No ccw devices ->
>> no stcrw.
>>
>> How could we ever add ccw devices to a TCG guest then?
> 
> Hotplug never worked before your patches. Coldplug does not create
> machine checks and thus does not trigger the guest to do STCRW. (Only
> STSCH, in keeping with "all channel I/O acronyms look the same".)
> 

But booting Fedora 26/27 without any involved hotplugs (therefore
machine checks) triggers a STCRW.

That's how I originally found it. (I started playing with machine checks
after I had fedora 26/27 running)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by Cornelia Huck 8 years, 2 months ago
On Mon, 4 Dec 2017 18:56:00 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.12.2017 18:53, Cornelia Huck wrote:
> > On Mon, 4 Dec 2017 18:34:36 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 04.12.2017 18:22, Cornelia Huck wrote:  
> >>> On Mon,  4 Dec 2017 13:55:05 +0100
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> We somehow missed that, new kernels require it.    
> >>>
> >>> Why _new_ kernels? I think we have unconditionally issued stcrw since
> >>> back in 2.2?    
> >>
> >> Okay, the problem is then rather related to my setup. No ccw devices ->
> >> no stcrw.
> >>
> >> How could we ever add ccw devices to a TCG guest then?  
> > 
> > Hotplug never worked before your patches. Coldplug does not create
> > machine checks and thus does not trigger the guest to do STCRW. (Only
> > STSCH, in keeping with "all channel I/O acronyms look the same".)
> >   
> 
> But booting Fedora 26/27 without any involved hotplugs (therefore
> machine checks) triggers a STCRW.
> 
> That's how I originally found it. (I started playing with machine checks
> after I had fedora 26/27 running)
> 

Confused. Do you have a backtrace? (Guest/host)

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by David Hildenbrand 8 years, 2 months ago
On 04.12.2017 18:58, Cornelia Huck wrote:
> On Mon, 4 Dec 2017 18:56:00 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.12.2017 18:53, Cornelia Huck wrote:
>>> On Mon, 4 Dec 2017 18:34:36 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 04.12.2017 18:22, Cornelia Huck wrote:  
>>>>> On Mon,  4 Dec 2017 13:55:05 +0100
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>     
>>>>>> We somehow missed that, new kernels require it.    
>>>>>
>>>>> Why _new_ kernels? I think we have unconditionally issued stcrw since
>>>>> back in 2.2?    
>>>>
>>>> Okay, the problem is then rather related to my setup. No ccw devices ->
>>>> no stcrw.
>>>>
>>>> How could we ever add ccw devices to a TCG guest then?  
>>>
>>> Hotplug never worked before your patches. Coldplug does not create
>>> machine checks and thus does not trigger the guest to do STCRW. (Only
>>> STSCH, in keeping with "all channel I/O acronyms look the same".)
>>>   
>>
>> But booting Fedora 26/27 without any involved hotplugs (therefore
>> machine checks) triggers a STCRW.
>>
>> That's how I originally found it. (I started playing with machine checks
>> after I had fedora 26/27 running)
>>
> 
> Confused. Do you have a backtrace? (Guest/host)
> 

Unfortunately not. Strange, I just tried to reproduce but can't trigger
it. Maybe aside effect from the other BUGs I have been fixing :)

So I'll just rephrase this to "CRW machine check handling requires STCRW."

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by Cornelia Huck 8 years, 2 months ago
On Mon, 4 Dec 2017 19:37:26 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.12.2017 18:58, Cornelia Huck wrote:
> > On Mon, 4 Dec 2017 18:56:00 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 04.12.2017 18:53, Cornelia Huck wrote:  
> >>> On Mon, 4 Dec 2017 18:34:36 +0100
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>     
> >>>> On 04.12.2017 18:22, Cornelia Huck wrote:    
> >>>>> On Mon,  4 Dec 2017 13:55:05 +0100
> >>>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>>       
> >>>>>> We somehow missed that, new kernels require it.      
> >>>>>
> >>>>> Why _new_ kernels? I think we have unconditionally issued stcrw since
> >>>>> back in 2.2?      
> >>>>
> >>>> Okay, the problem is then rather related to my setup. No ccw devices ->
> >>>> no stcrw.
> >>>>
> >>>> How could we ever add ccw devices to a TCG guest then?    
> >>>
> >>> Hotplug never worked before your patches. Coldplug does not create
> >>> machine checks and thus does not trigger the guest to do STCRW. (Only
> >>> STSCH, in keeping with "all channel I/O acronyms look the same".)
> >>>     
> >>
> >> But booting Fedora 26/27 without any involved hotplugs (therefore
> >> machine checks) triggers a STCRW.
> >>
> >> That's how I originally found it. (I started playing with machine checks
> >> after I had fedora 26/27 running)
> >>  
> > 
> > Confused. Do you have a backtrace? (Guest/host)
> >   
> 
> Unfortunately not. Strange, I just tried to reproduce but can't trigger
> it. Maybe aside effect from the other BUGs I have been fixing :)
> 
> So I'll just rephrase this to "CRW machine check handling requires STCRW."
> 

Yeah, let's do that.

Re: [Qemu-devel] [PATCH v1 for-2.12 5/5] s390x/tcg: wire up STORE CHANNEL REPORT WORD
Posted by Thomas Huth 8 years, 2 months ago
On 04.12.2017 13:55, David Hildenbrand wrote:
> We somehow missed that, new kernels require it.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.h      | 1 +
>  target/s390x/insn-data.def | 1 +
>  target/s390x/misc_helper.c | 9 +++++++++
>  target/s390x/translate.c   | 8 ++++++++
>  4 files changed, 19 insertions(+)

IANATE again, but the patch looks right to me, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>