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:46:31 +0200 Message-ID: <4DB98BD7.8080903@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]:34144 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933277Ab1D1Pqj (ORCPT ); Thu, 28 Apr 2011 11:46:39 -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, Thinking about this and the other sub-dword matches again, there is a problem in my approach: I redirect the complete write to the virtualized space once there is an overlap. So a dword write at PCI_INTERRUPT_LINE would not touch the real PCI_MIN_GNT & PCI_MAX_LAT. But writing directly to those without overlap would. The same applies to our reserved space, though this inconsistency is not critical here - but still undesirable. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux