|
[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
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?
> 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.
> @@ -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.
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 6f30af7..531bb13 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -10,6 +10,8 @@
> #include <xen/errno.h>
> #include <xen/sched.h>
> #include <xen/hypercall.h>
> +#include <xen/iocap.h>
> +#include <xsm/xsm.h>
> #include <public/domctl.h>
>
> long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> @@ -30,6 +32,81 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
> domain *d,
>
> return p2m_cache_flush(d, s, e);
> }
> + case XEN_DOMCTL_bind_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;
> +
> + /*
> + * XXX: For now map the interrupt 1:1. Other support will require to
> + * modify domain_pirq_to_irq macro.
> + */
> + if ( irq != virq )
> + return -EINVAL;
> +
> + /*
> + * ARM doesn't require to separate IRQ assignation in 2
"ARM doesn't require separating assignment of IRQs into 2 hypercalls"
> + * hypercalls (PHYSDEVOP_map_pirq and DOMCTL_bind_pt_irq).
> + *
> + * Call xsm_map_domain_irq in order to keep the same XSM checks
> + * done by the 2 hypercalls
... for consistency with other architectures.
> + */
> + rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
> + if ( rc )
> + return rc;
> +
> + rc = xsm_bind_pt_irq(XSM_HOOK, d, bind);
> + if ( rc )
> + return rc;
> +
> + if ( !irq_access_permitted(current->domain, irq) )
> + return -EPERM;
> +
> + if ( !vgic_reserve_virq(d, virq) )
> + return -EBUSY;
> +
> + rc = route_irq_to_guest(d, virq, irq, "routed IRQ");
> + if ( rc )
> + vgic_free_virq(d, virq);
> +
> + return rc;
> + }
> + 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.
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 4227093..c36e05f 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -445,6 +445,18 @@ static XSM_INLINE int
> xsm_unmap_domain_pirq(XSM_DEFAULT_ARG struct domain *d)
> return xsm_default_action(action, current->domain, d);
> }
>
> +static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d,
> struct xen_domctl_bind_pt_irq *bind)
> +{
> + XSM_ASSERT_ACTION(XSM_HOOK);
> + return xsm_default_action(action, current->domain, d);
> +}
> +
> +static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d,
> struct xen_domctl_bind_pt_irq *bind)
> +{
> + XSM_ASSERT_ACTION(XSM_HOOK);
> + return xsm_default_action(action, current->domain, d);
> +}
> +
> static XSM_INLINE int xsm_unmap_domain_irq(XSM_DEFAULT_ARG struct domain *d,
> int irq, void *data)
> {
> XSM_ASSERT_ACTION(XSM_HOOK);
> @@ -631,18 +643,6 @@ static XSM_INLINE int xsm_priv_mapping(XSM_DEFAULT_ARG
> struct domain *d, struct
> return xsm_default_action(action, d, t);
> }
>
> -static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d,
> struct xen_domctl_bind_pt_irq *bind)
> -{
> - XSM_ASSERT_ACTION(XSM_HOOK);
> - return xsm_default_action(action, current->domain, d);
> -}
> -
> -static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d,
> struct xen_domctl_bind_pt_irq *bind)
> -{
> - XSM_ASSERT_ACTION(XSM_HOOK);
> - return xsm_default_action(action, current->domain, d);
> -}
> -
> static XSM_INLINE int xsm_ioport_permission(XSM_DEFAULT_ARG struct domain
> *d, uint32_t s, uint32_t e, uint8_t allow)
> {
> XSM_ASSERT_ACTION(XSM_HOOK);
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index c2049f3..b7446be 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -107,6 +107,8 @@ struct xsm_operations {
> int (*map_domain_irq) (struct domain *d, int irq, void *data);
> int (*unmap_domain_pirq) (struct domain *d);
> int (*unmap_domain_irq) (struct domain *d, int irq, void *data);
> + int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
> *bind);
> + int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
> *bind);
> int (*irq_permission) (struct domain *d, int pirq, uint8_t allow);
> int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e,
> uint8_t allow);
> int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, uint8_t
> allow);
> @@ -169,8 +171,6 @@ struct xsm_operations {
> int (*mmuext_op) (struct domain *d, struct domain *f);
> int (*update_va_mapping) (struct domain *d, struct domain *f,
> l1_pgentry_t pte);
> int (*priv_mapping) (struct domain *d, struct domain *t);
> - int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
> *bind);
> - int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq
> *bind);
> int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e,
> uint8_t allow);
> int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t
> allow);
> #endif
> @@ -419,6 +419,18 @@ static inline int xsm_unmap_domain_irq (xsm_default_t
> def, struct domain *d, int
> return xsm_ops->unmap_domain_irq(d, irq, data);
> }
>
> +static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
> + struct xen_domctl_bind_pt_irq *bind)
> +{
> + return xsm_ops->bind_pt_irq(d, bind);
> +}
> +
> +static inline int xsm_unbind_pt_irq(xsm_default_t def, struct domain *d,
> + struct xen_domctl_bind_pt_irq *bind)
> +{
> + return xsm_ops->unbind_pt_irq(d, bind);
> +}
> +
> static inline int xsm_irq_permission (xsm_default_t def, struct domain *d,
> int pirq, uint8_t allow)
> {
> return xsm_ops->irq_permission(d, pirq, allow);
> @@ -643,18 +655,6 @@ static inline int xsm_priv_mapping(xsm_default_t def,
> struct domain *d, struct d
> return xsm_ops->priv_mapping(d, t);
> }
>
> -static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
> - struct
> xen_domctl_bind_pt_irq *bind)
> -{
> - return xsm_ops->bind_pt_irq(d, bind);
> -}
> -
> -static inline int xsm_unbind_pt_irq(xsm_default_t def, struct domain *d,
> - struct
> xen_domctl_bind_pt_irq *bind)
> -{
> - return xsm_ops->unbind_pt_irq(d, bind);
> -}
> -
> static inline int xsm_ioport_permission (xsm_default_t def, struct domain
> *d, uint32_t s, uint32_t e, uint8_t allow)
> {
> return xsm_ops->ioport_permission(d, s, e, allow);
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 25fca68..a3b8aab 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -81,6 +81,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
> set_to_dummy_if_null(ops, map_domain_irq);
> set_to_dummy_if_null(ops, unmap_domain_pirq);
> set_to_dummy_if_null(ops, unmap_domain_irq);
> + set_to_dummy_if_null(ops, bind_pt_irq);
> + set_to_dummy_if_null(ops, unbind_pt_irq);
> set_to_dummy_if_null(ops, irq_permission);
> set_to_dummy_if_null(ops, iomem_permission);
> set_to_dummy_if_null(ops, iomem_mapping);
> @@ -140,8 +142,6 @@ void xsm_fixup_ops (struct xsm_operations *ops)
> set_to_dummy_if_null(ops, mmuext_op);
> set_to_dummy_if_null(ops, update_va_mapping);
> set_to_dummy_if_null(ops, priv_mapping);
> - set_to_dummy_if_null(ops, bind_pt_irq);
> - set_to_dummy_if_null(ops, unbind_pt_irq);
> set_to_dummy_if_null(ops, ioport_permission);
> set_to_dummy_if_null(ops, ioport_mapping);
> #endif
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index ab5141d..688ba2a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -580,12 +580,14 @@ static int flask_domctl(struct domain *d, int cmd)
> #ifdef HAS_MEM_ACCESS
> case XEN_DOMCTL_vm_event_op:
> #endif
> +
> + /* These have individual XSM hooks (arch/../domctl.c) */
> + case XEN_DOMCTL_bind_pt_irq:
> + case XEN_DOMCTL_unbind_pt_irq:
> #ifdef CONFIG_X86
> /* These have individual XSM hooks (arch/x86/domctl.c) */
> case XEN_DOMCTL_shadow_op:
> case XEN_DOMCTL_ioport_permission:
> - case XEN_DOMCTL_bind_pt_irq:
> - case XEN_DOMCTL_unbind_pt_irq:
> case XEN_DOMCTL_ioport_mapping:
> /* These have individual XSM hooks (drivers/passthrough/iommu.c) */
> case XEN_DOMCTL_get_device_group:
> @@ -911,6 +913,36 @@ static int flask_unmap_domain_irq (struct domain *d, int
> irq, void *data)
> return rc;
> }
>
> +static int flask_bind_pt_irq (struct domain *d, struct
> xen_domctl_bind_pt_irq *bind)
> +{
> + u32 dsid, rsid;
> + int rc = -EPERM;
> + int irq;
> + struct avc_audit_data ad;
> +
> + rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
> + if ( rc )
> + return rc;
> +
> + irq = domain_pirq_to_irq(d, bind->machine_irq);
> +
> + rc = get_irq_sid(irq, &rsid, &ad);
> + if ( rc )
> + return rc;
> +
> + rc = avc_current_has_perm(rsid, SECCLASS_HVM, HVM__BIND_IRQ, &ad);
> + if ( rc )
> + return rc;
> +
> + dsid = domain_sid(d);
> + return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
> +}
> +
> +static int flask_unbind_pt_irq (struct domain *d, struct
> xen_domctl_bind_pt_irq *bind)
> +{
> + return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
> +}
> +
> static int flask_irq_permission (struct domain *d, int pirq, uint8_t access)
> {
> /* the PIRQ number is not useful; real IRQ is checked during mapping */
> @@ -1468,36 +1500,6 @@ static int flask_priv_mapping(struct domain *d, struct
> domain *t)
> {
> return domain_has_perm(d, t, SECCLASS_MMU, MMU__TARGET_HACK);
> }
> -
> -static int flask_bind_pt_irq (struct domain *d, struct
> xen_domctl_bind_pt_irq *bind)
> -{
> - u32 dsid, rsid;
> - int rc = -EPERM;
> - int irq;
> - struct avc_audit_data ad;
> -
> - rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
> - if ( rc )
> - return rc;
> -
> - irq = domain_pirq_to_irq(d, bind->machine_irq);
> -
> - rc = get_irq_sid(irq, &rsid, &ad);
> - if ( rc )
> - return rc;
> -
> - rc = avc_current_has_perm(rsid, SECCLASS_HVM, HVM__BIND_IRQ, &ad);
> - if ( rc )
> - return rc;
> -
> - dsid = domain_sid(d);
> - return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
> -}
> -
> -static int flask_unbind_pt_irq (struct domain *d, struct
> xen_domctl_bind_pt_irq *bind)
> -{
> - return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
> -}
> #endif /* CONFIG_X86 */
>
> long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
> @@ -1556,6 +1558,8 @@ static struct xsm_operations flask_ops = {
> .map_domain_irq = flask_map_domain_irq,
> .unmap_domain_pirq = flask_unmap_domain_pirq,
> .unmap_domain_irq = flask_unmap_domain_irq,
> + .bind_pt_irq = flask_bind_pt_irq,
> + .unbind_pt_irq = flask_unbind_pt_irq,
> .irq_permission = flask_irq_permission,
> .iomem_permission = flask_iomem_permission,
> .iomem_mapping = flask_iomem_mapping,
> @@ -1616,8 +1620,6 @@ static struct xsm_operations flask_ops = {
> .mmuext_op = flask_mmuext_op,
> .update_va_mapping = flask_update_va_mapping,
> .priv_mapping = flask_priv_mapping,
> - .bind_pt_irq = flask_bind_pt_irq,
> - .unbind_pt_irq = flask_unbind_pt_irq,
> .ioport_permission = flask_ioport_permission,
> .ioport_mapping = flask_ioport_mapping,
> #endif
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |