[PATCH v2] s390x: adapter routes error handling

[PATCH v2] s390x: adapter routes error handling

[Cornelia Huck]

If the kernel irqchip has been disabled, we don't want the
{add,release}_adapter_routes routines to call any kvm_irqchip_*
interfaces, as they may rely on an irqchip actually having been
created. Just take a quick exit in that case instead.

Also initialize routes->gsi[] with -1 in the virtio-ccw handling,
to make sure we don't trip over other errors, either. (Nobody
else uses the gsi array in that structure.)

Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

v1->v2:
  - also initialize the gsi array with -1

---
 hw/intc/s390_flic_kvm.c | 8 ++++++++
 hw/s390x/virtio-ccw.c   | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index dddd33ea61c8..44b7960ebcc8 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -331,6 +331,10 @@ static int kvm_s390_add_adapter_routes(S390FLICState *fs,
     int ret, i;
     uint64_t ind_offset = routes->adapter.ind_offset;
 
+    if (!kvm_gsi_routing_enabled()) {
+        return -ENOSYS;
+    }
+
     for (i = 0; i < routes->num_routes; i++) {
         ret = kvm_irqchip_add_adapter_route(kvm_state, &routes->adapter);
         if (ret < 0) {
@@ -358,6 +362,10 @@ static void kvm_s390_release_adapter_routes(S390FLICState *fs,
 {
     int i;
 
+    if (!kvm_gsi_routing_enabled()) {
+        return;
+    }
+
     for (i = 0; i < routes->num_routes; i++) {
         if (routes->gsi[i] >= 0) {
             kvm_irqchip_release_virq(kvm_state, routes->gsi[i]);
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 6580ce5907dd..13f57e7b67f1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -697,6 +697,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
     CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
     SubchDev *sch;
     Error *err = NULL;
+    int i;
 
     sch = css_create_sch(ccw_dev->devno, errp);
     if (!sch) {
@@ -717,6 +718,9 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
     ccw_dev->sch = sch;
     dev->indicators = NULL;
     dev->revision = -1;
+    for (i = 0; i < ADAPTER_ROUTES_MAX_GSI; i++) {
+        dev->routes.gsi[i] = -1;
+    }
     css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
 
     trace_virtio_ccw_new_device(
-- 
2.21.1

Re: [PATCH v2] s390x: adapter routes error handling

[Thomas Huth]

On 17/01/2020 12.11, Cornelia Huck wrote:
> If the kernel irqchip has been disabled, we don't want the
> {add,release}_adapter_routes routines to call any kvm_irqchip_*
> interfaces, as they may rely on an irqchip actually having been
> created. Just take a quick exit in that case instead.
> 
> Also initialize routes->gsi[] with -1 in the virtio-ccw handling,
> to make sure we don't trip over other errors, either. (Nobody
> else uses the gsi array in that structure.)
> 
> Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v1->v2:
>   - also initialize the gsi array with -1
> 
> ---
>  hw/intc/s390_flic_kvm.c | 8 ++++++++
>  hw/s390x/virtio-ccw.c   | 4 ++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index dddd33ea61c8..44b7960ebcc8 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -331,6 +331,10 @@ static int kvm_s390_add_adapter_routes(S390FLICState *fs,
>      int ret, i;
>      uint64_t ind_offset = routes->adapter.ind_offset;
>  
> +    if (!kvm_gsi_routing_enabled()) {
> +        return -ENOSYS;
> +    }
> +
>      for (i = 0; i < routes->num_routes; i++) {
>          ret = kvm_irqchip_add_adapter_route(kvm_state, &routes->adapter);
>          if (ret < 0) {
> @@ -358,6 +362,10 @@ static void kvm_s390_release_adapter_routes(S390FLICState *fs,
>  {
>      int i;
>  
> +    if (!kvm_gsi_routing_enabled()) {
> +        return;
> +    }
> +
>      for (i = 0; i < routes->num_routes; i++) {
>          if (routes->gsi[i] >= 0) {
>              kvm_irqchip_release_virq(kvm_state, routes->gsi[i]);
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 6580ce5907dd..13f57e7b67f1 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -697,6 +697,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
>      CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
>      SubchDev *sch;
>      Error *err = NULL;
> +    int i;
>  
>      sch = css_create_sch(ccw_dev->devno, errp);
>      if (!sch) {
> @@ -717,6 +718,9 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
>      ccw_dev->sch = sch;
>      dev->indicators = NULL;
>      dev->revision = -1;
> +    for (i = 0; i < ADAPTER_ROUTES_MAX_GSI; i++) {
> +        dev->routes.gsi[i] = -1;
> +    }
>      css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
>  
>      trace_virtio_ccw_new_device(
> 

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

Re: [PATCH v2] s390x: adapter routes error handling

[Christian Borntraeger]

On 17.01.20 12:11, Cornelia Huck wrote:
> If the kernel irqchip has been disabled, we don't want the
> {add,release}_adapter_routes routines to call any kvm_irqchip_*
> interfaces, as they may rely on an irqchip actually having been
> created. Just take a quick exit in that case instead.
> 
> Also initialize routes->gsi[] with -1 in the virtio-ccw handling,
> to make sure we don't trip over other errors, either. (Nobody
> else uses the gsi array in that structure.)
> 
> Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

I think it was actually quite good to see an error, because something went wrong
(kvmirqchip being off). Now the error (crash) was certainly a bad one.
What happens after this patch?
To me it _looks_ like every caller of set_guest_notifiers would get the ENOSYS
and bail out with an error so this should be ok, but it would be good
to add something to the patch description that says so.

Something like "instead of crashing we now fail with an error message for vhost
and friends"
of course only if this is true.

> ---
> 
> v1->v2:
>   - also initialize the gsi array with -1
> 
> ---
>  hw/intc/s390_flic_kvm.c | 8 ++++++++
>  hw/s390x/virtio-ccw.c   | 4 ++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index dddd33ea61c8..44b7960ebcc8 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -331,6 +331,10 @@ static int kvm_s390_add_adapter_routes(S390FLICState *fs,
>      int ret, i;
>      uint64_t ind_offset = routes->adapter.ind_offset;
>  
> +    if (!kvm_gsi_routing_enabled()) {
> +        return -ENOSYS;
> +    }
> +
>      for (i = 0; i < routes->num_routes; i++) {
>          ret = kvm_irqchip_add_adapter_route(kvm_state, &routes->adapter);
>          if (ret < 0) {
> @@ -358,6 +362,10 @@ static void kvm_s390_release_adapter_routes(S390FLICState *fs,
>  {
>      int i;
>  
> +    if (!kvm_gsi_routing_enabled()) {
> +        return;
> +    }
> +
>      for (i = 0; i < routes->num_routes; i++) {
>          if (routes->gsi[i] >= 0) {
>              kvm_irqchip_release_virq(kvm_state, routes->gsi[i]);
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 6580ce5907dd..13f57e7b67f1 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -697,6 +697,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
>      CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
>      SubchDev *sch;
>      Error *err = NULL;
> +    int i;
>  
>      sch = css_create_sch(ccw_dev->devno, errp);
>      if (!sch) {
> @@ -717,6 +718,9 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
>      ccw_dev->sch = sch;
>      dev->indicators = NULL;
>      dev->revision = -1;
> +    for (i = 0; i < ADAPTER_ROUTES_MAX_GSI; i++) {
> +        dev->routes.gsi[i] = -1;
> +    }
>      css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
>  
>      trace_virtio_ccw_new_device(
> 

Re: [PATCH v2] s390x: adapter routes error handling

[Cornelia Huck]

On Fri, 17 Jan 2020 12:22:45 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 17.01.20 12:11, Cornelia Huck wrote:
> > If the kernel irqchip has been disabled, we don't want the
> > {add,release}_adapter_routes routines to call any kvm_irqchip_*
> > interfaces, as they may rely on an irqchip actually having been
> > created. Just take a quick exit in that case instead.
> > 
> > Also initialize routes->gsi[] with -1 in the virtio-ccw handling,
> > to make sure we don't trip over other errors, either. (Nobody
> > else uses the gsi array in that structure.)
> > 
> > Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
> 
> I think it was actually quite good to see an error, because something went wrong
> (kvmirqchip being off). Now the error (crash) was certainly a bad one.
> What happens after this patch?
> To me it _looks_ like every caller of set_guest_notifiers would get the ENOSYS
> and bail out with an error so this should be ok, but it would be good
> to add something to the patch description that says so.
> 
> Something like "instead of crashing we now fail with an error message for vhost
> and friends"
> of course only if this is true.

It should work in the same way as it does for tcg right now (we return
-ENOSYS in the non-kvm flic as well). If you're not using irqfd,
everything will work just fine.

What about the following:

"If you are trying to use irqfd without a kernel irqchip, we will fail
with an error."

?

There probably won't be many people seeing anything like this, as I
guess most people will delegate the irqfd setup to libvirt anyway,
which will not turn off the kernel irqchip.

Re: [PATCH v2] s390x: adapter routes error handling

[Christian Borntraeger]

On 17.01.20 12:33, Cornelia Huck wrote:
> On Fri, 17 Jan 2020 12:22:45 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 17.01.20 12:11, Cornelia Huck wrote:
>>> If the kernel irqchip has been disabled, we don't want the
>>> {add,release}_adapter_routes routines to call any kvm_irqchip_*
>>> interfaces, as they may rely on an irqchip actually having been
>>> created. Just take a quick exit in that case instead.
>>>
>>> Also initialize routes->gsi[] with -1 in the virtio-ccw handling,
>>> to make sure we don't trip over other errors, either. (Nobody
>>> else uses the gsi array in that structure.)
>>>
>>> Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
>>
>> I think it was actually quite good to see an error, because something went wrong
>> (kvmirqchip being off). Now the error (crash) was certainly a bad one.
>> What happens after this patch?
>> To me it _looks_ like every caller of set_guest_notifiers would get the ENOSYS
>> and bail out with an error so this should be ok, but it would be good
>> to add something to the patch description that says so.
>>
>> Something like "instead of crashing we now fail with an error message for vhost
>> and friends"
>> of course only if this is true.
> 
> It should work in the same way as it does for tcg right now (we return
> -ENOSYS in the non-kvm flic as well). If you're not using irqfd,
> everything will work just fine.
> 
> What about the following:
> 
> "If you are trying to use irqfd without a kernel irqchip, we will fail
> with an error."
> 

With that
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Re: [PATCH v2] s390x: adapter routes error handling

[Cornelia Huck]

On Fri, 17 Jan 2020 12:11:47 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> If the kernel irqchip has been disabled, we don't want the
> {add,release}_adapter_routes routines to call any kvm_irqchip_*
> interfaces, as they may rely on an irqchip actually having been
> created. Just take a quick exit in that case instead.
> 
> Also initialize routes->gsi[] with -1 in the virtio-ccw handling,
> to make sure we don't trip over other errors, either. (Nobody
> else uses the gsi array in that structure.)
> 
> Fixes: d426d9fba8ea ("s390x/virtio-ccw: wire up irq routing and irqfds")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> v1->v2:
>   - also initialize the gsi array with -1
> 
> ---
>  hw/intc/s390_flic_kvm.c | 8 ++++++++
>  hw/s390x/virtio-ccw.c   | 4 ++++
>  2 files changed, 12 insertions(+)

Queued (with amended commit message) to s390-next.