Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote:
>>>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors,
>>>> +             bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>>>   {
>>>>       unsigned int vectors_order;
>>>> -    uint16_t flags;
>>>> +    uint16_t flags; /* Message Control register value */
>>>>       uint8_t cap_size;
>>>>       int config_offset;
>>>>
>>>>       if (!msi_supported) {
>>>> +        error_setg(errp, "MSI is not supported by interrupt controller");
>>>>           return -ENOTSUP;
>>>
>>> First failure mode: board does not support MSI (-ENOTSUP).
>>>
>>> Question to the PCI guys: why is this even an error?  A device with
>>> capability MSI should work just fine in such a board.
>>
>> Hi Markus,
>>
>> Adding Jan Kiszka, maybe he can help.
>>
>> That's a fair question. Is there any history for this decision?
>> The board not supporting MSI has nothing to do with the capability being there.
>> The HW should not change because the board doe not support it.
>>
>> The capability should be present but not active.
>
> Digging in git log will tell you why we have the msi_supported flag:
>
> commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions")
>
> 	This is a safety measure to avoid breaking platforms which should support
> 	MSI-X but currently lack this in the interrupt controller emulation.
>
> in other words, on some platforms we must hide msi support from devices
> because otherwise guests will try to use it, and our emulation is
> incomplete.


OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not
"msi_supported" that leads (at least me) to the assumption that some platform *does not support msi*
rather than it supports it, but we don't emulate it.


>
> And the conclusion from that is that for msi_init to fail silently is
> at the moment the right thing to do.

But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM
if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use
assigned devices. Emulated devices should be created with a specific "msi=off" flag.

Thanks,
Marcel

>
> The only other reason for it to fail is pci config space corruption,
> this probably never happens in practice.
>