Re: [RFC 03/19] xen/arm: follow-up to allow DOM0 manage IRQ and MMIO

[Julien Grall <julien.grall@linaro.org>]

On 18/06/14 21:21, Stefano Stabellini wrote:
> On Mon, 16 Jun 2014, Julien Grall wrote:
>> The commit 33233c2 "arch/arm: domain build: let dom0 access I/O memory of
>> mapped series" fill the iomem_caps to allow DOM0 managing MMIO of mapped
>> device.
>>
>> A device can be disabled (i.e by adding a property status="disabled" in the
>> device tree) because the user may want to passthrough this device to a guest.
>> This will avoid DOM0 loading (and few minutes after unloading) the driver to
>> handle this device.
>>
>> Even though, we don't want to let DOM0 using this device, the domain needs
>> to be able to manage the MMIO/IRQ range. Rework the function map_device
>> (renamed into handle_device) to:
>
> Is that so the toolstack in dom0 could re-assign the device to another
> guest?

Yes.


> In any case it would be good to write down exactly why DOM0 would still
> need to be able to manage the MMIO/IRQ range as a comment in the code.

Will do.

>> +static int handle_device(struct domain *d, struct dt_device_node *dev, bool_t map)
>
> This is confusing.
> If map == true then we get a similar behavior as before. If map ==
> false, what is the function supposed to achieve? Only permit IRQ access?
> Could we split it into a separate function?

I think the name "map" is confusing here. I should rename it to "available".

This function is supposed give access perrmision to IRQ and MMIO access 
(the latter was partially done by Arianna's patch series) and if the 
device is available map it to DOM0.

Splitting in 2 functions would mean duplicate the loop which is quiet 
complex in term of translation (see the interrupt and MMIO translation).

>> @@ -865,10 +888,9 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>>        *  property. Therefore these device doesn't need to be mapped. This
>>        *  solution can be use later for pass through.
>>        */
>> -    if ( !dt_device_type_is_equal(node, "memory") &&
>> -         dt_device_is_available(node) )
>> +    if ( !dt_device_type_is_equal(node, "memory") )
>>       {
>> -        res = map_device(d, node);
>> +        res = handle_device(d, node, dt_device_is_available(node));
>>
>>           if ( res )
>>               return res;
>
> We need a comment here

Hmmm... I don't see what kind of comment I can add here. There is 
already lots of comments explaining handle_device and the previous if.

Regards,

-- 
Julien Grall