[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 16:40, Ian Campbell wrote:
On Thu, 2015-04-16 at 16:20 +0100, Julien Grall wrote:
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?
That's right.
Ok. I will drop 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.
Sure. Although you hit machine_irq again at the next level in the stack
so I think it's rather moot.
[..]
+ 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.
Ah yes, (although you mean pt_irq_destroy_bind I think?)
No I mean pt_irq_create_bind. The function makes usage of machine_irq
and irq_type. Although, there is no clear switch case, just an if in the
code.
The both check are required in order to avoid future issue if we
introduce new type and when we will support virq != irq.
Shouldn't they be in pt_irq_destroy_bind then?
I'm not following you. pt_irq_destroy_bind is an x86 specific function.
The check "virq != irq" is done in both DOMCTL_{,un}bind_irq on ARM even
though the one in unbind is not necessary useful.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
