Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init()

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On 04/10/2016 12:38 PM, Cao jin wrote:
>>
>>
>> On 04/10/2016 04:20 PM, Marcel Apfelbaum wrote:
>>
>>>
>>> Hi,
>>> I'll let Markus to continue the review, it brings very valuable
>>> information,
>>> I will only try to answer the questions below.
>>>
>>>> Several questions on this topic:
>>>> 1. How to confirm whether a device model has non-MSI variant? AFAICT,
>>>> it is these who have msi property.

Examine how it uses msi_init().  That's how we give a PCI device
capability MSI.  If the device model treats msi_init() failure as fatal,
it doesn't have a non-MSI variant.

Some device models let the user ask for MSI with a property, and make
the msi_init() call depend on the property.  The property is commonly
called "msi" or "use_msi", with values "on" and "off".

msi=off then works as expected: you get the variant without MSI.

The meaning of msi=on depends on how msi_init() failure is handled.  If
it's fatal, then msi=on works as expected: you get the variant with MSI.
But if it's not fatal, then you may or may not get it, which I consider
a grossly misleading user interface.

To clean this up, we could add msi=auto, and move the non-fatal behavior
from msi=on to msi=auto.

Same for MSI-X with msix_init() and property "msix".

>>> MSI is required for PCI Express devices, optional for PCI devices.
>>> Even if a PCI device supports MSI, it is strongly advised to support
>>> legacy INTx for backward compatibility.

Devices commonly do, but the standards certainly permit devices that
only do MSI or MSI-X.

PCI Local Bus Specification Revision 3.0 (2004) section 6.8: "It is
recommended that devices implement interrupt pins to provide
compatibility in systems that do not support MSI (devices default to
interrupt pins).  However, it is expected that the need for interrupt
pins will diminish over time.  Devices that do not support interrupt
pins due to pin constraints (rely on polling for device service) may
implement messages to increase performance without adding additional
pins.  Therefore, system configuration software must not assume that a
message capable device has an interrupt pin."

PCIe emulates INTx for software compatibility with PCI.  PCI Express
Base Specification Revision 3.0 (2010) section 6.1.2: "The legacy INTx
emulation mechanism may be deprecated in a future version of this
specification."

>>> Bottom line, as far as I know, almost all PCI devices support legacy
>>> interrupts.
>>> (an exception is the ivshmem device that requires MSI)

Virtual devices modelled after a physical device should follow the
physical device's example as closely as practical.

For reasons discussed recently, we don't actually do that when it comes
to MSI and MSI-X.  Instead, we tend to provide up to four variants of a
virtual device: with/without MSI/MSI-X capability.  Typically they all
support legacy INTx.

ivshmem doesn't have a physical model, so we can do whatever we want.

The deprecated "ivshmem" device comes in four variants: with/without
interrupts, with/without MSI-X capability.  The variant without
interrupts but with MSI-X capability is of course silly.  The variant
with interrupts and with MSI-X capability can't do legacy INTx.  It
could, but it's not worth changing now.

The modern "ivshmem-plain" and "ivshmem-doorbell" device don't come in
variants.  The former doesn't have interrupts (guest-compatible to
"ivshmem" without interrupts and without MSI-X), and the latter only
supports MSI-X interrupts (guest-compatible to "ivshmem" with interrupts
and with MSI-X).

>>>> 2. For those have non-MSI variant devices(have msi property), as I see
>>>> in the code, they all have it on by default, So we won`t know it is
>>>> user order, or user don`t set it at all.

Correct.

Note that if we add value "auto" and make it the default, then value
"on" is known to be the user's order.

>>> I didn't quite understand the sentence, but some devices have a
>>> "use_msi" property that can be set by the user. If no such property exists,
>>> we can assume the user "prefers" the msi version.
>>>
>> Hi,
>> Sorry for my bad description. let me explain myself again.
>> I think(guess), if a device model has msi property, it means this device 
>> model has non-msi variant. For those devices who has msi property, I see 
>> most of them will have it on by default. So when these
>> devices initialize msi, qemu won`t know whether it is user order or not.
>>
>> If I understand Markus right:
>> 1). If user orders msi on, when msi_init returns -ENOTSUP, It is error. Then 
>> I suggest to inform user "set msi=off and try again"
>
> If the user *specifically* asked for msi=on (setting a property) is OK to 
> fail the device init process.

Yes, failing is the only sane behavior when the user asked for msi=on
but we can't provide it.

However, in the device model code, we can't know whether the asked for
msi=on, or whether the property defaults to on.  That's why I'd like us
to add msi=auto and make it the default.

>> 2). If user doesn`t order msi on(so device have msi on by default), when 
>> msi_init returns -ENOTSUP, I am ok with Markus`s suggestion: *caller should 
>> silently switch to the non-MSI variant*
>>
>> But now the condition is, qemu can`t distinguish whether user ordered msi or 
>> not, so for the condition 2) above, my suggestion is the same as 1)
>>
>
> If we can't distinguish between the cases, 2 would not be user-friendly. Once 
> it would be possible to differentiate we could go for 2.
>
>
> Thanks,
> Marcel
>
>>>
>>>> If user don`t know msi and don`t set it on, I think it is acceptable
>>>> to create the non-msi variant for user silently. But if it is user
>>>> order, like you said, it is an error.
>>>>
>>>
>>> I am not sure about this. At least a warning should be given IMHO.
>>>
>>>> So, how about: inform user to swich msi off and try again when
>>>> encounter -ENOTSUP, no matter it is user order, or user doesn`t set it
>>>> at all?
>>>>
>>>
>>> Not all devices have an "msi" switch. If the board has msi broken and
>>> the devices supports legacy interrupts, its OK to continue without MSI.
>>>
>>>> Actually in this v4, I do checked whether device has a msi property,
>>>> like cover-letter said:
>>>>
>>>>    3. most devices have msi/msix(except vmxnet3 & pvscsi) property as
>>>> a   switch, if it has and is switched on, then msi_init() failure
>>>> should   results in return directly. So in this version, mptsas
>>>> is updated
>>>>
>>>
>>> I don't see a "msi" properties on PCIDevice class or VirtioPCIClass, are
>>> you sure we have an msi switch for most of the PCI devices?
>>>
>>
>> My bad, I didn`t limit the range. I mean the devices who will call msi_init, 
>> they mostly have msi(or msix, or both) property

Further questions?