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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-03-31 at 13:23 +0100, Julien Grall wrote:
> 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.

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

Yes, I just didn't want to say that you must shave that yakk here, if
you are keen to do so them please have at it!

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

Thanks.
Ian.



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