Re: [Qemu-devel] [PATCH RFC v2 8/9] s390x/kvm: msi route fixup for non-pci

[Cornelia Huck]

On Wed, 19 Jul 2017 09:09:11 +0200
Thomas Huth <address@hidden> 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é <address@hidden> wrote:
> >>  
> >>> Hi Cornelia,
> >>>
> >>> On Tue, Jul 18, 2017 at 11:24 AM, Cornelia Huck <address@hidden>
> >>> 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 <address@hidden>
> >>>> ---
> >>>>   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.