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

Alexander Graf
2009-12-22T13:34:44Z

Michael S. Tsirkin wrote:
> 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.
>

I'm not sure the BIOS is the only one executing ROMs. If it is, then I'm
good with the change.
Maybe it'd make sense to also add a read only flag so we don't
accidently try to write to the ROM region with slow_map.

Alex