From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v1 0/2] xen/arm: maintenance_interrupt SMP fix Date: Wed, 29 Jan 2014 13:07:14 +0000 Message-ID: <52E8FD02.2060601@linaro.org> References: <1390844023-23123-1-git-send-email-oleksandr.tyshchenko@globallogic.com> <52E69CBC.3090207@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Oleksandr Tyshchenko Cc: Stefano Stabellini , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 29/01/14 10:56, Oleksandr Tyshchenko wrote: > Hello all, > > I just recollected about one hack which we created > as we needed to route HW IRQ in domU. > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 9d793ba..d0227b9 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -989,8 +989,6 @@ static void domcreate_launch_dm(libxl__egc *egc, > libxl__multidev *multidev, > > LOG(DEBUG, "dom%d irq %d", domid, irq); > > - ret = irq >= 0 ? xc_physdev_map_pirq(CTX->xch, domid, irq, &irq) > - : -EOVERFLOW; > if (!ret) > ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1); > if (ret < 0) { > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 2e4b11f..b54c08e 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -85,7 +85,7 @@ int domain_vgic_init(struct domain *d) > if ( d->domain_id == 0 ) > d->arch.vgic.nr_lines = gic_number_lines() - 32; > else > - d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */ > + d->arch.vgic.nr_lines = gic_number_lines() - 32; /* We do > need SPIs for the guest */ > > d->arch.vgic.shared_irqs = > xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d)); > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 75e2df3..ba88901 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > > static DEFINE_SPINLOCK(domctl_lock); > DEFINE_SPINLOCK(vcpu_alloc_lock); > @@ -782,8 +783,11 @@ long > do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > ret = -EINVAL; > else if ( xsm_irq_permission(XSM_HOOK, d, pirq, allow) ) > ret = -EPERM; > - else if ( allow ) > - ret = pirq_permit_access(d, pirq); > + else if ( allow ) { > + struct dt_irq irq = {pirq + NR_LOCAL_IRQS,0}; > + ret = pirq_permit_access(d, irq.irq); > + gic_route_irq_to_guest(d, &irq, ""); > + } > else > ret = pirq_deny_access(d, pirq); > } > (END) > > It seems, the following patch can violate the logic about routing > physical IRQs only to CPU0. I forgot the smp_processor_id() in gic_route_irq_to_guest(). As this function is only called (for upstream) when dom0 is building, only CPU0 is used. > In gic_route_irq_to_guest() we need to call gic_set_irq_properties() > where the one of the parameters is cpumask_of(smp_processor_id()). > But in this part of code this function can be executed on CPU1. And as > result this can cause to the fact that the wrong value would set to > target CPU mask. > Please, confirm my assumption. > If I am right we have to add a basic HW IRQ routing to DomU in a right way. With this current implementation, IRQ will be routed to CPU which has done the hypercall. This CPU could be different than the CPU where the domU (assuming we have only 1 VCPU) is running. I think for both dom0 and domU, in case of the VCPU is pinned, we should say use the cpumask of this VCPU. -- Julien Grall