From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH 2/2] device-assignment: Allow PCI to manage the option ROM Date: Thu, 07 Oct 2010 22:02:25 -0600 Message-ID: <1286510545.3016.36.camel@x201> References: <20101004212311.11167.40425.stgit@s20.home> <20101004212630.11167.93029.stgit@s20.home> <20101007171858.GA15537@redhat.com> <1286472841.3020.56.camel@x201> <20101007224523.GB20504@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, ddutile@redhat.com, chrisw@redhat.com To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37376 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750776Ab0JHEC1 (ORCPT ); Fri, 8 Oct 2010 00:02:27 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o9842RYC017416 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 8 Oct 2010 00:02:27 -0400 In-Reply-To: <20101007224523.GB20504@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, 2010-10-08 at 00:45 +0200, Michael S. Tsirkin wrote: > On Thu, Oct 07, 2010 at 11:34:01AM -0600, Alex Williamson wrote: > > On Thu, 2010-10-07 at 19:18 +0200, Michael S. Tsirkin wrote: > > > On Mon, Oct 04, 2010 at 03:26:30PM -0600, Alex Williamson wrote: > > > > --- a/hw/device-assignment.c > > > > +++ b/hw/device-assignment.c > > ... > > > > @@ -1644,58 +1621,64 @@ void add_assigned_devices(PCIBus *bus, const char **devices, int n_devices) > > > > */ > > > > static void assigned_dev_load_option_rom(AssignedDevice *dev) > > > > { > > > > - int size, len, ret; > > > > - void *buf; > > > > + char name[32], rom_file[64]; > > > > FILE *fp; > > > > - uint8_t i = 1; > > > > - char rom_file[64]; > > > > + uint8_t val; > > > > + struct stat st; > > > > + void *ptr; > > > > + > > > > + /* If loading ROM from file, pci handles it */ > > > > + if (dev->dev.romfile || !dev->dev.rom_bar) > > > > + return; > > > > > > > > snprintf(rom_file, sizeof(rom_file), > > > > "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/rom", > > > > dev->host.seg, dev->host.bus, dev->host.dev, dev->host.func); > > > > > > > > - if (access(rom_file, F_OK)) > > > > + if (stat(rom_file, &st)) { > > > > return; > > > > + } > > > > > > > > > > Just a note that stat on the ROM sysfs file returns window size, > > > not the ROM size. So this allocates more ram than really necessary for > > > ROM. Real size is returned by fread. > > > > > > Do we care? > > > > That was my intention with using stat. I thought that by default the > > ROM BAR should match physical hardware, so even if the contents could be > > rounded down to a smaller size, we maintain the size of the physical > > device. To use the minimum size, the contents could be extracted using > > pci-sysfs and passed with the romfile option, or the ROM could be > > disabled altogether with the rombar=0 option. Sound reasonable? > > Thanks, > > > > Alex > > For BAR size yes, but we do not need the buffer full of 0xff as it is > never accessed: let's have buffer size match real ROM, avoid wasting > memory: this can come up to megabytes easily. > Makes sense? I tend to doubt that hardware vendors are going to waste money putting seriously oversized eeproms on devices. It does seem pretty typical to find graphics cards with 128K ROM BARs where the actual ROM squeezes just under 64K, but that's a long way from megabytes of wasted memory. The only device I have with a ROM BAR in the megabytes is an 82576, but it comes up as an invalid rom through pci-sysfs, so we skip it. I assume that just means someone was lazy and didn't bother to fuse a transistor that disables the ROM BAR, leaving it at it's maximum aperture w/ no eeprom to back it. Anyone know? Examples to the contrary welcome. So I think the question comes down to whether there's any value to trying to exactly mimic the resource layout of the device. I'm doubtful that there is, but at the potential cost of 10-100s of KBs of memory, I thought it might be worthwhile. If you feel strongly otherwise, I'll follow-up with a patch to size it by the actual readable contents. Thanks, Alex