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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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?

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

Ian.


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