From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq Date: Thu, 16 Apr 2015 16:20:45 +0100 Message-ID: <552FD34D.20802@citrix.com> References: <1428592185-18581-1-git-send-email-julien.grall@citrix.com> <1428592185-18581-5-git-send-email-julien.grall@citrix.com> <1429196139.25195.148.camel@citrix.com> 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 1Yilc8-0006df-Gp for xen-devel@lists.xenproject.org; Thu, 16 Apr 2015 15:22:00 +0000 In-Reply-To: <1429196139.25195.148.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Julien Grall , Daniel De Graaf Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, Julien Grall , tim@xen.org, Jan Beulich List-Id: xen-devel@lists.xenproject.org Hi Ian, On 16/04/2015 15:55, Ian Campbell wrote: > On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote: >> From: Julien Grall > > I've left the XSM related quotes untrimmed and CCd Daniel. I think it's > all code motion (making x86 specific things generic), so perhaps no ack > needed but an opportunity to nack instead ;-) > >> 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, >> 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 >> >> In order to keep the same XSM checks done by the 2 hypercalls on x86, >> call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation. > > I think this paragraph makes the previous bit obsolete? Do you mean: "Concerning XSM..." until and the 2 paragraphs for XSM hypercalls? If so, yes. > >> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c >> index 1f2c80c..676ec50 100644 >> --- a/tools/libxc/xc_domain.c >> +++ b/tools/libxc/xc_domain.c >> @@ -1773,15 +1773,16 @@ int xc_domain_unbind_msi_irq( >> } >> >> /* Pass-through: binds machine irq to guests irq */ >> -int xc_domain_bind_pt_irq( >> - xc_interface *xch, >> - uint32_t domid, >> - uint8_t machine_irq, >> - uint8_t irq_type, >> - uint8_t bus, >> - uint8_t device, >> - uint8_t intx, >> - uint8_t isa_irq) >> +static int xc_domain_bind_pt_irq_int( >> + xc_interface *xch, >> + uint32_t domid, >> + uint8_t machine_irq, >> + uint8_t irq_type, >> + uint8_t bus, >> + uint8_t device, >> + uint8_t intx, >> + uint8_t isa_irq, >> + uint16_t spi) > > Please either leave the indentation of the arguments as it is or fix it > properly, i.e. put xch on the same line as the prototype and align > everything else below it. > > TBH I'd prefer the former even if it isn't strictly coding style since > it obscures the real change you made. Ok. I will do it. >> @@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq( >> PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq)); >> } >> >> +int xc_domain_bind_pt_spi_irq( >> + xc_interface *xch, >> + uint32_t domid, >> + uint16_t spi) >> +{ >> + /* vSPI == SPI */ > > I wonder if to avoid API churn later we should accept both machine and > guest IRQ here and rely on the check that htey are the same in the > hypervisor to reject? > > Fair enough we can change libxc interface if we want, but avoiding a > little churn later on seems reasonable and it makes it a better fit with > the existing interfaces. what about the following prototype: int xc_domain_bind_pt_spi_irq( xc_interface *xch, uint32_t domid, uint16_t spi, uint16_t vspi) I didn't reuse the current name i.e (machine_irq) because I find the name confusing. [..] >> + case XEN_DOMCTL_unbind_pt_irq: >> + { >> + int rc; >> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq; >> + uint32_t irq = bind->u.spi.spi; >> + uint32_t virq = bind->machine_irq; >> + >> + /* We only support PT_IRQ_TYPE_SPI */ >> + if ( bind->irq_type != PT_IRQ_TYPE_SPI ) >> + return -EOPNOTSUPP; >> + >> + /* For now map the interrupt 1:1 */ >> + if ( irq != virq ) >> + return -EINVAL; > > The x86 version doesn't appear to consider irq_type or irq, only virq > (from ->machine_irq). That seems to be logical to me (if a little > underdocumented) and I think we should be consistent. On x86, the parameters are used later in pt_create_bind which take the hypercall data in parameter. The both check are required in order to avoid future issue if we introduce new type and when we will support virq != irq. Regards, -- Julien Grall