Re: [PATCH 3/3] qemu-kvm: device assignment: emulate MSI-X mask bits

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 08, 2010 at 01:17:47PM +0800, Sheng Yang wrote:
> > > > > > > @@ -1250,10 +1297,16 @@ static void
> > > > > > > assigned_dev_update_msix(PCIDevice *pci_dev, int enable_msix)
> > > > > > > 
> > > > > > >          fprintf(stderr, "assigned_dev_update_msix: MSI-X
> > > > > > >          entries_max_nr == 0"); return;
> > > > > > >      
> > > > > > >      }
> > > > > > > 
> > > > > > > +    /*
> > > > > > > +     * Guest may try to enable MSI-X before setting MSI-X entry
> > > > > > > done, so +     * let's wait until guest unmask the entries.
> > > > > > > +     */
> > > > > > 
> > > > > > Well it can also set up any number of entries, enable msix then
> > > > > > set up more entries. Now what?
> > > > > 
> > > > > It's the same. If it want to set up more entries, it have to unmask
> > > > > them. Then we would get them.
> > > > 
> > > > Why can't we handle the first enable in the same way?
> > > 
> > > Don't understand the question, but I guess the answer is
> > > pci_enable_msix().
> > 
> > pci_enable_msix is an internal kernel API to enable msix, isn't it?
> > We are discussing qemu patch here.
> 
> OK, I understand the question now. The others entries would be enabled with 
> unmask, but not very first ones. Guest can unmask the entries first, then enable 
> MSI-X. We should scan the table when MSI-X enabled, that's why the first one is 
> different from others.

I see. I think the comment mislead me: you really mean
'Guest can unmask MSI-X entries when MSI-X is disabled,
 don't do anything until MSI-X is enabled'.



> > > > > > >      entries_max_nr); if (entries_nr == 0) {
> > > > > > > 
> > > > > > > +#ifndef KVM_CAP_DEVICE_MSIX_MASK
> > > > > > > 
> > > > > > >          if (enable_msix)
> > > > > > >          
> > > > > > >              fprintf(stderr, "MSI-X entry number is zero!\n");
> > > > > > 
> > > > > > And what happens then?
> > > > > 
> > > > > MSI-X can't work for old ones without MSIX mask support.
> > > > 
> > > > Old ones?
> > > 
> > > Old userspace without KVM_CAP_DEVICE_MSIX_MASK
> > > 
> > > > > Reload the guest module
> > > > > may help.
> > > > 
> > > > Guest might not have any concept of modules.
> > > 
> > > Just an workaround. Not a suggestion method.
> > 
> > Look, if there's some failure mode we need a way to recover,
> > or to make it very unlikely. If this is guest doing something
> > illegal let's add a comment explaining what it is and why
> > it's illegal. If this is legal, why the print out?
> 
> It's unsupported in the old QEmu.


I think I have it. The comment you want is:

/* Error: number of MSI-X entries is zero. Using this device
   requires KVM_CAP_DEVICE_MSIX_MASK support in kvm
   which is missing.
 */

As I said prebiously, we must check runtime capabilities
for this, ifdef is not enough to know kernel supports a feature
because there's no dependency between kernel and qemu packages.

> I can change the comment, but I do think it's 
> good to let user know something important is wrong. 

stderr is really there for developers.
You can't expect the user to understand this message.

> Maybe it's better to put it into DEBUG.
> --
> regards
> Yang, Sheng
> 
> > 
> > > --
> > > regards
> > > Yang, Sheng
> > > 
> > > > > --
> > > > > regards
> > > > > Yang, Sheng