From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH 3/5] pci-assign: Fix dword read at PCI_COMMAND Date: Thu, 28 Apr 2011 08:51:06 -0600 Message-ID: <1304002266.3125.9.camel@x201> References: <0fe595f76c6b3d72df01a575b26b754f6e411021.1303981185.git.jan.kiszka@siemens.com> 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: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:18862 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760563Ab1D1OvK (ORCPT ); Thu, 28 Apr 2011 10:51:10 -0400 In-Reply-To: <0fe595f76c6b3d72df01a575b26b754f6e411021.1303981185.git.jan.kiszka@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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, Alex > + } > + > if (!pci_dev->cap.available) { > /* kill the special capabilities */ > if (address == PCI_COMMAND && len == 4) {