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

[Marcel Apfelbaum]

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.
> >
> > 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.
> > Bottom line, as far as I know, almost all PCI devices support legacy
> > interrupts.
> > (an exception is the ivshmem device that requires MSI)
> >
> > > 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.
> >
> > 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.

> 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