From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 12/13] pci-assign: Generic config space access management Date: Tue, 28 Jun 2011 12:10:53 +0300 Message-ID: <4E099A9D.6050803@redhat.com> References: <007a0322c5a4af8a621375c4d2951eee01f2aa05.1309198794.git.jan.kiszka@web.de> <20110628085108.GL10881@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Jan Kiszka , Marcelo Tosatti , kvm@vger.kernel.org, Alex Williamson To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:2161 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756260Ab1F1JK5 (ORCPT ); Tue, 28 Jun 2011 05:10:57 -0400 In-Reply-To: <20110628085108.GL10881@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 06/28/2011 11:51 AM, Michael S. Tsirkin wrote: > On Mon, Jun 27, 2011 at 08:19:55PM +0200, Jan Kiszka wrote: > > -/* There can be multiple VNDR capabilities per device, we need to find the > > - * one that starts closet to the given address without going over. */ > > -static uint8_t find_vndr_start(PCIDevice *pci_dev, uint32_t address) > > +static uint32_t assigned_dev_pci_read_config(PCIDevice *pci_dev, > > + uint32_t address, int len) > > { > > - uint8_t cap, pos; > > + AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev); > > + uint32_t virt_val = pci_default_read_config(pci_dev, address, len); > > + uint32_t real_val, direct_mask; > > > > - for (cap = pos = 0; > > - (pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VNDR, pos)); > > - pos += PCI_CAP_LIST_NEXT) { > > - if (pos<= address) { > > - cap = MAX(pos, cap); > > - } > > + switch (len) { > > + case 4: > > + direct_mask = > > + pci_get_long(assigned_dev->direct_config_read + address); > > + break; > > + case 2: > > + direct_mask = > > + pci_get_word(assigned_dev->direct_config_read + address); > > + break; > > + case 1: > > + direct_mask = > > + pci_get_byte(assigned_dev->direct_config_read + address); > > + break; > > + default: > > + abort(); > > } > > One small issue here is that I think read config is not guaranteed to > get a length aligned address, while pci_get_XXX assume length > alignment. Further, because of alignment you can get beyond > the end of array. > > I don't think it ever happens in practice on the PC, It cannot happen with CF8/CFC configuration (bits 1:0 of CF8 are reserved), but it can happen with mmconfig. > so we can > add an assert, but it's might be easier to look at how > pci_default_read_config handles all these issues: > > uint32_t val = 0; > assert(len == 1 || len == 2 || len == 4); > len = MIN(len, pci_config_size(d) - address); > memcpy(&val, d->config + address, len); > return le32_to_cpu(val); > > You can replace config with direct access, and check full access > with: > > allones = ~0x0; > memcmp(&allones,&val, len) > and full emulation: > zero = 0x0; > memcmp(&zero,&val, len) > > This is not done correctly by current code btw, I think. I think this should be done by the bus code. On the original Pentium, a 4-byte write to address 7 would be issued as a write to address 0 with #BE7 (byte enable 7) asserted, then a write to address 0 with #BE0, #BE1, and #B2 asserted. That is, a one-byte write followed by a 3-byte write. The memory API has support for this (only at the device level at present, but shouldn't be to difficult to implement at the bus level as well). -- error compiling committee.c: too many arguments to function