From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWVTa-0005e4-F2 for qemu-devel@nongnu.org; Tue, 14 Jun 2011 11:24:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QWVTY-0004IC-PD for qemu-devel@nongnu.org; Tue, 14 Jun 2011 11:24:22 -0400 Received: from cantor2.suse.de ([195.135.220.15]:44739 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QWVTY-0004Hi-8M for qemu-devel@nongnu.org; Tue, 14 Jun 2011 11:24:20 -0400 Message-ID: <4DF77D21.4020807@suse.de> Date: Tue, 14 Jun 2011 17:24:17 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1307116614-11775-1-git-send-email-stefano.stabellini@eu.citrix.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] xen: avoid tracking the region 0xa0000 - 0xbffff List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Anthony Perard , "xen-devel@lists.xensource.com" , "qemu-devel@nongnu.org" On 06/14/2011 05:24 PM, Stefano Stabellini wrote: > On Tue, 14 Jun 2011, Alexander Graf wrote: >> On 14.06.2011, at 13:48, Stefano Stabellini wrote: >> >>> On Tue, 14 Jun 2011, Alexander Graf wrote: >>>> On 03.06.2011, at 17:56, wrote: >>>> >>>>> From: Stefano Stabellini >>>>> >>>>> Xen can only do dirty bit tracking for one memory region, so we should >>>>> explicitly avoid trying to track the legacy VGA region between 0xa0000 >>>>> and 0xbffff, rather than trying and failing. >>>>> >>>>> Signed-off-by: Stefano Stabellini >>>>> --- >>>>> xen-all.c | 4 ++++ >>>>> 1 files changed, 4 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/xen-all.c b/xen-all.c >>>>> index 9a5c3ec..1fdc2e8 100644 >>>>> --- a/xen-all.c >>>>> +++ b/xen-all.c >>>>> @@ -218,6 +218,10 @@ static int xen_add_to_physmap(XenIOState *state, >>>>> if (get_physmapping(state, start_addr, size)) { >>>>> return 0; >>>>> } >>>>> + /* do not try to map legacy VGA memory */ >>>>> + if (start_addr>= 0xa0000&& start_addr + size<= 0xbffff) { >>>> I don't quite like the hardcoded range here. What exactly is the issue? The fact that you can only map a single region? Then do a counter and fail when it's> 1. >>> That is what we were doing before: succeeding the first time and >>> failing from the second time on. >>> By "coincidence" the second time was the range 0xa0000-0xbffff so >>> everything worked as expected, but it wasn't obvious why. >>> I am just trying to make sure that one year from now it will be clear >>> just looking at the code why it works. >>> >>> >>>> If you don't want to map the VGA region as memory slot, why not change the actual mapping code in the cirrus adapter? >>> Because I didn't want to introduce any ugly if (xen_enable()) in generic >>> code, if it is that simple to catch the issue from xen specific code. >> Well sure, but 2 years from now yet another region will be introduced that might even be registered before the FB and everyone's puzzled again :). How about you print a warning when anyone tries to map anything after the first map? Or - as Jan suggests - implement multiple regions. >> >> If you prefer, you could even check for the VGA range as "known broken" and only print warnings on others. > I can do that (actually we already do it) but it wouldn't change the > fact that if somebody modifies hw/cirrus_vga.c:map_linear_vram to call > cpu_register_physical_memory_log for the legacy range first, it would > break xen, that is the problem I was trying to solve. > > In order to make the code more reliable, and also catch the scenario > where another region is registered before the framebuffer, we could do > something like this, but it is not very pretty: I agree, but it's probably the most pretty one I've seen so far. I'll let others comment on it too before taking it in. Alex > > > diff --git a/xen-all.c b/xen-all.c > index 9a5c3ec..de1e724 100644 > --- a/xen-all.c > +++ b/xen-all.c > @@ -214,6 +214,7 @@ static int xen_add_to_physmap(XenIOState *state, > unsigned long i = 0; > int rc = 0; > XenPhysmap *physmap = NULL; > + RAMBlock *block; > > if (get_physmapping(state, start_addr, size)) { > return 0; > @@ -221,7 +222,16 @@ static int xen_add_to_physmap(XenIOState *state, > if (size<= 0) { > return -1; > } > + /* only add the vga vram to physmap */ > + QLIST_FOREACH(block,&ram_list.blocks, next) { > + if (!strcmp(block->idstr, "vga.vram")&& block->offset == phys_offset > +&& start_addr> 0xbffff) { > + goto go_physmap; > + } > + } > + return -1; > > +go_physmap: > DPRINTF("mapping vram to %llx - %llx, from %llx\n", start_addr, start_addr + size, phys_offset); > for (i = 0; i< size>> TARGET_PAGE_BITS; i++) { > unsigned long idx = (phys_offset>> TARGET_PAGE_BITS) + i;