| Message ID | 1402935486-29136-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? 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. > * 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 > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/domain_build.c | 66 ++++++++++++++++++++++++++++--------------- > 1 file changed, 44 insertions(+), 22 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index c3783cf..6a711cc 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -680,8 +680,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 dt_device_node *dev) > +/* For a given device node: > + * - Give permission to the guest to manage IRQ and MMIO range > + * - Retrieve the IRQ configuration (i.e edge/level) from device tree > + * 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 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? > { > unsigned int nirq; > unsigned int naddr; > @@ -694,9 +700,10 @@ static int map_device(struct domain *d, struct dt_device_node *dev) > nirq = dt_number_of_irq(dev); > naddr = dt_number_of_address(dev); > > - DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr); > + DPRINT("%s map = %d nirq = %d naddr = %u\n", dt_node_full_name(dev), > + map, 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); > @@ -708,7 +715,7 @@ static int map_device(struct domain *d, struct dt_device_node *dev) > } > } > > - /* Map IRQs */ > + /* Give permission and map IRQs */ > for ( i = 0; i < nirq; i++ ) > { > res = dt_device_get_raw_irq(dev, i, &rirq); > @@ -741,16 +748,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 ( map ) > + { > + 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 */ > for ( i = 0; i < naddr; i++ ) > { > res = dt_device_get_address(dev, i, &addr, &size); > @@ -774,17 +793,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 ( 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), > + DIV_ROUND_UP(size, PAGE_SIZE), > + 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; > + } > } > } > > @@ -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
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,
On Wed, 2014-06-18 at 21:32 +0100, Julien Grall wrote: > >> @@ -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. I'd be inclined to push the dt_device_is_available call down into the handle_function. Otherwise I would go with res = make_deevice_available(node) if (!res && dt_...available(node) res = map_device(node). Ian
On 07/03/2014 12:02 PM, Ian Campbell wrote: > On Wed, 2014-06-18 at 21:32 +0100, Julien Grall wrote: >>>> @@ -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. > > I'd be inclined to push the dt_device_is_available call down into the > handle_function. > > Otherwise I would go with > res = make_deevice_available(node) What will do make_device_available? > if (!res && dt_...available(node) > res = map_device(node). AFAIU, it would means to duplicate the loop to get interrupt/MMIO twice which I think it's stupid. So, I prefer to push the dt_device_is_available call down into the handle_function. Regards,
On Thu, 2014-07-03 at 12:23 +0100, Julien Grall wrote: > On 07/03/2014 12:02 PM, Ian Campbell wrote: > > On Wed, 2014-06-18 at 21:32 +0100, Julien Grall wrote: > >>>> @@ -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. > > > > I'd be inclined to push the dt_device_is_available call down into the > > handle_function. > > > > Otherwise I would go with > > res = make_deevice_available(node) > > What will do make_device_available? The tweaking of the iocaps arrays etc (perhaps a badly chosen name given the dt_....available). > > if (!res && dt_...available(node) > > res = map_device(node). > > AFAIU, it would means to duplicate the loop to get interrupt/MMIO twice > which I think it's stupid. > > So, I prefer to push the dt_device_is_available call down into the > handle_function. Sure. Ian.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index c3783cf..6a711cc 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -680,8 +680,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 dt_device_node *dev) +/* For a given device node: + * - Give permission to the guest to manage IRQ and MMIO range + * - Retrieve the IRQ configuration (i.e edge/level) from device tree + * 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 dt_device_node *dev, bool_t map) { unsigned int nirq; unsigned int naddr; @@ -694,9 +700,10 @@ static int map_device(struct domain *d, struct dt_device_node *dev) nirq = dt_number_of_irq(dev); naddr = dt_number_of_address(dev); - DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr); + DPRINT("%s map = %d nirq = %d naddr = %u\n", dt_node_full_name(dev), + map, 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); @@ -708,7 +715,7 @@ static int map_device(struct domain *d, struct dt_device_node *dev) } } - /* Map IRQs */ + /* Give permission and map IRQs */ for ( i = 0; i < nirq; i++ ) { res = dt_device_get_raw_irq(dev, i, &rirq); @@ -741,16 +748,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 ( map ) + { + 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 */ for ( i = 0; i < naddr; i++ ) { res = dt_device_get_address(dev, i, &addr, &size); @@ -774,17 +793,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 ( 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), + DIV_ROUND_UP(size, PAGE_SIZE), + 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; + } } } @@ -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;
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: * 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 Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/domain_build.c | 66 ++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 22 deletions(-)