| Message ID | 1406818852-31856-6-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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") > > 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. 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. 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. > + { > + 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? > 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. > { > - 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; > + } > } > } >
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,
On Thu, 2014-09-11 at 15:32 -0700, Julien Grall wrote: > The current solution doesn't map the device into DOM0 memory but the > device is still described in the device tree. I had some thoughts on that in the thread on the 00/21 mail BTW. > 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. You would translate it into status="disabled" for dom0, that's all, no need to remove anything. > > 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. That's what boolean means in DT context, a name without a value. > > >> + { > >> + 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. Ah, right. Ian.
Hi Ian, On 12/09/14 03:13, Ian Campbell wrote: > On Thu, 2014-09-11 at 15:32 -0700, Julien Grall wrote: >> The current solution doesn't map the device into DOM0 memory but the >> device is still described in the device tree. > > I had some thoughts on that in the thread on the 00/21 mail BTW. > >> 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. > > You would translate it into status="disabled" for dom0, that's all, no > need to remove anything. I didn't think about this. So this "xen.passthrough" property will be only used by Xen. I will implement it for the next version. >> >> 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. > > That's what boolean means in DT context, a name without a value. Oh right. I had in mind the status="disabled" stuff. >> > >>>> + { >>>> + 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. > > Ah, right. I can update the comment to make more clear. Regards,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 867dfd4..47d114f 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -929,8 +929,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) { unsigned int nirq; unsigned int naddr; @@ -939,13 +945,15 @@ static int map_device(struct domain *d, struct dt_device_node *dev) unsigned int irq; struct dt_raw_irq rirq; u64 addr, size; + bool_t available = dt_device_is_available(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 available = %d nirq = %d naddr = %u\n", dt_node_full_name(dev), + available, nirq, naddr); - if ( dt_device_is_protected(dev) ) + if ( dt_device_is_protected(dev) && available ) { DPRINT("%s setup iommu\n", dt_node_full_name(dev)); res = iommu_assign_dt_device(d, dev); @@ -957,7 +965,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); @@ -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 ) + { + 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); @@ -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 ) { - 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; + } } } @@ -1110,15 +1134,10 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo, * Some device doesn't need to be mapped in Xen: * - Memory: the guest will see a different view of memory. It will * be allocated later. - * - Disabled device: Linux is able to cope with status="disabled" - * 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); 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. 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. 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> --- Changes in v2: - Improve commit message - Remove the third parameter of handle_device and get the status of the device directly in the function. --- xen/arch/arm/domain_build.c | 71 +++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 26 deletions(-)