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

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-04-17 at 07:02 +0100, Julien Grall wrote:
> 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.

My initial comment was talking specifically about
XEN_DOMCTL_unbind_pt_irq, which AFAICT does not call pt_irq_create_bind
(which is why I assumed you must mean unbind).

I agree that XEN_DOMCTL_bind_pt_irq should contain checks of all its
inputs, of course.

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

This was exactly my point, on x86 XEN_DOMCTL_unbind_pt_irq didn't appear
to pay attention to anything other than the virq provided, I assumed
since it doesn't need any other information from the caller to do as it
has been asked.

But it seems like maybe I was wrong and pt_irq_destroy_bind (called from
XEN_DOMCTL_unbind_pt_irq on x86) does actually need other info (I'm not
sure why, it seems like it ought to know these things without being told
by the toolstack). In which case having your check for consistency with
x86 is probably tolerable.

Ian.