[Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file

Thomas Huth posted 1 patch 5 years, 1 month ago
Test asan passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1548956828-10210-1-git-send-email-thuth@redhat.com
Maintainers: Collin Walling <walling@linux.ibm.com>, Richard Henderson <rth@twiddle.net>, Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>
hw/s390x/s390-pci-stub.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Thomas Huth 5 years, 1 month ago
The arguments of the stub functions to not match the real implementation
(and the prototypes in the header) anymore, so if you try to compile s390x
without CONFIG_PCI, the build currently fails.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/s390-pci-stub.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
index ad4c5a7..145759e 100644
--- a/hw/s390x/s390-pci-stub.c
+++ b/hw/s390x/s390-pci-stub.c
@@ -29,38 +29,40 @@ void s390_pci_sclp_deconfigure(SCCB *sccb)
 }
 
 /* target/s390x/kvm.c */
-int clp_service_call(S390CPU *cpu, uint8_t r2)
+int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
 {
     return -1;
 }
 
-int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
+int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
 {
     return -1;
 }
 
-int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
+int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
 {
     return -1;
 }
 
-int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)
+int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
+                         uintptr_t ra)
 {
     return -1;
 }
 
-int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
+int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
 {
     return -1;
 }
 
 int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
-                        uint8_t ar)
+                        uint8_t ar, uintptr_t ra)
 {
     return -1;
 }
 
-int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)
+int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
+                        uintptr_t ra)
 {
     return -1;
 }
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Cornelia Huck 5 years, 1 month ago
On Thu, 31 Jan 2019 18:47:08 +0100
Thomas Huth <thuth@redhat.com> wrote:

> The arguments of the stub functions to not match the real implementation

s/to/do/

> (and the prototypes in the header) anymore, so if you try to compile s390x
> without CONFIG_PCI, the build currently fails.
> 

Fixes: 468a93898a97 ("s390x/pci: pass the retaddr to all PCI instructions")

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/s390-pci-stub.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

This file seems to be in danger of bitrot. Do you think it'll be easier
to test rarely used configs like that after we switch to Kconfig?

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Thomas Huth 5 years, 1 month ago
On 2019-01-31 18:56, Cornelia Huck wrote:
> On Thu, 31 Jan 2019 18:47:08 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The arguments of the stub functions to not match the real implementation
> 
> s/to/do/

D'oh!

>> (and the prototypes in the header) anymore, so if you try to compile s390x
>> without CONFIG_PCI, the build currently fails.
>>
> 
> Fixes: 468a93898a97 ("s390x/pci: pass the retaddr to all PCI instructions")
> 
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/s390x/s390-pci-stub.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> This file seems to be in danger of bitrot. Do you think it'll be easier
> to test rarely used configs like that after we switch to Kconfig?

I hope so, yes. There will be a new --without-default-devices options
for "configure" (which matches "make allnoconfig" from the kernel) - if
we do it right in the Kconfig file for s390x, it should be possible to
catch this problem with that option.

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Paolo Bonzini 5 years, 1 month ago
On 31/01/19 19:00, Thomas Huth wrote:
>>> (and the prototypes in the header) anymore, so if you try to compile s390x
>>> without CONFIG_PCI, the build currently fails.
>>>
>> Fixes: 468a93898a97 ("s390x/pci: pass the retaddr to all PCI instructions")
>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  hw/s390x/s390-pci-stub.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>> This file seems to be in danger of bitrot. Do you think it'll be easier
>> to test rarely used configs like that after we switch to Kconfig?
> I hope so, yes. There will be a new --without-default-devices options
> for "configure" (which matches "make allnoconfig" from the kernel) - if
> we do it right in the Kconfig file for s390x, it should be possible to
> catch this problem with that option.

Yes, it will be in .travis.yml too.

Right now there is a "select PCI" in the hw/s390x/Kconfig file, but
probably it's best to add a config S390_ZPCI with "default y if
S390_CCW_VIRTIO" and "select PCI" in it.  Not a blocker, but I can
integrate it if you send me a fixup patch.

Paolo

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Thomas Huth 5 years, 1 month ago
On 2019-01-31 19:08, Paolo Bonzini wrote:
> On 31/01/19 19:00, Thomas Huth wrote:
>>>> (and the prototypes in the header) anymore, so if you try to compile s390x
>>>> without CONFIG_PCI, the build currently fails.
>>>>
>>> Fixes: 468a93898a97 ("s390x/pci: pass the retaddr to all PCI instructions")
>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  hw/s390x/s390-pci-stub.c | 16 +++++++++-------
>>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>> This file seems to be in danger of bitrot. Do you think it'll be easier
>>> to test rarely used configs like that after we switch to Kconfig?
>> I hope so, yes. There will be a new --without-default-devices options
>> for "configure" (which matches "make allnoconfig" from the kernel) - if
>> we do it right in the Kconfig file for s390x, it should be possible to
>> catch this problem with that option.
> 
> Yes, it will be in .travis.yml too.
> 
> Right now there is a "select PCI" in the hw/s390x/Kconfig file, but
> probably it's best to add a config S390_ZPCI with "default y if
> S390_CCW_VIRTIO" and "select PCI" in it.  Not a blocker, but I can
> integrate it if you send me a fixup patch.

Yes, that's what I had in mind, too. I'll send a fixup patch...

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Thomas Huth 5 years, 1 month ago
On 2019-02-01 07:14, Thomas Huth wrote:
> On 2019-01-31 19:08, Paolo Bonzini wrote:
>> On 31/01/19 19:00, Thomas Huth wrote:
>>>>> (and the prototypes in the header) anymore, so if you try to compile s390x
>>>>> without CONFIG_PCI, the build currently fails.
>>>>>
>>>> Fixes: 468a93898a97 ("s390x/pci: pass the retaddr to all PCI instructions")
>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  hw/s390x/s390-pci-stub.c | 16 +++++++++-------
>>>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>> This file seems to be in danger of bitrot. Do you think it'll be easier
>>>> to test rarely used configs like that after we switch to Kconfig?
>>> I hope so, yes. There will be a new --without-default-devices options
>>> for "configure" (which matches "make allnoconfig" from the kernel) - if
>>> we do it right in the Kconfig file for s390x, it should be possible to
>>> catch this problem with that option.
>>
>> Yes, it will be in .travis.yml too.
>>
>> Right now there is a "select PCI" in the hw/s390x/Kconfig file, but
>> probably it's best to add a config S390_ZPCI with "default y if
>> S390_CCW_VIRTIO" and "select PCI" in it.  Not a blocker, but I can
>> integrate it if you send me a fixup patch.
> 
> Yes, that's what I had in mind, too. I'll send a fixup patch...

Actually, disabling the s390-pci code works from a compiling+linking
point of view, but when you then try to start the machine, it fails:

$ s390x-softmmu/qemu-system-s390x -nographic
qemu-system-s390x: Unknown device 's390-pcihost' for default sysbus
Aborted (core dumped)

IIRC we originally wanted to make the "s390-pcihost" device really
optional, but then decided not to do it since it would break migration.
So the status of the "PCI disablement" got stuck somewhere inbetween,
where we've created the switch for the Makefile and the stub file, but
never really got it to a point where it could really really be disabled.

So I see two options now:

1) Finally really make the device optional, at least for new machine
types, so we can really disable CONFIG_PCI and get a working executable.

2) Scratch the idea completely to make this optional, always link the
s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
s390-pci-stub.c file.

I assume options 2 is preferred, since we likely rather want to move
into the PCI direction in the long run, instead of ignoring it...

Other opinions?

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Cornelia Huck 5 years, 1 month ago
On Fri, 1 Feb 2019 07:46:40 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2019-02-01 07:14, Thomas Huth wrote:
> > On 2019-01-31 19:08, Paolo Bonzini wrote:  
> >> On 31/01/19 19:00, Thomas Huth wrote:  
> >>>>> (and the prototypes in the header) anymore, so if you try to compile s390x
> >>>>> without CONFIG_PCI, the build currently fails.
> >>>>>  
> >>>> Fixes: 468a93898a97 ("s390x/pci: pass the retaddr to all PCI instructions")
> >>>>  
> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>>> ---
> >>>>>  hw/s390x/s390-pci-stub.c | 16 +++++++++-------
> >>>>>  1 file changed, 9 insertions(+), 7 deletions(-)  
> >>>> This file seems to be in danger of bitrot. Do you think it'll be easier
> >>>> to test rarely used configs like that after we switch to Kconfig?  
> >>> I hope so, yes. There will be a new --without-default-devices options
> >>> for "configure" (which matches "make allnoconfig" from the kernel) - if
> >>> we do it right in the Kconfig file for s390x, it should be possible to
> >>> catch this problem with that option.  
> >>
> >> Yes, it will be in .travis.yml too.
> >>
> >> Right now there is a "select PCI" in the hw/s390x/Kconfig file, but
> >> probably it's best to add a config S390_ZPCI with "default y if
> >> S390_CCW_VIRTIO" and "select PCI" in it.  Not a blocker, but I can
> >> integrate it if you send me a fixup patch.  
> > 
> > Yes, that's what I had in mind, too. I'll send a fixup patch...  
> 
> Actually, disabling the s390-pci code works from a compiling+linking
> point of view, but when you then try to start the machine, it fails:
> 
> $ s390x-softmmu/qemu-system-s390x -nographic
> qemu-system-s390x: Unknown device 's390-pcihost' for default sysbus
> Aborted (core dumped)
> 
> IIRC we originally wanted to make the "s390-pcihost" device really
> optional, but then decided not to do it since it would break migration.
> So the status of the "PCI disablement" got stuck somewhere inbetween,
> where we've created the switch for the Makefile and the stub file, but
> never really got it to a point where it could really really be disabled.

Well you can disable it, but it's not usable ;)

But yes, that annoying pcihost device needed for backwards compat got
into the way.

> 
> So I see two options now:
> 
> 1) Finally really make the device optional, at least for new machine
> types, so we can really disable CONFIG_PCI and get a working executable.
> 
> 2) Scratch the idea completely to make this optional, always link the
> s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
> s390-pci-stub.c file.
> 
> I assume options 2 is preferred, since we likely rather want to move
> into the PCI direction in the long run, instead of ignoring it...

I think both options are viable, but option 1 is of course more work.
The win there is that we could disable an entire subsystem.

I guess that the basic questions are: How important is it that
subsystems can be compiled out, and do we see a use case for a pci-less
s390 machine in the future? We really don't want to spend much time on
something of dubious use...

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Cornelia Huck 5 years, 1 month ago
On Fri, 1 Feb 2019 09:23:56 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 1 Feb 2019 07:46:40 +0100
> Thomas Huth <thuth@redhat.com> wrote:

> > So I see two options now:
> > 
> > 1) Finally really make the device optional, at least for new machine
> > types, so we can really disable CONFIG_PCI and get a working executable.
> > 
> > 2) Scratch the idea completely to make this optional, always link the
> > s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
> > s390-pci-stub.c file.
> > 
> > I assume options 2 is preferred, since we likely rather want to move
> > into the PCI direction in the long run, instead of ignoring it...  
> 
> I think both options are viable, but option 1 is of course more work.
> The win there is that we could disable an entire subsystem.
> 
> I guess that the basic questions are: How important is it that
> subsystems can be compiled out, and do we see a use case for a pci-less
> s390 machine in the future? We really don't want to spend much time on
> something of dubious use...

Any thoughts on this?

I'm currently tending towards option 2 (and can cook up a patch for
that). Unless someone is already working on option 1 :)

Of course, I can also apply the original patch, but the end result is
not very useful...

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Thomas Huth 5 years, 1 month ago
On 2019-02-11 11:48, Cornelia Huck wrote:
> On Fri, 1 Feb 2019 09:23:56 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Fri, 1 Feb 2019 07:46:40 +0100
>> Thomas Huth <thuth@redhat.com> wrote:
> 
>>> So I see two options now:
>>>
>>> 1) Finally really make the device optional, at least for new machine
>>> types, so we can really disable CONFIG_PCI and get a working executable.
>>>
>>> 2) Scratch the idea completely to make this optional, always link the
>>> s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
>>> s390-pci-stub.c file.
>>>
>>> I assume options 2 is preferred, since we likely rather want to move
>>> into the PCI direction in the long run, instead of ignoring it...  
>>
>> I think both options are viable, but option 1 is of course more work.
>> The win there is that we could disable an entire subsystem.
>>
>> I guess that the basic questions are: How important is it that
>> subsystems can be compiled out, and do we see a use case for a pci-less
>> s390 machine in the future? We really don't want to spend much time on
>> something of dubious use...
> 
> Any thoughts on this?
> 
> I'm currently tending towards option 2 (and can cook up a patch for
> that). Unless someone is already working on option 1 :)

Since nobody currently has a need to completely disable PCI, I think we
should go with option 2.

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Cornelia Huck 5 years, 1 month ago
On Mon, 11 Feb 2019 11:54:45 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2019-02-11 11:48, Cornelia Huck wrote:
> > On Fri, 1 Feb 2019 09:23:56 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> >> On Fri, 1 Feb 2019 07:46:40 +0100
> >> Thomas Huth <thuth@redhat.com> wrote:  
> >   
> >>> So I see two options now:
> >>>
> >>> 1) Finally really make the device optional, at least for new machine
> >>> types, so we can really disable CONFIG_PCI and get a working executable.
> >>>
> >>> 2) Scratch the idea completely to make this optional, always link the
> >>> s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
> >>> s390-pci-stub.c file.
> >>>
> >>> I assume options 2 is preferred, since we likely rather want to move
> >>> into the PCI direction in the long run, instead of ignoring it...    
> >>
> >> I think both options are viable, but option 1 is of course more work.
> >> The win there is that we could disable an entire subsystem.
> >>
> >> I guess that the basic questions are: How important is it that
> >> subsystems can be compiled out, and do we see a use case for a pci-less
> >> s390 machine in the future? We really don't want to spend much time on
> >> something of dubious use...  
> > 
> > Any thoughts on this?
> > 
> > I'm currently tending towards option 2 (and can cook up a patch for
> > that). Unless someone is already working on option 1 :)  
> 
> Since nobody currently has a need to completely disable PCI, I think we
> should go with option 2.

Hm... I'm wondering if we also should move S390_FEAT_ZPCI from the max
cpu model to the qemu cpu model (is there any reason not to turn it on
by default in tcg?)

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Thomas Huth 5 years, 1 month ago
On 2019-02-11 12:04, Cornelia Huck wrote:
> On Mon, 11 Feb 2019 11:54:45 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2019-02-11 11:48, Cornelia Huck wrote:
>>> On Fri, 1 Feb 2019 09:23:56 +0100
>>> Cornelia Huck <cohuck@redhat.com> wrote:
>>>   
>>>> On Fri, 1 Feb 2019 07:46:40 +0100
>>>> Thomas Huth <thuth@redhat.com> wrote:  
>>>   
>>>>> So I see two options now:
>>>>>
>>>>> 1) Finally really make the device optional, at least for new machine
>>>>> types, so we can really disable CONFIG_PCI and get a working executable.
>>>>>
>>>>> 2) Scratch the idea completely to make this optional, always link the
>>>>> s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
>>>>> s390-pci-stub.c file.
>>>>>
>>>>> I assume options 2 is preferred, since we likely rather want to move
>>>>> into the PCI direction in the long run, instead of ignoring it...    
>>>>
>>>> I think both options are viable, but option 1 is of course more work.
>>>> The win there is that we could disable an entire subsystem.
>>>>
>>>> I guess that the basic questions are: How important is it that
>>>> subsystems can be compiled out, and do we see a use case for a pci-less
>>>> s390 machine in the future? We really don't want to spend much time on
>>>> something of dubious use...  
>>>
>>> Any thoughts on this?
>>>
>>> I'm currently tending towards option 2 (and can cook up a patch for
>>> that). Unless someone is already working on option 1 :)  
>>
>> Since nobody currently has a need to completely disable PCI, I think we
>> should go with option 2.
> 
> Hm... I'm wondering if we also should move S390_FEAT_ZPCI from the max
> cpu model to the qemu cpu model (is there any reason not to turn it on
> by default in tcg?)

Migration compatibility? Wouldn't that cause problems when migrating
back to older versions of QEMU?

 Thomas

Re: [Qemu-devel] [PATCH] hw/s390x: Fix the function arguments in the pci stub file
Posted by Cornelia Huck 5 years, 1 month ago
On Mon, 11 Feb 2019 12:17:26 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2019-02-11 12:04, Cornelia Huck wrote:
> > On Mon, 11 Feb 2019 11:54:45 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 2019-02-11 11:48, Cornelia Huck wrote:  
> >>> On Fri, 1 Feb 2019 09:23:56 +0100
> >>> Cornelia Huck <cohuck@redhat.com> wrote:
> >>>     
> >>>> On Fri, 1 Feb 2019 07:46:40 +0100
> >>>> Thomas Huth <thuth@redhat.com> wrote:    
> >>>     
> >>>>> So I see two options now:
> >>>>>
> >>>>> 1) Finally really make the device optional, at least for new machine
> >>>>> types, so we can really disable CONFIG_PCI and get a working executable.
> >>>>>
> >>>>> 2) Scratch the idea completely to make this optional, always link the
> >>>>> s390-pci-bus.o and s390-pci-inst.o files unconditionally, and remove the
> >>>>> s390-pci-stub.c file.
> >>>>>
> >>>>> I assume options 2 is preferred, since we likely rather want to move
> >>>>> into the PCI direction in the long run, instead of ignoring it...      
> >>>>
> >>>> I think both options are viable, but option 1 is of course more work.
> >>>> The win there is that we could disable an entire subsystem.
> >>>>
> >>>> I guess that the basic questions are: How important is it that
> >>>> subsystems can be compiled out, and do we see a use case for a pci-less
> >>>> s390 machine in the future? We really don't want to spend much time on
> >>>> something of dubious use...    
> >>>
> >>> Any thoughts on this?
> >>>
> >>> I'm currently tending towards option 2 (and can cook up a patch for
> >>> that). Unless someone is already working on option 1 :)    
> >>
> >> Since nobody currently has a need to completely disable PCI, I think we
> >> should go with option 2.  
> > 
> > Hm... I'm wondering if we also should move S390_FEAT_ZPCI from the max
> > cpu model to the qemu cpu model (is there any reason not to turn it on
> > by default in tcg?)  
> 
> Migration compatibility? Wouldn't that cause problems when migrating
> back to older versions of QEMU?

Not sure. But we can still do that in a follow-on patch, if wanted.