Re: [PATCH 3/4 RFC] x86/p2m: use large pages for MMIO mappings

[Julien Grall <julien.grall@citrix.com>]

On 29/09/15 13:46, Jan Beulich wrote:
>>>> On 29.09.15 at 14:16, <julien.grall@citrix.com> wrote:
>> On 29/09/15 12:44, Jan Beulich wrote:
>>>>>> On 29.09.15 at 13:33, <julien.grall@citrix.com> wrote:
>>>> On 15/09/15 08:34, Jan Beulich wrote:
>>>>> RFC reasons:
>>>>> - ARM side unimplemented (and hence libxc for now made cope with both
>>>>>   models), the main issue (besides my inability to test any change
>>>>>   there) being the many internal uses of map_mmio_regions())
>>>>
>>>> map_mmio_regions is used in ARM to map all the device memory in a guest.
>>>> We expect this function to map everything at once when called during
>>>> DOM0 build and/or when a guest is created (used to map the interrupt
>>>> controller).
>>>>
>>>> I would rather prefer to avoid introducing specific helpers with
>>>> slightly different behavior (i.e one is only mapping N page, the other
>>>> everything).
>>>>
>>>> What about extending map_mmio_regions to take a parameter telling if we
>>>> want to limit the number of mapping in a single invocation?
>>>
>>> Sure an option, albeit something that would be sufficient to be
>>> done in ARM specific code, albeit the only user using variable
>>> length is map_range_to_domain(). All the others, using fixed
>>> lengths up to 32 pages, would implicitly get everything done at
>>> once as long as the threshold is >= 32.
>>
>> While this is the case today, we have different patch series coming up
>> using variable lenght in different place within the ARM code (vGIC,
>> ACPI...).
> 
> Okay.
> 
>> It won't be possible to use map_range_to_domain because it's very
>> specific to build DOM0.
> 
> Sure; I didn't even think of suggesting that.
> 
>> So, I would extend map_mmio_region like that
>>
>> map_mmio_regions(struct domain *d,
>> 		 unsigned long start_gfn,
>> 		 unsigned long nr,
>> 		 unsigned long mfn,
>> 		 unsigned long limit);
>>
>> The limit parameter would be 0 if there is no limit otherwise the
>> maximum of iteration.
> 
> Again, make map_mmio_regions() a wrapper around an ARM-specific
> function with the extra argument. No need to alter common or x86
> code.

TBH, extending the mapp_mmio_region is the best solution.

The name map_mmio_region is very generic and there is no reason we can't
use it in the hypervisor. Adding yet another wrapper will confuse people
and it will be hard for both the reviewer and the developer to know
which one to use.

Regards,

-- 
Julien Grall