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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
