1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v1 0/4] s390x/zpci: some hotplug handler cleanups
  4. View patch

[Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available

David Hildenbrand posted 4 patches 7 years ago
[Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available
Posted by David Hildenbrand 7 years ago 
I fail to see why this is useful as we require MSIX always and
completely fail adding a device.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-pci-bus.c | 2 --
 hw/s390x/s390-pci-bus.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index e1b14b131b..1eaae3aca6 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
 
     pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
-        pbdev->msix.available = false;
         return -1;
     }
 
@@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
     pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
-    pbdev->msix.available = true;
 
     name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
     memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 1f7f9b5814..f47a0f2da5 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
 } QEMU_PACKED ChscSeiNt2Res;
 
 typedef struct S390MsixInfo {
-    bool available;
     uint8_t table_bar;
     uint8_t pba_bar;
     uint16_t entries;
-- 
2.17.2
Re: [Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available
Posted by Thomas Huth 7 years ago 
On 2018-11-05 12:03, David Hildenbrand wrote:
> I fail to see why this is useful as we require MSIX always and
> completely fail adding a device.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 2 --
>  hw/s390x/s390-pci-bus.h | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e1b14b131b..1eaae3aca6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>  
>      pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
>      if (!pos) {
> -        pbdev->msix.available = false;
>          return -1;
>      }
>  
> @@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>      pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
>      pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> -    pbdev->msix.available = true;
>  
>      name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
>      memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 1f7f9b5814..f47a0f2da5 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
>  } QEMU_PACKED ChscSeiNt2Res;
>  
>  typedef struct S390MsixInfo {
> -    bool available;
>      uint8_t table_bar;
>      uint8_t pba_bar;
>      uint16_t entries;
> 

Seems like the code that used this variable has been removed with commit
ID 4f6482bfe3da1e6b51ad4722a0c22f22f0d54a3b.

Fixes: 4f6482bfe3da1e6b51ad4722a0c22f22f0d54a3b
Reviewed-by: Thomas Huth <thuth@redhat.com>
Re: [Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available
Posted by Cornelia Huck 6 years, 11 months ago 
On Mon,  5 Nov 2018 12:03:10 +0100
David Hildenbrand <david@redhat.com> wrote:

> I fail to see why this is useful as we require MSIX always and
> completely fail adding a device.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 2 --
>  hw/s390x/s390-pci-bus.h | 1 -
>  2 files changed, 3 deletions(-)

Thanks, queued to s390-next.
Re: [Qemu-devel] [PATCH v1 1/4] s390x/zpci: drop msix.available
Posted by Cornelia Huck 7 years ago 
On Mon,  5 Nov 2018 12:03:10 +0100
David Hildenbrand <david@redhat.com> wrote:

> I fail to see why this is useful as we require MSIX always and
> completely fail adding a device.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-pci-bus.c | 2 --
>  hw/s390x/s390-pci-bus.h | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index e1b14b131b..1eaae3aca6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>  
>      pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
>      if (!pos) {
> -        pbdev->msix.available = false;
>          return -1;
>      }
>  
> @@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>      pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
>      pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> -    pbdev->msix.available = true;
>  
>      name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
>      memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 1f7f9b5814..f47a0f2da5 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
>  } QEMU_PACKED ChscSeiNt2Res;
>  
>  typedef struct S390MsixInfo {
> -    bool available;
>      uint8_t table_bar;
>      uint8_t pba_bar;
>      uint16_t entries;

OK, so that was a write-only variable? :)

Question for the IBMers: is there any change we might have a different
implementation not relying on msi-x in the future?
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/4] s390x/zpci: drop msix.available
Posted by Collin Walling 6 years, 12 months ago 
On 11/5/18 6:19 AM, Cornelia Huck wrote:
> On Mon,  5 Nov 2018 12:03:10 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> I fail to see why this is useful as we require MSIX always and
>> completely fail adding a device.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-pci-bus.c | 2 --
>>  hw/s390x/s390-pci-bus.h | 1 -
>>  2 files changed, 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index e1b14b131b..1eaae3aca6 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>>  
>>      pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
>>      if (!pos) {
>> -        pbdev->msix.available = false;
>>          return -1;
>>      }
>>  
>> @@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
>>      pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
>>      pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>>      pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
>> -    pbdev->msix.available = true;
>>  
>>      name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
>>      memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
>> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
>> index 1f7f9b5814..f47a0f2da5 100644
>> --- a/hw/s390x/s390-pci-bus.h
>> +++ b/hw/s390x/s390-pci-bus.h
>> @@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
>>  } QEMU_PACKED ChscSeiNt2Res;
>>  
>>  typedef struct S390MsixInfo {
>> -    bool available;
>>      uint8_t table_bar;
>>      uint8_t pba_bar;
>>      uint16_t entries;
> 
> OK, so that was a write-only variable? :)
> 
> Question for the IBMers: is there any change we might have a different
> implementation not relying on msi-x in the future?
> 

@Conny
Currently, the plan would be to stick with a hard requirement for MSIX unless someone 
strongly supports the legacy alternatives. I'm certainly open to discuss that. Maybe it 
would make sense to fallback to MSI for devices that don't support MSIX?

@David
Thanks for the cleanup.

Reviewed-by: Collin Walling <walling@linux.ibm.com>

-- 
Respectfully,
- Collin Walling
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/4] s390x/zpci: drop msix.available
Posted by Cornelia Huck 6 years, 12 months ago 
On Wed, 7 Nov 2018 11:26:50 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> On 11/5/18 6:19 AM, Cornelia Huck wrote:
> > On Mon,  5 Nov 2018 12:03:10 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> I fail to see why this is useful as we require MSIX always and
> >> completely fail adding a device.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/s390x/s390-pci-bus.c | 2 --
> >>  hw/s390x/s390-pci-bus.h | 1 -
> >>  2 files changed, 3 deletions(-)
> >>
> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >> index e1b14b131b..1eaae3aca6 100644
> >> --- a/hw/s390x/s390-pci-bus.c
> >> +++ b/hw/s390x/s390-pci-bus.c
> >> @@ -745,7 +745,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
> >>  
> >>      pos = pci_find_capability(pbdev->pdev, PCI_CAP_ID_MSIX);
> >>      if (!pos) {
> >> -        pbdev->msix.available = false;
> >>          return -1;
> >>      }
> >>  
> >> @@ -761,7 +760,6 @@ static int s390_pci_msix_init(S390PCIBusDevice *pbdev)
> >>      pbdev->msix.pba_bar = pba & PCI_MSIX_FLAGS_BIRMASK;
> >>      pbdev->msix.pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> >>      pbdev->msix.entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> >> -    pbdev->msix.available = true;
> >>  
> >>      name = g_strdup_printf("msix-s390-%04x", pbdev->uid);
> >>      memory_region_init_io(&pbdev->msix_notify_mr, OBJECT(pbdev),
> >> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> >> index 1f7f9b5814..f47a0f2da5 100644
> >> --- a/hw/s390x/s390-pci-bus.h
> >> +++ b/hw/s390x/s390-pci-bus.h
> >> @@ -252,7 +252,6 @@ typedef struct ChscSeiNt2Res {
> >>  } QEMU_PACKED ChscSeiNt2Res;
> >>  
> >>  typedef struct S390MsixInfo {
> >> -    bool available;
> >>      uint8_t table_bar;
> >>      uint8_t pba_bar;
> >>      uint16_t entries;  
> > 
> > OK, so that was a write-only variable? :)
> > 
> > Question for the IBMers: is there any change we might have a different
> > implementation not relying on msi-x in the future?
> >   
> 
> @Conny
> Currently, the plan would be to stick with a hard requirement for MSIX unless someone 
> strongly supports the legacy alternatives. I'm certainly open to discuss that. Maybe it 
> would make sense to fallback to MSI for devices that don't support MSIX?

I'm not sure whether that would be very useful. My question was more
along the lines of "ok, so now that available flag is gone for good?" :)

> 
> @David
> Thanks for the cleanup.
> 
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.