diff mbox

[RFC,v2,8/9] s390x/kvm: msi route fixup for non-pci

Message ID 20170718142455.32676-9-cohuck@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck July 18, 2017, 2:24 p.m. UTC
If we don't provide pci, we cannot have a pci device for which we
have to translate to adapter routes: just return -ENODEV.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/kvm.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

Philippe Mathieu-Daudé July 18, 2017, 2:58 p.m. UTC | #1
Hi Cornelia,

On Tue, Jul 18, 2017 at 11:24 AM, Cornelia Huck <cohuck@redhat.com> wrote:
> If we don't provide pci, we cannot have a pci device for which we
> have to translate to adapter routes: just return -ENODEV.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 60688888c3..df0e5af151 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2424,22 +2424,27 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>      uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
>
> -    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> -    if (!pbdev) {
> -        DPRINTF("add_msi_route no dev\n");
> -        return -ENODEV;
> -    }
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> +        if (!pbdev) {
> +            DPRINTF("add_msi_route no dev\n");
> +            return -ENODEV;
> +        }
>
> -    pbdev->routes.adapter.ind_offset = vec;
> +        pbdev->routes.adapter.ind_offset = vec;
>
> -    route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
> -    route->flags = 0;
> -    route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
> -    route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
> -    route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
> -    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
> -    route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
> -    return 0;
> +        route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
> +        route->flags = 0;
> +        route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
> +        route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
> +        route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
> +        route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
> +        route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
> +        return 0;
> +    } else {
> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> +        return -ENODEV;
> +    }
>  }
>
>  int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
> --
> 2.13.3

What about inverting the check?

+    if (!s390_has_feat(S390_FEAT_ZPCI)) {
+        DPRINTF("fixup_msi_route on non-pci machine?!\n");
+        return -ENODEV;
+    }

Phil.
Cornelia Huck July 18, 2017, 3:22 p.m. UTC | #2
On Tue, 18 Jul 2017 11:58:08 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi Cornelia,
> 
> On Tue, Jul 18, 2017 at 11:24 AM, Cornelia Huck <cohuck@redhat.com> wrote:
> > If we don't provide pci, we cannot have a pci device for which we
> > have to translate to adapter routes: just return -ENODEV.
> >
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index 60688888c3..df0e5af151 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2424,22 +2424,27 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
> >      uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
> >      uint32_t vec = data & ZPCI_MSI_VEC_MASK;
> >
> > -    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> > -    if (!pbdev) {
> > -        DPRINTF("add_msi_route no dev\n");
> > -        return -ENODEV;
> > -    }
> > +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> > +        pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> > +        if (!pbdev) {
> > +            DPRINTF("add_msi_route no dev\n");
> > +            return -ENODEV;
> > +        }
> >
> > -    pbdev->routes.adapter.ind_offset = vec;
> > +        pbdev->routes.adapter.ind_offset = vec;
> >
> > -    route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
> > -    route->flags = 0;
> > -    route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
> > -    route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
> > -    route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
> > -    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
> > -    route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
> > -    return 0;
> > +        route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
> > +        route->flags = 0;
> > +        route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
> > +        route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
> > +        route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
> > +        route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
> > +        route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
> > +        return 0;
> > +    } else {
> > +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> > +        return -ENODEV;
> > +    }
> >  }
> >
> >  int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
> > --
> > 2.13.3  
> 
> What about inverting the check?
> 
> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> +        return -ENODEV;
> +    }

I usually prefer the more common branch on top, but (1) this causes
more changes in this case and (2) I'm not so sure if zpci on really is
the common case...
Yi Min Zhao July 19, 2017, 3:13 a.m. UTC | #3
I think moving the new code on the top of this function would make it 
more readable.


在 2017/7/18 下午10:24, Cornelia Huck 写道:
> +    } else {
> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> +        return -ENODEV;
> +    }
Yi Min Zhao July 19, 2017, 3:16 a.m. UTC | #4
在 2017/7/18 下午11:22, Cornelia Huck 写道:
> On Tue, 18 Jul 2017 11:58:08 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
>> Hi Cornelia,
>>
>> On Tue, Jul 18, 2017 at 11:24 AM, Cornelia Huck <cohuck@redhat.com> wrote:
>>> If we don't provide pci, we cannot have a pci device for which we
>>> have to translate to adapter routes: just return -ENODEV.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>   target/s390x/kvm.c | 33 +++++++++++++++++++--------------
>>>   1 file changed, 19 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 60688888c3..df0e5af151 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2424,22 +2424,27 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>>>       uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>>>       uint32_t vec = data & ZPCI_MSI_VEC_MASK;
>>>
>>> -    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>>> -    if (!pbdev) {
>>> -        DPRINTF("add_msi_route no dev\n");
>>> -        return -ENODEV;
>>> -    }
>>> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
>>> +        pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>>> +        if (!pbdev) {
>>> +            DPRINTF("add_msi_route no dev\n");
>>> +            return -ENODEV;
>>> +        }
>>>
>>> -    pbdev->routes.adapter.ind_offset = vec;
>>> +        pbdev->routes.adapter.ind_offset = vec;
>>>
>>> -    route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
>>> -    route->flags = 0;
>>> -    route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
>>> -    route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
>>> -    route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
>>> -    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
>>> -    route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
>>> -    return 0;
>>> +        route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
>>> +        route->flags = 0;
>>> +        route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
>>> +        route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
>>> +        route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
>>> +        route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
>>> +        route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
>>> +        return 0;
>>> +    } else {
>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
>>> +        return -ENODEV;
>>> +    }
>>>   }
>>>
>>>   int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
>>> --
>>> 2.13.3
>> What about inverting the check?
>>
>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
>> +        return -ENODEV;
>> +    }
> I usually prefer the more common branch on top, but (1) this causes
> more changes in this case and (2) I'm not so sure if zpci on really is
> the common case...
>
>
Sorry for my duplicated comment. I think we don't know which is more 
common. Currently 2.9
machine doesn' t support zpci facility. But in the future, how will the 
thing change?
Thomas Huth July 19, 2017, 7:09 a.m. UTC | #5
On 19.07.2017 05:16, Yi Min Zhao wrote:
> 
> 
> 在 2017/7/18 下午11:22, Cornelia Huck 写道:
>> On Tue, 18 Jul 2017 11:58:08 -0300
>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>>> Hi Cornelia,
>>>
>>> On Tue, Jul 18, 2017 at 11:24 AM, Cornelia Huck <cohuck@redhat.com>
>>> wrote:
>>>> If we don't provide pci, we cannot have a pci device for which we
>>>> have to translate to adapter routes: just return -ENODEV.
>>>>
>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>> ---
>>>>   target/s390x/kvm.c | 33 +++++++++++++++++++--------------
>>>>   1 file changed, 19 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index 60688888c3..df0e5af151 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -2424,22 +2424,27 @@ int kvm_arch_fixup_msi_route(struct
>>>> kvm_irq_routing_entry *route,
>>>>       uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
>>>>       uint32_t vec = data & ZPCI_MSI_VEC_MASK;
>>>>
>>>> -    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>>>> -    if (!pbdev) {
>>>> -        DPRINTF("add_msi_route no dev\n");
>>>> -        return -ENODEV;
>>>> -    }
>>>> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
>>>> +        pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
>>>> +        if (!pbdev) {
>>>> +            DPRINTF("add_msi_route no dev\n");
>>>> +            return -ENODEV;
>>>> +        }
>>>>
>>>> -    pbdev->routes.adapter.ind_offset = vec;
>>>> +        pbdev->routes.adapter.ind_offset = vec;
>>>>
>>>> -    route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
>>>> -    route->flags = 0;
>>>> -    route->u.adapter.summary_addr =
>>>> pbdev->routes.adapter.summary_addr;
>>>> -    route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
>>>> -    route->u.adapter.summary_offset =
>>>> pbdev->routes.adapter.summary_offset;
>>>> -    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
>>>> -    route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
>>>> -    return 0;
>>>> +        route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
>>>> +        route->flags = 0;
>>>> +        route->u.adapter.summary_addr =
>>>> pbdev->routes.adapter.summary_addr;
>>>> +        route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
>>>> +        route->u.adapter.summary_offset =
>>>> pbdev->routes.adapter.summary_offset;
>>>> +        route->u.adapter.ind_offset =
>>>> pbdev->routes.adapter.ind_offset;
>>>> +        route->u.adapter.adapter_id =
>>>> pbdev->routes.adapter.adapter_id;
>>>> +        return 0;
>>>> +    } else {
>>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>>   }
>>>>
>>>>   int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
>>>> -- 
>>>> 2.13.3
>>> What about inverting the check?
>>>
>>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
>>> +        return -ENODEV;
>>> +    }
>> I usually prefer the more common branch on top, but (1) this causes
>> more changes in this case and (2) I'm not so sure if zpci on really is
>> the common case...
>>
> Sorry for my duplicated comment. I think we don't know which is more
> common. Currently 2.9
> machine doesn' t support zpci facility. But in the future, how will the
> thing change?

No matter whether zpci is the more common case or not, but one good
coding style is to keep the level of indentation small. So the early
exit as suggested by Philippe is certainly a good idea in this case here.

 Thomas
Cornelia Huck July 19, 2017, 7:30 a.m. UTC | #6
On Wed, 19 Jul 2017 09:09:11 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 19.07.2017 05:16, Yi Min Zhao wrote:
> > 
> > 
> > 在 2017/7/18 下午11:22, Cornelia Huck 写道:  
> >> On Tue, 18 Jul 2017 11:58:08 -0300
> >> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>  
> >>> Hi Cornelia,
> >>>
> >>> On Tue, Jul 18, 2017 at 11:24 AM, Cornelia Huck <cohuck@redhat.com>
> >>> wrote:  
> >>>> If we don't provide pci, we cannot have a pci device for which we
> >>>> have to translate to adapter routes: just return -ENODEV.
> >>>>
> >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>>> ---
> >>>>   target/s390x/kvm.c | 33 +++++++++++++++++++--------------
> >>>>   1 file changed, 19 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>> index 60688888c3..df0e5af151 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -2424,22 +2424,27 @@ int kvm_arch_fixup_msi_route(struct
> >>>> kvm_irq_routing_entry *route,
> >>>>       uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
> >>>>       uint32_t vec = data & ZPCI_MSI_VEC_MASK;
> >>>>
> >>>> -    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> >>>> -    if (!pbdev) {
> >>>> -        DPRINTF("add_msi_route no dev\n");
> >>>> -        return -ENODEV;
> >>>> -    }
> >>>> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> >>>> +        pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> >>>> +        if (!pbdev) {
> >>>> +            DPRINTF("add_msi_route no dev\n");
> >>>> +            return -ENODEV;
> >>>> +        }
> >>>>
> >>>> -    pbdev->routes.adapter.ind_offset = vec;
> >>>> +        pbdev->routes.adapter.ind_offset = vec;
> >>>>
> >>>> -    route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
> >>>> -    route->flags = 0;
> >>>> -    route->u.adapter.summary_addr =
> >>>> pbdev->routes.adapter.summary_addr;
> >>>> -    route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
> >>>> -    route->u.adapter.summary_offset =
> >>>> pbdev->routes.adapter.summary_offset;
> >>>> -    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
> >>>> -    route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
> >>>> -    return 0;
> >>>> +        route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
> >>>> +        route->flags = 0;
> >>>> +        route->u.adapter.summary_addr =
> >>>> pbdev->routes.adapter.summary_addr;
> >>>> +        route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
> >>>> +        route->u.adapter.summary_offset =
> >>>> pbdev->routes.adapter.summary_offset;
> >>>> +        route->u.adapter.ind_offset =
> >>>> pbdev->routes.adapter.ind_offset;
> >>>> +        route->u.adapter.adapter_id =
> >>>> pbdev->routes.adapter.adapter_id;
> >>>> +        return 0;
> >>>> +    } else {
> >>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> >>>> +        return -ENODEV;
> >>>> +    }
> >>>>   }
> >>>>
> >>>>   int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
> >>>> -- 
> >>>> 2.13.3  
> >>> What about inverting the check?
> >>>
> >>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> >>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> >>> +        return -ENODEV;
> >>> +    }  
> >> I usually prefer the more common branch on top, but (1) this causes
> >> more changes in this case and (2) I'm not so sure if zpci on really is
> >> the common case...
> >>  
> > Sorry for my duplicated comment. I think we don't know which is more
> > common. Currently 2.9
> > machine doesn' t support zpci facility. But in the future, how will the
> > thing change?  
> 
> No matter whether zpci is the more common case or not, but one good
> coding style is to keep the level of indentation small. So the early
> exit as suggested by Philippe is certainly a good idea in this case here.

Yes, I will change that in the next revision.
diff mbox

Patch

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 60688888c3..df0e5af151 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2424,22 +2424,27 @@  int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
     uint32_t idx = data >> ZPCI_MSI_VEC_BITS;
     uint32_t vec = data & ZPCI_MSI_VEC_MASK;
 
-    pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
-    if (!pbdev) {
-        DPRINTF("add_msi_route no dev\n");
-        return -ENODEV;
-    }
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
+        if (!pbdev) {
+            DPRINTF("add_msi_route no dev\n");
+            return -ENODEV;
+        }
 
-    pbdev->routes.adapter.ind_offset = vec;
+        pbdev->routes.adapter.ind_offset = vec;
 
-    route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
-    route->flags = 0;
-    route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
-    route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
-    route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
-    route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
-    route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
-    return 0;
+        route->type = KVM_IRQ_ROUTING_S390_ADAPTER;
+        route->flags = 0;
+        route->u.adapter.summary_addr = pbdev->routes.adapter.summary_addr;
+        route->u.adapter.ind_addr = pbdev->routes.adapter.ind_addr;
+        route->u.adapter.summary_offset = pbdev->routes.adapter.summary_offset;
+        route->u.adapter.ind_offset = pbdev->routes.adapter.ind_offset;
+        route->u.adapter.adapter_id = pbdev->routes.adapter.adapter_id;
+        return 0;
+    } else {
+        DPRINTF("fixup_msi_route on non-pci machine?!\n");
+        return -ENODEV;
+    }
 }
 
 int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,