From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH] slow_map: minor improvements to ROM BAR handling Date: Tue, 22 Dec 2009 17:19:11 +0200 Message-ID: <20091222151911.GC18541@redhat.com> References: <20091222111044.GA16000@redhat.com> <4B30B603.8080707@suse.de> <20091222124311.GD16165@redhat.com> <4B30CAF2.4040409@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: avi@redhat.com, kvm@vger.kernel.org To: Alexander Graf Return-path: Received: from mx1.redhat.com ([209.132.183.28]:20472 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753465AbZLVPWA (ORCPT ); Tue, 22 Dec 2009 10:22:00 -0500 Content-Disposition: inline In-Reply-To: <4B30CAF2.4040409@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Dec 22, 2009 at 02:34:42PM +0100, Alexander Graf wrote: > 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 > >>> --- > >>> > >>> 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 Correct: I think it's made readonly down the road with mprotect, so attempt to do so will crash qemu :) -- MST