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

[Cornelia Huck]

On Wed, 26 Jul 2017 10:37:12 +0200
David Hildenbrand <address@hidden> wrote:

> On 26.07.2017 10:25, Thomas Huth wrote:
> > On 26.07.2017 10:20, Cornelia Huck wrote:  
> >> On Wed, 26 Jul 2017 09:09:06 +0200
> >> Thomas Huth <address@hidden> wrote:
> >>  
> >>> On 25.07.2017 17:33, Cornelia Huck 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 | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>>> index dc3f940b95..fb3e21a3a4 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -2424,6 +2424,11 @@ 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;
> >>>>  
> >>>> +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> >>>> +        DPRINTF("fixup_msi_route on non-pci machine?!\n");
> >>>> +        return -ENODEV;
> >>>> +    }
> >>>> +
> >>>>      pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx);
> >>>>      if (!pbdev) {
> >>>>          DPRINTF("add_msi_route no dev\n");
> >>>>     
> >>>
> >>> Is this additional check really needed here? I'd rather expect
> >>> s390_pci_find_dev_by_idx() to return NULL here already, so we should
> >>> already be fine, shouldn't we?  
> >>
> >> Yes, the end result is the same, but (1) better safe than sorry and (2)
> >> I can add a debug print here.
> >>
> >> I had actually considered throwing an error here, as this function
> >> really should not be called for !pci. Opinions?  
> > 
> > At least the current DPRINTF will go unnoticed in 99% of all cases since
> > it is not compiled in by default. So I'd say either do a proper
> > error_report() or even g_assert() here, or simply drop the patch.
> > 
> >  Thomas
> >   
> 
> I'd vote for g_assert() or simply dropping it.
> 

I don't like dropping the check. I'll go for g_assert().