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

[Alexander Graf <agraf@xxxxxxx>]

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@xxxxxxxxxxxxx> 
>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>> 
>>> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>>> 
>>> 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@xxxxxxxxxxxxx>
>>> ---
>>> 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.


Alex


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel