On 28/09/15 15:16, Qais Yousef wrote:
> On 09/28/2015 11:55 AM, Marc Zyngier wrote:
>> On 28/09/15 11:11, Markos Chandras wrote:
>>
>>> +
>>> + pfn = (gic_base_addr + USM_VISIBLE_SECTION_OFS) >> PAGE_SHIFT;
>>> + return io_remap_pfn_range(vma, base, pfn, size,
>>> + pgprot_noncached(PAGE_READONLY));
>>
>> - Does this code have to be in the irqchip driver? It really feels out
>> of place, and I'd rather see a function that returns the mappable range
>> to the VDSO code, where the mapping would occur.
>>
>
>
> I don't think it's a good idea either for the VDSO code to know about
> gic_base_addr. Maybe this function could be split to return the pfn and
> let the caller do io_remap_pfn_range(). Though I think it's nice to have
> it all there. USM stands for USer Mode - GIC wants to make some stuff
> visible to user mode and it puts them in that special section. So it
> makes sense to do it all there IMO.
Maybe I wasn't clear enough. My suggestion was to expose this in
the VDSO setup code:
@@ -90,8 +133,15 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
goto out;
}
+ /* Map GIC user page. */
+ if (gic_size) {
+ ret = gic_map_user_section(vma, base, gic_size);
+ if (ret)
+ goto out;
+ }
+
This could easily be written as:
if (gic_size) {
struct resource gic_res;
ret = gic_get_usm_range(&gic_res);
if (ret)
goto out;
... and perform the mapping here...
}
You can also rewrite the hunks above to actually get the present/size
information from the GIC. And if you have DT, you should be able to
directly find the memory region there, without involving the GIC
driver at all.
I don't really fancy having some userspace visible stuff in an
interrupt controller driver, and I tend to find it nicer to split
the responsabilities: the VDSO code deals with the userspace mapping,
and the interrupt controller deals with interrupts.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
