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

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

On Mon, Oct 11, 2010 at 09:15:52AM -0600, Alex Williamson wrote:
> On Sat, 2010-10-09 at 23:44 +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 08, 2010 at 09:12:52AM -0600, Alex Williamson wrote:
> > > On Fri, 2010-10-08 at 10:40 +0200, Michael S. Tsirkin wrote:
> > > > 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.
> > > > 
> > > 
> > > Ok, so you're looking for something like below.  We can no longer map
> > > the ROM into the guest,
> > > but it's a ROM, so we don't care about speed.
> > 
> > Why can't we map ROM? Map full pages, leave 0xff unmapped.
> > The reason there will be such is because BAR is power of 2.
> 
> If I understand correctly, you're suggesting we round the ROM up to a
> power of two, allocate a full buffer to back that, and map that to the
> guest.

No, I suggested rounding up to full pages.

>  If the physical device has a larger ROM BAR, the remainder is
> pointed to a set of read functions that return 0xff and probably never
> get called.

Yes.

> > > Here's the big problem... it breaks migration.  The ramblock live
> > > migration code isn't going to deal well with migration from a VM with a
> > > BAR sized ramblock to a ROM sized ramblock (likewise the reverse).
> > 
> > You mean cross-version migration? Otherwise, why would not both
> > sides be ROM sized?
> 
> Yes, cross-version migration, though probably not an issue with the
> above since it doesn't change the size of existing emulated device ROMs.
> 
> > >  So
> > > we could do it for passthrough devices since they can't migrate anyway,
> > > but then we have to go back to separate code to handle assigned device
> > > ROMs vs emulated device ROMs.
> > 
> > I think this is based on the assumption we do not map ROM.
> > If we do map it, then most of the code is still same,
> > just add 0xff handling for pages after end of ROM.
> > These typically are unaccessed anyway.
> 
> Not on the no mapping assumption, but the assumption that you were
> looking to use the minimum buffer to back the ROM.  If we agree that
> it's ok to waste memory rounding the ROM up to a power of two, then
> things work out a little better, though I'm still dubious whether the
> memory savings is worth the code necessary to potentially handle the ROM
> as two discrete pieces.  Thanks,
> 
> Alex

True. Note this optimization is not there in existing code,
so it's definitely not urgent to implement this - just
something to keep in mind.

-- 
MST