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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 07, 2010 at 10:02:25PM -0600, Alex Williamson wrote:
> 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

I actually agree sizing ROM BAR exactly the same as the device
is a good idea. I just thought we can save the extra memory
by not allocating the RAM in question, and writing code
to return 0xff on reads within the BAR but outside ROM.
And no, I don't feel strongly about this optimization.

-- 
MST