Re: [PATCH 2/2] device-assignment: Allow PCI to manage the option ROM

[Alex Williamson <alex.williamson@redhat.com>]

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