Re: [PATCH v4 19/33] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 31/03/15 12:11, Ian Campbell wrote:
> On Thu, 2015-03-19 at 19:29 +0000, Julien Grall wrote:
>> 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,
> 
> "figure out the right way"
> 
>> 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
>>
>> Rather than checking that the current domain can both add and bind the
>> IRQ, we only check the bind permission. I think this is not a big deal
>> because we don't have emulator on ARM and therefore no disaggregation is
>> required.
> 
> Is this because we don't have the "add" concept on arm?

We don't need the 2 concepts on ARM. So I choose on of them. The "bind"
concept is tight to DOMCTL_bind_irq on x86.

Although, thinking a bit more, it would make more sense to use check
"add" but not "bind".

This is because on x86, "add" concept if for the toolstack and "bind"
for the emulator/stubdomain.

FWIW, the example policy give both "add" and "bind" right to the
toolstack domain.

> 
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index 579d266..8243b70 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -1764,7 +1764,7 @@ int xc_domain_bind_pt_irq(
>>      uint8_t bus,
>>      uint8_t device,
>>      uint8_t intx,
>> -    uint8_t isa_irq)
>> +    uint16_t isa_irq)
> 
> This interface is pretty awful, taking the union of all the options
> needed for each type as separate parameters. Reusing the isa_irq
> parameter is making this worse along a different axis as well.
> 
> AFAICT its only user is qemu-trad with PT_IRQ_TYPE_MSI_TRANSLATE.

I didn't find any other caller. I could replace the usage in
xc_domain_update_msi_irq.

> I think we should discourage any new uses of this function, and hide any
> ugliness in an internal static function to be used by the more specific
> xc_domain_bind_pt_isa_irq et al. i.e. make the current
> xc_doamin_bind_pt_irq an internal helper with a new name and a new
> spi_irq parameter and make the replacement xc_domain_bind_pt_irq a
> wrapper which handles only the set of types which it handles today and a
> new xc_domain_bind_pt_spi_irq which exposes the new functionality.
>
> Hopefully we can eventually remove xc_domain_bind_pt_irq. If you are
> minded to you could do that today, but it's not required I think.

IIRC, the libxc API is not stable so we could drop a function easily.

Every possible types of IRQ already have helpers. Making
xc_domain_bind_pt_irq static is the easiest things to do (compare to
clean the current function).

I will give a look.

Regards,

-- 
Julien Grall