|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 01/11] arch, arm: domain build: let dom0 access I/O memory of mapped devices
On 04/29/2014 03:44 PM, Julien Grall wrote:
> On 04/29/2014 01:37 PM, Julien Grall wrote:
>> Hi Arianna,
>>
>> On 04/21/2014 02:44 PM, Arianna Avanzini wrote:
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 187e071..1802b6e 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -11,6 +11,7 @@
>>> #include <xen/device_tree.h>
>>> #include <xen/libfdt/libfdt.h>
>>> #include <xen/guest_access.h>
>>> +#include <xen/iocap.h>
>>> #include <asm/setup.h>
>>> #include <asm/platform.h>
>>> #include <asm/psci.h>
>>> @@ -740,6 +741,16 @@ static int map_device(struct domain *d, const struct
>>> dt_device_node *dev)
>>> DPRINT("addr %u = 0x%"PRIx64" - 0x%"PRIx64"\n",
>>> i, addr, addr + size - 1);
>>>
>>> + res = iomem_permit_access(d, paddr_to_pfn(addr & PAGE_MASK),
>>> + paddr_to_pfn(PAGE_ALIGN(addr + size -
>>> 1)));
>>> + if ( res )
>>> + {
>>> + printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
>>> + d->domain_id,
>>> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>>> + return res;
>>> + }
>>> res = map_mmio_regions(d, addr & PAGE_MASK,
>>> PAGE_ALIGN(addr + size) - 1,
>>> addr & PAGE_MASK);
>>>
>>
>> I though a bit more about this patch. If the device is disabled (i.e
>> status="disabled"), Xen doesn't call map_device.
>> Futhermore, in your use case (e.g with iomem=) you might want to map
>> memory that is not describe to the device tree.
>>
>> I would either:
>> 1) give a full access to the I/O range
>> 2) give a full access to the I/O range except on the RAM region
>>
>> IHMO, the second solution might be better but I don't know if it's easy
>> to implement it.
>
> I quickly wrote a follow-up of the series (see below). We can either merge
> in this patch or I can carry it with the device passthrough patch series.
>
For me it is OK that the follow-up is kept in the device passthrough patch
series, if it's fine for you.
If you agree, I'll update the commit description of the 0001 patch in the
memory_mapping patchset and try to explain its limitations with regard to the
scenario you highlighted.
Also, sorry for the delay; I'll be surely sending a v7 of the memory_mapping
series in the next few days.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 2f6ffe9..3498549 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -717,9 +717,14 @@ static int make_timer_node(const struct domain *d, void
> *fdt,
> return res;
> }
>
> -/* Map the device in the domain */
> -static int map_device(struct domain *d, struct kernel_info *kinfo,
> - struct dt_device_node *dev)
> +/* For a specific device node :
> + * - Give access permission to the guest
> + * When the device is available:
> + * - Assign the device to the guest if it's protected by an IOMMU
> + * - Map the IRQs and iomem regions to DOM0
> + */
> +static int handle_device(struct domain *d, struct kernel_info *kinfo,
> + struct dt_device_node *dev, bool_t map)
> {
> unsigned int nirq;
> unsigned int naddr;
> @@ -734,7 +739,7 @@ static int map_device(struct domain *d, struct
> kernel_info *kinfo,
>
> DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr);
>
> - if ( dt_device_is_protected(dev) )
> + if ( dt_device_is_protected(dev) && map )
> {
> DPRINT("%s setup iommu\n", dt_node_full_name(dev));
> res = iommu_assign_dt_device(d, dev);
> @@ -778,12 +783,15 @@ static int map_device(struct domain *d, struct
> kernel_info *kinfo,
> }
>
> DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
> - res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> - if ( res )
> + if ( map )
> {
> - printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> - irq.irq, d->domain_id);
> - return res;
> + res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> + irq.irq, d->domain_id);
> + return res;
> + }
> }
> }
>
> @@ -811,17 +819,21 @@ static int map_device(struct domain *d, struct
> kernel_info *kinfo,
> addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> return res;
> }
> - res = map_mmio_regions(d,
> - paddr_to_pfn(addr & PAGE_MASK),
> - paddr_to_pfn_aligned(addr + size) - 1,
> - paddr_to_pfn(addr & PAGE_MASK));
> - if ( res )
> +
> + if ( map )
> {
> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> - " - 0x%"PRIx64" in domain %d\n",
> - addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
> - d->domain_id);
> - return res;
> + res = map_mmio_regions(d,
> + paddr_to_pfn(addr & PAGE_MASK),
> + paddr_to_pfn_aligned(addr + size) - 1,
> + paddr_to_pfn(addr & PAGE_MASK));
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> + " - 0x%"PRIx64" in domain %d\n",
> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1,
> + d->domain_id);
> + return res;
> + }
> }
> }
>
> @@ -902,10 +914,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, kinfo, node);
> + res = handle_device(d, kinfo, node, dt_device_is_available(node));
>
> if ( res )
> return res;
>
>
>
--
/*
* Arianna Avanzini
* avanzini.arianna@xxxxxxxxx
* 73628@xxxxxxxxxxxxxxxxxxx
*/
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |