| Message ID | 20190708125433.16927-2-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: fallthrough annotations | expand |
On 08.07.19 14:54, Cornelia Huck wrote: > According to the comment, the bits are supposed to accumulate. > > Reported-by: Stefan Weil <sw@weilnetz.de> > Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") > Signed-off-by: Cornelia Huck <cohuck@redhat.com> This patch does not change behaviour, so it is certainly not wrong. So lets have a look at if the bug report was actually a real bug or just a missing annotation. > --- > hw/s390x/s390-pci-inst.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 61f30b8e55d2..00235148bed7 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, > * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ > case ZPCI_FS_ERROR: > fib.fc |= 0x20; > + /* fallthrough */ This is correct, in case of an error we are also blocked. > case ZPCI_FS_BLOCKED: > fib.fc |= 0x40; > + /* fallthrough */ I think this is also correct, but it would be good if Collin could verify. > case ZPCI_FS_ENABLED: > fib.fc |= 0x80; > if (pbdev->iommu->enabled) { >
On 7/8/19 9:23 AM, Christian Borntraeger wrote: > > > On 08.07.19 14:54, Cornelia Huck wrote: >> According to the comment, the bits are supposed to accumulate. >> >> Reported-by: Stefan Weil <sw@weilnetz.de> >> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > This patch does not change behaviour, so it is certainly not wrong. > > So lets have a look at if the bug report was actually a real bug or > just a missing annotation. > >> --- >> hw/s390x/s390-pci-inst.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index 61f30b8e55d2..00235148bed7 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, >> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ >> case ZPCI_FS_ERROR: >> fib.fc |= 0x20; >> + /* fallthrough */ > > This is correct, in case of an error we are also blocked. > Agreed. This is definitely correct based on our architecture. >> case ZPCI_FS_BLOCKED: >> fib.fc |= 0x40; >> + /* fallthrough */ > > I think this is also correct, but it would be good if Collin could verify. > I failed to find anything to support setting the function control enabled bit when the function state is in error / blocked. I'm assuming this might be some QEMU hack to get things working? I'll have to dive further to understand why this was done this way, as it doesn't align with how the s390x architecture is documented. It's confusing. Functionally, this doesn't change anything... so I'll at least give it an ACK for now and investigate this further. >> case ZPCI_FS_ENABLED: >> fib.fc |= 0x80; >> if (pbdev->iommu->enabled) { >> > > Acked-by: Collin Walling <walling@linux.ibm.com> Side note: is there somewhere that I could access this bug report? :)
On Tue, 9 Jul 2019 18:55:34 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 7/8/19 9:23 AM, Christian Borntraeger wrote: > > > > > > On 08.07.19 14:54, Cornelia Huck wrote: > >> According to the comment, the bits are supposed to accumulate. > >> > >> Reported-by: Stefan Weil <sw@weilnetz.de> > >> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") > >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > > > This patch does not change behaviour, so it is certainly not wrong. > > > > So lets have a look at if the bug report was actually a real bug or > > just a missing annotation. > > > >> --- > >> hw/s390x/s390-pci-inst.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > >> index 61f30b8e55d2..00235148bed7 100644 > >> --- a/hw/s390x/s390-pci-inst.c > >> +++ b/hw/s390x/s390-pci-inst.c > >> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, > >> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ > >> case ZPCI_FS_ERROR: > >> fib.fc |= 0x20; > >> + /* fallthrough */ > > > > This is correct, in case of an error we are also blocked. > > > > Agreed. This is definitely correct based on our architecture. > > >> case ZPCI_FS_BLOCKED: > >> fib.fc |= 0x40; > >> + /* fallthrough */ > > > > I think this is also correct, but it would be good if Collin could verify. > > > > I failed to find anything to support setting the function control > enabled bit when the function state is in error / blocked. I'm > assuming this might be some QEMU hack to get things working? I'll have > to dive further to understand why this was done this way, as it doesn't > align with how the s390x architecture is documented. It's confusing. Might this also be a real issue? Not matching the architecture is not a good sign... > > Functionally, this doesn't change anything... so I'll at least give it > an ACK for now and investigate this further. I prefer to hold off on this patch for now, unless we're really sure that the code does not have a problem here. Actually documenting something that might be wrong does not sound like the right thing to do :/ > > >> case ZPCI_FS_ENABLED: > >> fib.fc |= 0x80; > >> if (pbdev->iommu->enabled) { > >> > > > > > > Acked-by: Collin Walling <walling@linux.ibm.com> > > Side note: is there somewhere that I could access this bug report? :) It's the build log with the extra warnings in https://qemu.weilnetz.de/results/build-20190708.txt (referenced in <591d71a5-5b10-ab22-4751-01da8613d84c@weilnetz.de> on qemu-devel).
On Wed, 10 Jul 2019 10:20:41 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, 9 Jul 2019 18:55:34 -0400 > Collin Walling <walling@linux.ibm.com> wrote: > > > On 7/8/19 9:23 AM, Christian Borntraeger wrote: > > > > > > > > > On 08.07.19 14:54, Cornelia Huck wrote: > > >> According to the comment, the bits are supposed to accumulate. > > >> > > >> Reported-by: Stefan Weil <sw@weilnetz.de> > > >> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") > > >> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > > > > > This patch does not change behaviour, so it is certainly not wrong. > > > > > > So lets have a look at if the bug report was actually a real bug or > > > just a missing annotation. > > > > > >> --- > > >> hw/s390x/s390-pci-inst.c | 2 ++ > > >> 1 file changed, 2 insertions(+) > > >> > > >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > > >> index 61f30b8e55d2..00235148bed7 100644 > > >> --- a/hw/s390x/s390-pci-inst.c > > >> +++ b/hw/s390x/s390-pci-inst.c > > >> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, > > >> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ > > >> case ZPCI_FS_ERROR: > > >> fib.fc |= 0x20; > > >> + /* fallthrough */ > > > > > > This is correct, in case of an error we are also blocked. > > > > > > > Agreed. This is definitely correct based on our architecture. > > > > >> case ZPCI_FS_BLOCKED: > > >> fib.fc |= 0x40; > > >> + /* fallthrough */ > > > > > > I think this is also correct, but it would be good if Collin could verify. > > > > > > > I failed to find anything to support setting the function control > > enabled bit when the function state is in error / blocked. I'm > > assuming this might be some QEMU hack to get things working? I'll have > > to dive further to understand why this was done this way, as it doesn't > > align with how the s390x architecture is documented. It's confusing. > > Might this also be a real issue? Not matching the architecture is not a > good sign... Friendly ping. If we still want to have this patch or a fix in 4.1, we need to find out soon... > > > > > Functionally, this doesn't change anything... so I'll at least give it > > an ACK for now and investigate this further. > > I prefer to hold off on this patch for now, unless we're really sure > that the code does not have a problem here. Actually documenting > something that might be wrong does not sound like the right thing to > do :/ > > > > > >> case ZPCI_FS_ENABLED: > > >> fib.fc |= 0x80; > > >> if (pbdev->iommu->enabled) { > > >> > > > > > > > > > > Acked-by: Collin Walling <walling@linux.ibm.com> > > > > Side note: is there somewhere that I could access this bug report? :) > > It's the build log with the extra warnings in > https://qemu.weilnetz.de/results/build-20190708.txt (referenced in > <591d71a5-5b10-ab22-4751-01da8613d84c@weilnetz.de> on qemu-devel).
On 7/16/19 11:20 AM, Cornelia Huck wrote: > On Wed, 10 Jul 2019 10:20:41 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Tue, 9 Jul 2019 18:55:34 -0400 >> Collin Walling <walling@linux.ibm.com> wrote: >> >>> On 7/8/19 9:23 AM, Christian Borntraeger wrote: >>>> >>>> >>>> On 08.07.19 14:54, Cornelia Huck wrote: >>>>> According to the comment, the bits are supposed to accumulate. >>>>> >>>>> Reported-by: Stefan Weil <sw@weilnetz.de> >>>>> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") >>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>> >>>> This patch does not change behaviour, so it is certainly not wrong. >>>> >>>> So lets have a look at if the bug report was actually a real bug or >>>> just a missing annotation. >>>> >>>>> --- >>>>> hw/s390x/s390-pci-inst.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >>>>> index 61f30b8e55d2..00235148bed7 100644 >>>>> --- a/hw/s390x/s390-pci-inst.c >>>>> +++ b/hw/s390x/s390-pci-inst.c >>>>> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, >>>>> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ >>>>> case ZPCI_FS_ERROR: >>>>> fib.fc |= 0x20; >>>>> + /* fallthrough */ >>>> >>>> This is correct, in case of an error we are also blocked. >>>> >>> >>> Agreed. This is definitely correct based on our architecture. >>> >>>>> case ZPCI_FS_BLOCKED: >>>>> fib.fc |= 0x40; >>>>> + /* fallthrough */ >>>> >>>> I think this is also correct, but it would be good if Collin could verify. >>>> >>> >>> I failed to find anything to support setting the function control >>> enabled bit when the function state is in error / blocked. I'm >>> assuming this might be some QEMU hack to get things working? I'll have >>> to dive further to understand why this was done this way, as it doesn't >>> align with how the s390x architecture is documented. It's confusing. >> >> Might this also be a real issue? Not matching the architecture is not a >> good sign... > > Friendly ping. If we still want to have this patch or a fix in 4.1, we > need to find out soon... > Let's take it for now. Acked-by: Collin Walling <walling@linux.ibm.com>
On Tue, 16 Jul 2019 14:34:22 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 7/16/19 11:20 AM, Cornelia Huck wrote: > > On Wed, 10 Jul 2019 10:20:41 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Tue, 9 Jul 2019 18:55:34 -0400 > >> Collin Walling <walling@linux.ibm.com> wrote: > >> > >>> On 7/8/19 9:23 AM, Christian Borntraeger wrote: > >>>> > >>>> > >>>> On 08.07.19 14:54, Cornelia Huck wrote: > >>>>> According to the comment, the bits are supposed to accumulate. > >>>>> > >>>>> Reported-by: Stefan Weil <sw@weilnetz.de> > >>>>> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") > >>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > >>>> > >>>> This patch does not change behaviour, so it is certainly not wrong. > >>>> > >>>> So lets have a look at if the bug report was actually a real bug or > >>>> just a missing annotation. > >>>> > >>>>> --- > >>>>> hw/s390x/s390-pci-inst.c | 2 ++ > >>>>> 1 file changed, 2 insertions(+) > >>>>> > >>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > >>>>> index 61f30b8e55d2..00235148bed7 100644 > >>>>> --- a/hw/s390x/s390-pci-inst.c > >>>>> +++ b/hw/s390x/s390-pci-inst.c > >>>>> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, > >>>>> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ > >>>>> case ZPCI_FS_ERROR: > >>>>> fib.fc |= 0x20; > >>>>> + /* fallthrough */ > >>>> > >>>> This is correct, in case of an error we are also blocked. > >>>> > >>> > >>> Agreed. This is definitely correct based on our architecture. > >>> > >>>>> case ZPCI_FS_BLOCKED: > >>>>> fib.fc |= 0x40; > >>>>> + /* fallthrough */ > >>>> > >>>> I think this is also correct, but it would be good if Collin could verify. > >>>> > >>> > >>> I failed to find anything to support setting the function control > >>> enabled bit when the function state is in error / blocked. I'm > >>> assuming this might be some QEMU hack to get things working? I'll have > >>> to dive further to understand why this was done this way, as it doesn't > >>> align with how the s390x architecture is documented. It's confusing. > >> > >> Might this also be a real issue? Not matching the architecture is not a > >> good sign... > > > > Friendly ping. If we still want to have this patch or a fix in 4.1, we > > need to find out soon... > > > > Let's take it for now. > > Acked-by: Collin Walling <walling@linux.ibm.com> > Just to be clear: You think that the current code is correct AFAYCS?
On 17.07.19 10:54, Cornelia Huck wrote: > On Tue, 16 Jul 2019 14:34:22 -0400 > Collin Walling <walling@linux.ibm.com> wrote: > >> On 7/16/19 11:20 AM, Cornelia Huck wrote: >>> On Wed, 10 Jul 2019 10:20:41 +0200 >>> Cornelia Huck <cohuck@redhat.com> wrote: >>> >>>> On Tue, 9 Jul 2019 18:55:34 -0400 >>>> Collin Walling <walling@linux.ibm.com> wrote: >>>> >>>>> On 7/8/19 9:23 AM, Christian Borntraeger wrote: >>>>>> >>>>>> >>>>>> On 08.07.19 14:54, Cornelia Huck wrote: >>>>>>> According to the comment, the bits are supposed to accumulate. >>>>>>> >>>>>>> Reported-by: Stefan Weil <sw@weilnetz.de> >>>>>>> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") >>>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>>>> >>>>>> This patch does not change behaviour, so it is certainly not wrong. >>>>>> >>>>>> So lets have a look at if the bug report was actually a real bug or >>>>>> just a missing annotation. >>>>>> >>>>>>> --- >>>>>>> hw/s390x/s390-pci-inst.c | 2 ++ >>>>>>> 1 file changed, 2 insertions(+) >>>>>>> >>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >>>>>>> index 61f30b8e55d2..00235148bed7 100644 >>>>>>> --- a/hw/s390x/s390-pci-inst.c >>>>>>> +++ b/hw/s390x/s390-pci-inst.c >>>>>>> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, >>>>>>> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ >>>>>>> case ZPCI_FS_ERROR: >>>>>>> fib.fc |= 0x20; >>>>>>> + /* fallthrough */ >>>>>> >>>>>> This is correct, in case of an error we are also blocked. >>>>>> >>>>> >>>>> Agreed. This is definitely correct based on our architecture. >>>>> >>>>>>> case ZPCI_FS_BLOCKED: >>>>>>> fib.fc |= 0x40; >>>>>>> + /* fallthrough */ >>>>>> >>>>>> I think this is also correct, but it would be good if Collin could verify. >>>>>> >>>>> >>>>> I failed to find anything to support setting the function control >>>>> enabled bit when the function state is in error / blocked. I'm >>>>> assuming this might be some QEMU hack to get things working? I'll have >>>>> to dive further to understand why this was done this way, as it doesn't >>>>> align with how the s390x architecture is documented. It's confusing. >>>> >>>> Might this also be a real issue? Not matching the architecture is not a >>>> good sign... >>> >>> Friendly ping. If we still want to have this patch or a fix in 4.1, we >>> need to find out soon... >>> >> >> Let's take it for now. >> >> Acked-by: Collin Walling <walling@linux.ibm.com> >> > > Just to be clear: You think that the current code is correct AFAYCS? I also looked into this again. There is a possibility to also be in disabled state. From what I can see, it makes sense that blocked and error belong to the enable state so the patch seems correct.
On 7/17/19 5:27 AM, Christian Borntraeger wrote: > > > On 17.07.19 10:54, Cornelia Huck wrote: >> On Tue, 16 Jul 2019 14:34:22 -0400 >> Collin Walling <walling@linux.ibm.com> wrote: >> >>> On 7/16/19 11:20 AM, Cornelia Huck wrote: >>>> On Wed, 10 Jul 2019 10:20:41 +0200 >>>> Cornelia Huck <cohuck@redhat.com> wrote: >>>> >>>>> On Tue, 9 Jul 2019 18:55:34 -0400 >>>>> Collin Walling <walling@linux.ibm.com> wrote: >>>>> >>>>>> On 7/8/19 9:23 AM, Christian Borntraeger wrote: >>>>>>> >>>>>>> >>>>>>> On 08.07.19 14:54, Cornelia Huck wrote: >>>>>>>> According to the comment, the bits are supposed to accumulate. >>>>>>>> >>>>>>>> Reported-by: Stefan Weil <sw@weilnetz.de> >>>>>>>> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") >>>>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>>>>> >>>>>>> This patch does not change behaviour, so it is certainly not wrong. >>>>>>> >>>>>>> So lets have a look at if the bug report was actually a real bug or >>>>>>> just a missing annotation. >>>>>>> >>>>>>>> --- >>>>>>>> hw/s390x/s390-pci-inst.c | 2 ++ >>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>> >>>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >>>>>>>> index 61f30b8e55d2..00235148bed7 100644 >>>>>>>> --- a/hw/s390x/s390-pci-inst.c >>>>>>>> +++ b/hw/s390x/s390-pci-inst.c >>>>>>>> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, >>>>>>>> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ >>>>>>>> case ZPCI_FS_ERROR: >>>>>>>> fib.fc |= 0x20; >>>>>>>> + /* fallthrough */ >>>>>>> >>>>>>> This is correct, in case of an error we are also blocked. >>>>>>> >>>>>> >>>>>> Agreed. This is definitely correct based on our architecture. >>>>>> >>>>>>>> case ZPCI_FS_BLOCKED: >>>>>>>> fib.fc |= 0x40; >>>>>>>> + /* fallthrough */ >>>>>>> >>>>>>> I think this is also correct, but it would be good if Collin could verify. >>>>>>> >>>>>> >>>>>> I failed to find anything to support setting the function control >>>>>> enabled bit when the function state is in error / blocked. I'm >>>>>> assuming this might be some QEMU hack to get things working? I'll have >>>>>> to dive further to understand why this was done this way, as it doesn't >>>>>> align with how the s390x architecture is documented. It's confusing. >>>>> >>>>> Might this also be a real issue? Not matching the architecture is not a >>>>> good sign... >>>> >>>> Friendly ping. If we still want to have this patch or a fix in 4.1, we >>>> need to find out soon... >>>> >>> >>> Let's take it for now. >>> >>> Acked-by: Collin Walling <walling@linux.ibm.com> >>> >> >> Just to be clear: You think that the current code is correct AFAYCS? > > I also looked into this again. > There is a possibility to also be in disabled state. > From what I can see, it makes sense that blocked and error belong to the enable state > so the patch seems correct. > Yes I agree. The material I referenced required me to look over a few times and ask around a bit. The patch is good. Apologies for my ambiguous response.
On Wed, 17 Jul 2019 08:52:54 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 7/17/19 5:27 AM, Christian Borntraeger wrote: > > > > > > On 17.07.19 10:54, Cornelia Huck wrote: > >> On Tue, 16 Jul 2019 14:34:22 -0400 > >> Collin Walling <walling@linux.ibm.com> wrote: > >> > >>> On 7/16/19 11:20 AM, Cornelia Huck wrote: > >>>> On Wed, 10 Jul 2019 10:20:41 +0200 > >>>> Cornelia Huck <cohuck@redhat.com> wrote: > >>>> > >>>>> On Tue, 9 Jul 2019 18:55:34 -0400 > >>>>> Collin Walling <walling@linux.ibm.com> wrote: > >>>>> > >>>>>> On 7/8/19 9:23 AM, Christian Borntraeger wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 08.07.19 14:54, Cornelia Huck wrote: > >>>>>>>> According to the comment, the bits are supposed to accumulate. > >>>>>>>> > >>>>>>>> Reported-by: Stefan Weil <sw@weilnetz.de> > >>>>>>>> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") > >>>>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > >>>>>>> > >>>>>>> This patch does not change behaviour, so it is certainly not wrong. > >>>>>>> > >>>>>>> So lets have a look at if the bug report was actually a real bug or > >>>>>>> just a missing annotation. > >>>>>>> > >>>>>>>> --- > >>>>>>>> hw/s390x/s390-pci-inst.c | 2 ++ > >>>>>>>> 1 file changed, 2 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > >>>>>>>> index 61f30b8e55d2..00235148bed7 100644 > >>>>>>>> --- a/hw/s390x/s390-pci-inst.c > >>>>>>>> +++ b/hw/s390x/s390-pci-inst.c > >>>>>>>> @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, > >>>>>>>> * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ > >>>>>>>> case ZPCI_FS_ERROR: > >>>>>>>> fib.fc |= 0x20; > >>>>>>>> + /* fallthrough */ > >>>>>>> > >>>>>>> This is correct, in case of an error we are also blocked. > >>>>>>> > >>>>>> > >>>>>> Agreed. This is definitely correct based on our architecture. > >>>>>> > >>>>>>>> case ZPCI_FS_BLOCKED: > >>>>>>>> fib.fc |= 0x40; > >>>>>>>> + /* fallthrough */ > >>>>>>> > >>>>>>> I think this is also correct, but it would be good if Collin could verify. > >>>>>>> > >>>>>> > >>>>>> I failed to find anything to support setting the function control > >>>>>> enabled bit when the function state is in error / blocked. I'm > >>>>>> assuming this might be some QEMU hack to get things working? I'll have > >>>>>> to dive further to understand why this was done this way, as it doesn't > >>>>>> align with how the s390x architecture is documented. It's confusing. > >>>>> > >>>>> Might this also be a real issue? Not matching the architecture is not a > >>>>> good sign... > >>>> > >>>> Friendly ping. If we still want to have this patch or a fix in 4.1, we > >>>> need to find out soon... > >>>> > >>> > >>> Let's take it for now. > >>> > >>> Acked-by: Collin Walling <walling@linux.ibm.com> > >>> > >> > >> Just to be clear: You think that the current code is correct AFAYCS? > > > > I also looked into this again. > > There is a possibility to also be in disabled state. > > From what I can see, it makes sense that blocked and error belong to the enable state > > so the patch seems correct. > > > > Yes I agree. The material I referenced required me to look over a few > times and ask around a bit. The patch is good. Apologies for my > ambiguous response. > Ok, thanks for the clarification. Queued now.
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 61f30b8e55d2..00235148bed7 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -1209,8 +1209,10 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, * FH Enabled bit is set to one in states of ENABLED, BLOCKED or ERROR. */ case ZPCI_FS_ERROR: fib.fc |= 0x20; + /* fallthrough */ case ZPCI_FS_BLOCKED: fib.fc |= 0x40; + /* fallthrough */ case ZPCI_FS_ENABLED: fib.fc |= 0x80; if (pbdev->iommu->enabled) {
According to the comment, the bits are supposed to accumulate. Reported-by: Stefan Weil <sw@weilnetz.de> Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking") Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- hw/s390x/s390-pci-inst.c | 2 ++ 1 file changed, 2 insertions(+)