Re: [Xen-devel] [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.

> >> @@ -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?)

> 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?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel