[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [qemu-s390x] [PATCH for-4.1 1/2] s390x/pci: add some fallthrough ann

From: Collin Walling
Subject: Re: [qemu-s390x] [PATCH for-4.1 1/2] s390x/pci: add some fallthrough annotations
Date: Tue, 16 Jul 2019 14:34:22 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

On 7/16/19 11:20 AM, Cornelia Huck wrote:
On Wed, 10 Jul 2019 10:20:41 +0200
Cornelia Huck <address@hidden> wrote:

On Tue, 9 Jul 2019 18:55:34 -0400
Collin Walling <address@hidden> 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 <address@hidden>
Fixes: 5d1abf234462 ("s390x/pci: enforce zPCI state checking")
Signed-off-by: Cornelia Huck <address@hidden>

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 <address@hidden>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]