From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 19/33] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq Date: Tue, 31 Mar 2015 12:11:32 +0100 Message-ID: <1427800292.2115.73.camel@citrix.com> References: <1426793399-6283-1-git-send-email-julien.grall@linaro.org> <1426793399-6283-20-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 1Ycu5A-0007ab-BM for xen-devel@lists.xenproject.org; Tue, 31 Mar 2015 11:11:44 +0000 In-Reply-To: <1426793399-6283-20-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, Jan Beulich , stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On Thu, 2015-03-19 at 19:29 +0000, Julien Grall wrote: > On x86, an IRQ is assigned in 2 steps to an HVM guest: > - The toolstack is calling PHYSDEVOP_map_pirq in order to create a > guest PIRQ (IRQ bound to an event channel) > - The emulator (QEMU) is calling DOMCTL_bind_pt_irq in order to > bind the IRQ > > On ARM, there is no concept of PIRQ as the IRQ can be assigned to a > virtual IRQ using the interrupt controller. > > It's not clear if we will need 2 different hypercalls on ARM to assign > IRQ and, for now, only the toolstack will manage IRQ. > > In order to avoid re-using a fixed ABI hypercall (PHYSDEVOP_*) for a > different purpose and allow us more time to figure out the right out, "figure out the right way" > only DOMCTL_{,un}bind_pt_pirq is implemented on ARM. > > The DOMCTL is extended with a new type PT_IRQ_TYPE_SPI and only IRQ == > vIRQ (i.e machine_irq == spi) is supported. > > Concerning XSM, even if ARM is using one hypercall rather than 2, the > resulting check is nearly the same. > > XSM PHYSDEVOP_map_pirq: > 1) Check if the current domain can add resource to the domain > 2) Check if the current domain has permission to add the IRQ > 3) Check if the target domain has permission to use the IRQ > > XSM DOMCTL_bind_pirq_irq: > 1) Check if the current domain can add resource to the domain > 2) Check if the current domain has permission to bind the IRQ > 3) Check if the target domain has permission to use the IRQ > > Rather than checking that the current domain can both add and bind the > IRQ, we only check the bind permission. I think this is not a big deal > because we don't have emulator on ARM and therefore no disaggregation is > required. Is this because we don't have the "add" concept on arm? > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index 579d266..8243b70 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -1764,7 +1764,7 @@ int xc_domain_bind_pt_irq( > uint8_t bus, > uint8_t device, > uint8_t intx, > - uint8_t isa_irq) > + uint16_t isa_irq) This interface is pretty awful, taking the union of all the options needed for each type as separate parameters. Reusing the isa_irq parameter is making this worse along a different axis as well. AFAICT its only user is qemu-trad with PT_IRQ_TYPE_MSI_TRANSLATE. I think we should discourage any new uses of this function, and hide any ugliness in an internal static function to be used by the more specific xc_domain_bind_pt_isa_irq et al. i.e. make the current xc_doamin_bind_pt_irq an internal helper with a new name and a new spi_irq parameter and make the replacement xc_domain_bind_pt_irq a wrapper which handles only the set of types which it handles today and a new xc_domain_bind_pt_spi_irq which exposes the new functionality. Hopefully we can eventually remove xc_domain_bind_pt_irq. If you are minded to you could do that today, but it's not required I think. Ian.