From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config Date: Thu, 28 Apr 2011 08:54:08 -0600 Message-ID: <1304002448.3125.12.camel@x201> References: <1304000997.3125.2.camel@x201> <4DB97DDF.7020106@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]:63298 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932429Ab1D1OyL (ORCPT ); Thu, 28 Apr 2011 10:54:11 -0400 In-Reply-To: <4DB97DDF.7020106@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, 2011-04-28 at 16:46 +0200, Jan Kiszka wrote: > On 2011-04-28 16:29, Alex Williamson wrote: > > On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote: > >> Use rages_overlap and proper constants to match the access range against > > ^^^^^ typo - only if you resend > > > >> regions that need special handling. This also fixes yet uncaught > >> high-byte write access to the command register. Moreover, use more > >> constants instead of magic numbers. > >> > >> Signed-off-by: Jan Kiszka > >> --- > >> hw/device-assignment.c | 39 +++++++++++++++++++++++++++++---------- > >> 1 files changed, 29 insertions(+), 10 deletions(-) > >> > >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c > >> index 606d725..3481c93 100644 > >> --- a/hw/device-assignment.c > >> +++ b/hw/device-assignment.c > >> @@ -404,13 +404,20 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, > >> return assigned_device_pci_cap_write_config(d, address, val, len); > >> } > >> > >> - if (address == 0x4) { > >> + if (ranges_overlap(address, len, PCI_COMMAND, 2)) { > >> pci_default_write_config(d, address, val, len); > >> /* Continue to program the card */ > >> } > >> > >> - if ((address >= 0x10 && address <= 0x24) || address == 0x30 || > >> - address == 0x34 || address == 0x3c || address == 0x3d) { > >> + /* > >> + * Catch access to > >> + * - base address registers > >> + * - ROM base address & capability pointer > >> + * - interrupt line & pin > >> + */ > >> + if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || > >> + ranges_overlap(address, len, PCI_ROM_ADDRESS, 8) || > > > > Should this be 5 bytes instead of 8? I'm not sure why we'd add catching > > these reserved fields, but not those immediately after this range. > > Yes, that's asking for clarification: Should we allow direct access to > the complete reserved space or virtualize it? Depending on this, the > proper value should be 5 or 14 (the latter would also save one > ranges_overlap). I vote for 5 here since a cleanup patch shouldn't have behavior changes hidden in it. I don't see any great value in virtualizing reserved bits. It seems like it could only make things not work if a vendor was stupid enough to hide something in there. Thanks, Alex