Re: [Qemu-devel] [PATCH] xen: avoid tracking the region 0xa0000 - 0xbffff

[Alexander Graf <agraf@suse.de>]

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,<stefano.stabellini@eu.citrix.com>  <stefano.stabellini@eu.citrix.com>  wrote:
>>>>
>>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>>
>>>>> 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<stefano.stabellini@eu.citrix.com>
>>>>> ---
>>>>> 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;