Re: [PATCH] slow_map: minor improvements to ROM BAR handling

Michael S. Tsirkin
2009-12-22T12:45:59Z

On Tue, Dec 22, 2009 at 01:05:23PM +0100, Alexander Graf wrote:
> Michael S. Tsirkin wrote:
> > ROM BAR can be handled same as regular BAR:
> > load_option_roms utility will take care of
> > copying it to RAM as appropriate.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > This patch applies on top of agraf's one,
> > it takes care of non-page aligned ROM BARs as well:
> > they mostly are taken care of, we just do not
> > need to warn user about them.
> >
> > hw/device-assignment.c | 20 +++++++++-----------
> > 1 files changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index 000fa61..066fdb6 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -486,25 +486,23 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
> > : PCI_BASE_ADDRESS_SPACE_MEMORY;
> >
> > if (cur_region->size & 0xFFF) {
> > - fprintf(stderr, "PCI region %d at address 0x%llx "
> > - "has size 0x%x, which is not a multiple of 4K. "
> > - "You might experience some performance hit due to that.\n",
> > - i, (unsigned long long)cur_region->base_addr,
> > - cur_region->size);
> > + if (i != PCI_ROM_SLOT) {
> > + fprintf(stderr, "PCI region %d at address 0x%llx "
> > + "has size 0x%x, which is not a multiple of 4K. "
> > + "You might experience some performance hit "
> > + "due to that.\n",
> > + i, (unsigned long long)cur_region->base_addr,
> > + cur_region->size);
> > + }
> > slow_map = 1;
> >
>
> This is wrong. You're setting slow_map = 1 on code that is very likely
> to be executed inside the guest. That doesn't work.

It is? Can you really run code directly from a PCI card?
I looked at BIOS boot specification and it always talks
about shadowing PCI ROMs.

> Better pad the ROM size to page boundary and use the shadow mapping we
> have in place already.

Changing BAR size might break some drivers.
Our BIOS seems to shadow ROM instead of running it directly,
so we should be fine I think?

>
> Alex