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

[Julien Grall <julien.grall@citrix.com>]

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 <julien.grall@linaro.org>
>
> 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