From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH 2/2] device-assignment: Allow PCI to manage the option ROM
Date: Fri, 8 Oct 2010 00:45:23 +0200
Message-ID: <20101007224523.GB20504@redhat.com>
References: <20101004212311.11167.40425.stgit@s20.home>
 <20101004212630.11167.93029.stgit@s20.home>
 <20101007171858.GA15537@redhat.com>
 <1286472841.3020.56.camel@x201>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, ddutile@redhat.com, chrisw@redhat.com
To: Alex Williamson <alex.williamson@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:58455 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751595Ab0JGWv1 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 7 Oct 2010 18:51:27 -0400
Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o97MpQ8P021624
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <kvm@vger.kernel.org>; Thu, 7 Oct 2010 18:51:27 -0400
Content-Disposition: inline
In-Reply-To: <1286472841.3020.56.camel@x201>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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?