Re: [PATCH v2 05/21] xen/arm: follow-up to allow DOM0 manage IRQ and MMIO

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

Hi Ian,

On 09/09/14 06:07, Ian Campbell wrote:
> On Thu, 2014-07-31 at 16:00 +0100, Julien Grall wrote:
>
> Please make the subject line reflect what this patches function is, not
> that it is a follow up to some other patch.

>> 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.
>
> "fills in ... to allow DOM0 to manage MMIO of mapped devices" (I think
> that is what was meant")

Yes, I will change in the next version.

>>
>> 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
>
> s/after/later/
>
>> handle this device.
>>
>> Even though, we don't want to let DOM0 using this device, the domain needs
>
> "use". And I think s/the domain/it/ since you are referring to DOM0
> (just mentioned, so "it" is fine in the context) not the domain as in
> the guest.
>
>> to be able to manage the MMIO/IRQ range. This will allow the toolstack
>> to map MMIO/IRQ to a guest explicitly via "iomem" and "irqs" VM config
>> property or implicitly by passthrough a device.
>
> "by passing through"
>
>> Rework the function map_device (renamed into handle_device) to:
>>
>> * For a given device node:
>>      - Give permission to manage IRQ/MMIO for this device
>>      - Retrieve the IRQ configuration (i.e edge/level) from the device
>>      tree
>> * For available device (i.e status != disabled in the DT)
>>      - Assign the device to the guest if it's protected by an IOMMU
>>      - Map the IRQs and MMIOs regions to the guest
>
> So based on that I think a suitable $subject would be something like
> "give domain 0 permission to manage IRQ/MMIO for disabled devices" or
> something like that.

It's a follow-up in the sense that it improves Arianna patch to handle 
MMIO and IRQ.

Your suggestion for the title sounds good. I will use it.

> Is it a good idea to override status="disabled" in this way or should be
> add our own xen,passthrough as a boolean DT property?
>
> My concern is that we are also doing this for devices which are disabled
> for some other reason (buggy? security sensitive? not present on this
> partiocular board etc). It also leaves no way to mark a device as "not
> for any domain, including dom0 or passthrough". Quite a few .dtsi files
> define loads of stuff as disabled and then let the board files override
> what they actually have as okay.
>
> So I think keeping status="disabled" as more of a firmware thing and
> having our own for devices which are to be passed through makes sense.

Using status="disabled" was an easy solution because Linux, and most of 
the other OS, knows the device should not be used by DOM0.

The current solution doesn't map the device into DOM0 memory but the 
device is still described in the device tree. That would let us the 
possibility to retrieve from DOM0 the properties/compatible of the 
device via a driver (and would avoid the bunch of hypercalls introduced 
later).

With the new property "xen,passthrough", we would have to remove the 
node from DOM0, or teach DOM0 that the device should not be used.

Overall, I don't think dropping the node in DOM0 device tree will impact 
it. If it's the case that would mean the device should not be 
passthrough to another guest. So I will give a look to introduce this 
new property. Shall I send a patch to the device tree bindings ML?

BTW, I don't think the new property should be a boolean. Use only the 
name should be enough here.

> Since I expect the naming of the property isn't going to have a major
> impact on the code itself outside of a few predicate functions I took a
> look and it looks good, apart from a few nits which I'll mention.
>
>> @@ -957,7 +965,7 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>           }
>>       }
>>
>> -    /* Map IRQs */
>> +    /* Give permission and  map IRQs */
>
> double space.
>
>>       for ( i = 0; i < nirq; i++ )
>>       {
>>           res = dt_device_get_raw_irq(dev, i, &rirq);
>> @@ -990,16 +998,28 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>           irq = res;
>>
>>           DPRINT("irq %u = %u\n", i, irq);
>> -        res = route_irq_to_guest(d, irq, dt_node_name(dev));
>> +
>> +        res = irq_permit_access(d, irq);
>>           if ( res )
>>           {
>> -            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
>> -                   irq, d->domain_id);
>> +            printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
>> +                   d->domain_id, irq);
>>               return res;
>>           }
>> +
>> +        if ( available )
>
> if ( !avaiable ) \n continue would save you an indentation level.

Ok.

>> +        {
>> +            res = route_irq_to_guest(d, irq, dt_node_name(dev));
>> +            if ( res )
>> +            {
>> +                printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
>> +                       irq, d->domain_id);
>> +                return res;
>> +            }
>> +        }
>>       }
>>
>> -    /* Map the address ranges */
>> +    /* Give permission and map MMIOs */
>
> Permissions are now given above, not below, aren't they?

Only permission for IRQ. The MMIO ones are given few lines below.

>>       for ( i = 0; i < naddr; i++ )
>>       {
>>           res = dt_device_get_address(dev, i, &addr, &size);
>> @@ -1023,17 +1043,21 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>                      addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>>               return res;
>>           }
>> -        res = map_mmio_regions(d,
>> -                               paddr_to_pfn(addr & PAGE_MASK),
>> -                               DIV_ROUND_UP(size, PAGE_SIZE),
>> -                               paddr_to_pfn(addr & PAGE_MASK));
>> -        if ( res )
>> +
>> +        if ( available )
>
> if ( !avaiable ) \n continue again.

Ok.

Regards,

-- 
Julien Grall