From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 3/5] pci-assign: Fix dword read at PCI_COMMAND Date: Thu, 28 Apr 2011 17:36:16 +0200 Message-ID: <4DB98970.5030303@siemens.com> References: <0fe595f76c6b3d72df01a575b26b754f6e411021.1303981185.git.jan.kiszka@siemens.com> <1304002266.3125.9.camel@x201> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" To: Alex Williamson Return-path: Received: from david.siemens.de ([192.35.17.14]:27182 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754951Ab1D1Pg0 (ORCPT ); Thu, 28 Apr 2011 11:36:26 -0400 In-Reply-To: <1304002266.3125.9.camel@x201> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-04-28 16:51, Alex Williamson wrote: > On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote: >> If we emulate the command register, we must only read its content from >> the shadow config space. For dword read of both PCI_COMMAND and >> PCI_STATUS, at least the latter must be read from the device. >> >> For simplicity reasons and as the code path is not considered >> performance critical for the affected SRIOV devices, the fix performes >> device access to the command word unconditionally, even if emulation is >> enabled and only that word is read. >> >> Signed-off-by: Jan Kiszka >> --- >> hw/device-assignment.c | 8 +++++--- >> 1 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c >> index f37f108..ee81434 100644 >> --- a/hw/device-assignment.c >> +++ b/hw/device-assignment.c >> @@ -482,14 +482,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, >> /* >> * Catch access to >> * - vendor & device ID >> - * - command register (if emulation needed) >> * - base address registers >> * - ROM base address & capability pointer >> * - interrupt line & pin >> */ >> if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || >> - (pci_dev->need_emulate_cmd && >> - ranges_overlap(address, len, PCI_COMMAND, 2)) || >> ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || >> ranges_overlap(address, len, PCI_ROM_ADDRESS, 8) || >> ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { >> @@ -521,6 +518,11 @@ do_log: >> DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", >> (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); >> >> + if (pci_dev->need_emulate_cmd) { >> + val = merge_bits(val, pci_default_read_config(d, PCI_COMMAND, 2), >> + address, len, PCI_COMMAND, 0xffff); > > Shouldn't this be pci_default_read_config(d, address, len)? I think > what you have works since PCI_COMMAND is at the start of a dword, but it > violates the merge_bits() assumption that val and mval are from the same > address with the same length. Thanks, This is actually a real issue, reading a byte from PCI_COMMAND+1 is broken. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux