From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjdp8-0004ox-Ua for qemu-devel@nongnu.org; Mon, 12 Sep 2016 22:51:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjdp5-0002FA-HX for qemu-devel@nongnu.org; Mon, 12 Sep 2016 22:51:50 -0400 Received: from [59.151.112.132] (port=53572 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjdp5-0002Bl-3e for qemu-devel@nongnu.org; Mon, 12 Sep 2016 22:51:47 -0400 References: <1471944454-13895-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1471944454-13895-2-git-send-email-caoj.fnst@cn.fujitsu.com> <87y42xw8jk.fsf@dusky.pond.sub.org> From: Cao jin Message-ID: <57D769C7.4000503@cn.fujitsu.com> Date: Tue, 13 Sep 2016 10:51:51 +0800 MIME-Version: 1.0 In-Reply-To: <87y42xw8jk.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/5] msix_init: assert programming error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Marcel Apfelbaum , "Michael S. Tsirkin" On 09/12/2016 09:29 PM, Markus Armbruster wrote: > Cao jin writes: > >> The input parameters is used for creating the msix capable device, so >> they must obey the PCI spec, or else, it should be programming error. > > True when the the parameters come from a device model attempting to > define a PCI device violating the spec. But what if the parameters come > from an actual PCI device violating the spec, via device assignment? Before the patch, on invalid param, the vfio behaviour is: error_report("vfio: msix_init failed"); then, device create fail. After the patch, its behaviour is: asserted. Do you mean we should still report some useful info to user on invalid params? Cao jin > > For what it's worth, the new behavior seems consistent with msi_init(), > which is good. > >> CC: Markus Armbruster >> CC: Marcel Apfelbaum >> CC: Michael S. Tsirkin >> Signed-off-by: Cao jin >> --- >> hw/pci/msix.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c >> index 0ec1cb1..384a29d 100644 >> --- a/hw/pci/msix.c >> +++ b/hw/pci/msix.c >> @@ -253,9 +253,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, >> return -ENOTSUP; >> } >> >> - if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) { >> - return -EINVAL; >> - } >> + assert(nentries >= 1 && nentries <= PCI_MSIX_FLAGS_QSIZE + 1); >> >> table_size = nentries * PCI_MSIX_ENTRY_SIZE; >> pba_size = QEMU_ALIGN_UP(nentries, 64) / 8; >> @@ -266,7 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, > /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */ > if ((table_bar_nr == pba_bar_nr && > ranges_overlap(table_offset, table_size, pba_offset, pba_size)) || >> table_offset + table_size > memory_region_size(table_bar) || >> pba_offset + pba_size > memory_region_size(pba_bar) || >> (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) { >> - return -EINVAL; >> + assert(0); >> } > > Instead of > > if (... complicated condition ...) { > assert(0); > } > > let's write > > assert(... negation of the complicated condition ...); > >> >> cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH); > > > . >