From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v2 05/21] xen/arm: follow-up to allow DOM0 manage IRQ and MMIO Date: Tue, 9 Sep 2014 14:07:23 +0100 Message-ID: <1410268043.8217.173.camel@kazak.uk.xensource.com> References: <1406818852-31856-1-git-send-email-julien.grall@linaro.org> <1406818852-31856-6-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XRL9O-0002kz-90 for xen-devel@lists.xenproject.org; Tue, 09 Sep 2014 13:08:02 +0000 In-Reply-To: <1406818852-31856-6-git-send-email-julien.grall@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org 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; > + } > } > } >