From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 1/5] pci-assign: Clean up assigned_dev_pci_read/write_config Date: Thu, 28 Apr 2011 17:06:02 +0200 Message-ID: <4DB9825A.7050701@siemens.com> References: <1304000997.3125.2.camel@x201> <4DB97DDF.7020106@siemens.com> <1304002448.3125.12.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 thoth.sbs.de ([192.35.17.2]:21924 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754241Ab1D1PGK (ORCPT ); Thu, 28 Apr 2011 11:06:10 -0400 In-Reply-To: <1304002448.3125.12.camel@x201> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-04-28 16:54, Alex Williamson wrote: > 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, Yeah, and as we properly restore the config space now, it should be Mostly Harmless. Will update. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux