From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC 03/19] xen/arm: follow-up to allow DOM0 manage IRQ and MMIO Date: Wed, 18 Jun 2014 21:32:35 +0100 Message-ID: <53A1F763.1030102@linaro.org> References: <1402935486-29136-1-git-send-email-julien.grall@linaro.org> <1402935486-29136-4-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WxMX9-0008Hh-3a for xen-devel@lists.xenproject.org; Wed, 18 Jun 2014 20:32:39 +0000 Received: by mail-wg0-f51.google.com with SMTP id x12so1354188wgg.34 for ; Wed, 18 Jun 2014 13:32:37 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org 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, -- Julien Grall