| Message ID | 535FACA1.5030702@linaro.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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; > > >
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;