| Message ID | 20170821091614.28251-10-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/21/2017 11:16 AM, 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 | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 9de165d8b1..d8db1cbf6e 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -2424,6 +2424,12 @@ 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)) { > + /* How can we get here without pci enabled? */ > + g_assert(false); You don't tell us about the g_assert in the commit message. Do you expect G_DISABLE_ASSERT being defined for production builds. I've grepped for G_DISABLE_ASSERT and found nothing. And why g_assert over assert (again no guidance in HACKING mostly asking for my own learning)? Other that that LGTM. > + return -ENODEV; > + } > + > pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx); > if (!pbdev) { > DPRINTF("add_msi_route no dev\n"); >
On Mon, 21 Aug 2017 14:00:15 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 08/21/2017 11:16 AM, 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 | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > > index 9de165d8b1..d8db1cbf6e 100644 > > --- a/target/s390x/kvm.c > > +++ b/target/s390x/kvm.c > > @@ -2424,6 +2424,12 @@ 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)) { > > + /* How can we get here without pci enabled? */ > > + g_assert(false); > > You don't tell us about the g_assert in the commit message. > Do you expect G_DISABLE_ASSERT being defined for production > builds. I've grepped for G_DISABLE_ASSERT and found nothing. AFAIK this is set by distribution builds. I've also noticed that mingw builds treat (g_)assert() as if code flow continues, but I don't know whether asserts do anything there at all. > > And why g_assert over assert (again no guidance in HACKING > mostly asking for my own learning)? I do recall a recent(ish) discussion, but not the details. Anyway, using glib interfaces seems more consistent. > > Other that that LGTM. Thanks! > > > > + return -ENODEV; > > + } > > + > > pbdev = s390_pci_find_dev_by_idx(s390_get_phb(), idx); > > if (!pbdev) { > > DPRINTF("add_msi_route no dev\n"); > > >
On 08/21/2017 02:13 PM, Cornelia Huck wrote: > On Mon, 21 Aug 2017 14:00:15 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 08/21/2017 11:16 AM, 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 | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>> index 9de165d8b1..d8db1cbf6e 100644 >>> --- a/target/s390x/kvm.c >>> +++ b/target/s390x/kvm.c >>> @@ -2424,6 +2424,12 @@ 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)) { >>> + /* How can we get here without pci enabled? */ >>> + g_assert(false); >> >> You don't tell us about the g_assert in the commit message. >> Do you expect G_DISABLE_ASSERT being defined for production >> builds. I've grepped for G_DISABLE_ASSERT and found nothing. > > AFAIK this is set by distribution builds. I've also noticed that mingw > builds treat (g_)assert() as if code flow continues, but I don't know > whether asserts do anything there at all. > After thinking some more, I would prefer the the commit message being modified so, that it's clear, what we really want is the assert; or this assert being dropped or replaced with some kind of warning/tracing. I assume no production QEMU should ever return -ENODEV here (and if it does, it's due to an QEMU bug). So the assert is there to communicate with the devel, and just in case if the client code fails to fulfill their part of the contract. In this case we shall fail fast and hard, and no such QEMU should ship. The return -ENODEV is then 'just in case' on the square -- a failsafe for the failsafe (which does make sense to me). On the contrary if we assume this is a legit error condition (and a part of the contract) then according to HACKING 7.2 Handling errors we shall not call exit() or abort() to handle an error that can be triggered by the guest in particular and operation related errors in general. Makes perfect sense to me. Pierre helped me, kvm_arch_fixup_msi_route is guest triggered and also considering this particular case killing off the QEMU, and the guest does not seem like the lesser evil. The situation is just complicated by the fact that there is code which relies on assert(true) asserting for correctness (e.g. virtio goes so far to make builds with normal asserts disabled fail). Thus for me it's hard to assume that the assertion is guaranteed to be disabled in production. But if it ain't disabled than it calls abort() which we should not do (HACKING and IMHO common sense). TL;DR I'm for modifying the commit message so it tells us about the assert. >> >> And why g_assert over assert (again no guidance in HACKING >> mostly asking for my own learning)? > > I do recall a recent(ish) discussion, but not the details. Anyway, > using glib interfaces seems more consistent. We can't live with NDEBUG is another reason for using g_assert (makes my preferred solution work). Regards, Halil [..]
On 21.08.2017 17:10, Halil Pasic wrote: [...] > The situation is just complicated by the fact that there is code which > relies on assert(true) asserting for correctness (e.g. virtio goes so far > to make builds with normal asserts disabled fail). Thus for me it's hard > to assume that the assertion is guaranteed to be disabled in production. FYI: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html Thomas
On 08/21/2017 05:17 PM, Thomas Huth wrote: > On 21.08.2017 17:10, Halil Pasic wrote: > [...] >> The situation is just complicated by the fact that there is code which >> relies on assert(true) asserting for correctness (e.g. virtio goes so far >> to make builds with normal asserts disabled fail). Thus for me it's hard >> to assume that the assertion is guaranteed to be disabled in production. > > FYI: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html > > Thomas > Thanks, I've missed that. With that assumed it becomes either assert(false) or return -ENODEV but not both. Regards, Halil
On Mon, 21 Aug 2017 17:30:58 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 08/21/2017 05:17 PM, Thomas Huth wrote: > > On 21.08.2017 17:10, Halil Pasic wrote: > > [...] > >> The situation is just complicated by the fact that there is code which > >> relies on assert(true) asserting for correctness (e.g. virtio goes so far > >> to make builds with normal asserts disabled fail). Thus for me it's hard > >> to assume that the assertion is guaranteed to be disabled in production. > > > > FYI: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03608.html > > > > Thomas > > > > Thanks, I've missed that. With that assumed it becomes either > assert(false) or return -ENODEV but not both. > > Regards, > Halil > Thinking about this some more, this seems to be completely covered within the next statement: - For builds with pci completely disabled, we'll end up with NULL in both s390_get_phb() and s390_pci_find_dev_by_idx() and return -ENODEV. - If only the zpci facility bit is not set, we'll hit the assert in s390_get_phb(). Without an error message, there does not really seem to be additional value (other than failing explicitly), so I'll drop this patch. (Yeah, deja vu...)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 9de165d8b1..d8db1cbf6e 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -2424,6 +2424,12 @@ 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)) { + /* How can we get here without pci enabled? */ + g_assert(false); + 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 | 6 ++++++ 1 file changed, 6 insertions(+)