Re: [Qemu-devel] [PATCH v4 09/10] s390x/kvm: msi route fixup for non-pci

[Halil Pasic]

On 08/21/2017 02:13 PM, Cornelia Huck wrote:
> On Mon, 21 Aug 2017 14:00:15 +0200
> Halil Pasic <address@hidden> 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 <address@hidden>
>>> ---
>>>  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

[..]