From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH 3/3] qemu-kvm: device assignment: emulate MSI-X mask bits Date: Mon, 8 Nov 2010 08:27:19 +0200 Message-ID: <20101108062718.GA30181@redhat.com> References: <1288851501-4180-1-git-send-email-sheng@linux.intel.com> <201011051902.02425.sheng@linux.intel.com> <20101105135416.GD17667@redhat.com> <201011081317.47253.sheng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org To: Sheng Yang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:28265 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751079Ab0KHG12 (ORCPT ); Mon, 8 Nov 2010 01:27:28 -0500 Content-Disposition: inline In-Reply-To: <201011081317.47253.sheng@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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