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: Fri, 17 Apr 2015 07:02:17 +0100 Message-ID: <5530A1E9.6060304@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> <552FD34D.20802@citrix.com> <1429198830.25195.168.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.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YizMA-0005dW-5L for xen-devel@lists.xenproject.org; Fri, 17 Apr 2015 06:02:26 +0000 In-Reply-To: <1429198830.25195.168.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 Cc: Julien Grall , tim@xen.org, stefano.stabellini@citrix.com, Jan Beulich , xen-devel@lists.xenproject.org, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org Hi Ian, On 16/04/2015 16:40, Ian Campbell wrote: > On Thu, 2015-04-16 at 16:20 +0100, Julien Grall wrote: >>>> 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? > > That's right. Ok. I will drop 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. > > Sure. Although you hit machine_irq again at the next level in the stack > so I think it's rather moot. > >> >> [..] >> >>>> + 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. > > Ah yes, (although you mean pt_irq_destroy_bind I think?) No I mean pt_irq_create_bind. The function makes usage of machine_irq and irq_type. Although, there is no clear switch case, just an if in the code. >> The both check are required in order to avoid future issue if we >> introduce new type and when we will support virq != irq. > > Shouldn't they be in pt_irq_destroy_bind then? I'm not following you. pt_irq_destroy_bind is an x86 specific function. The check "virq != irq" is done in both DOMCTL_{,un}bind_irq on ARM even though the one in unbind is not necessary useful. Regards, -- Julien Grall