[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
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@xxxxxxxxxx>
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
