From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 10/24] xen/arm: gic: Add sanity checks gic_route_irq_to_guest Date: Fri, 20 Feb 2015 17:28:12 +0000 Message-ID: <54E76EAC.6090100@linaro.org> References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org> <1421159133-31526-11-git-send-email-julien.grall@linaro.org> <1424448429.30924.333.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YOrNZ-0004vL-K5 for xen-devel@lists.xenproject.org; Fri, 20 Feb 2015 17:28:41 +0000 Received: by mail-wi0-f170.google.com with SMTP id hi2so7835835wib.1 for ; Fri, 20 Feb 2015 09:28:40 -0800 (PST) In-Reply-To: <1424448429.30924.333.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org Hi Ian, On 20/02/15 16:07, Ian Campbell wrote: > More importantly: We have (hopefully) guaranteed elsewhere that an PPI > or SGI can never make it here, I take it. If that's the case then either > the comment should say that, or more likely, the comment is redundently > restating the assert's condition. I will update the comment to /* Caller has already checked that the IRQ is an SPIs */ >> + ASSERT(virq >= 32 && virq < vgic_num_irqs(d)); > > NR_LOCAL_IRQS? Yes. > Also splitting the two conditions into two asserts will make it more > obvious which one failed if we hit it. Will do. >> + >> + vgic_lock_rank(v_target, rank, flags); >> + >> + if ( p->desc || >> + /* The VIRQ should not be already enabled by the guest */ >> + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) >> + goto out; >> >> desc->handler = gic_hw_ops->gic_guest_irq_type; >> set_bit(_IRQ_GUEST, &desc->status); >> >> - gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); >> + gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority); > > This smells like a functional change, not a sanity check, what is it > for? Hmmm right. I will move it to a separate patch. > Is v_target->processor always configured, even for the first routing of > an IRQ to dom0? Yes, IRQ are routed to CPU0 by default. > > Care needs to be taken here that priority is not under unfettered guest > control -- since this configures the physical GIC we need to e.g. ensure > that Xen's own IPIs have higher priority than anything a guest can ever > set. (Realistically this probably means we want to constrain guests to > the bottom half of the priority range and expose different BPR etc in > the vgic, out of scope here though) The priority is controlled by route_irq_to_guest and set statically using GIC_PRI_IRQ. If we decide to hardcoded the priority here, we should drop the parameter on gic_route_irq_guest. But not keeping both. Regards, -- Julien Grall