| Message ID | 20170725153330.14966-9-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <cohuck@redhat.com> > --- > 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? Thomas
On Wed, 26 Jul 2017 09:09:06 +0200 Thomas Huth <thuth@redhat.com> 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 <cohuck@redhat.com> > > --- > > 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?
On 26.07.2017 10:20, Cornelia Huck wrote: > On Wed, 26 Jul 2017 09:09:06 +0200 > Thomas Huth <thuth@redhat.com> 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 <cohuck@redhat.com> >>> --- >>> 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
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 <thuth@redhat.com> 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 <cohuck@redhat.com> >>>> --- >>>> 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.
On Wed, 26 Jul 2017 10:37:12 +0200 David Hildenbrand <david@redhat.com> 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 <thuth@redhat.com> 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 <cohuck@redhat.com> > >>>> --- > >>>> 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().
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");
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 | 5 +++++ 1 file changed, 5 insertions(+)