From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu Date: Fri, 23 May 2014 18:46:53 +0100 Message-ID: <537F898D.3090205@linaro.org> References: <1400761950-25035-13-git-send-email-stefano.stabellini@eu.citrix.com> <537E2166.1060401@linaro.org> <537E3DA0.7060007@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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: Stefano Stabellini Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On 05/23/2014 06:33 PM, Stefano Stabellini wrote: > On Thu, 22 May 2014, Julien Grall wrote: >> On 22/05/14 18:45, Stefano Stabellini wrote: >>> On Thu, 22 May 2014, Julien Grall wrote: >>>> Hi Stefano, >>>> >>>> On 22/05/14 13:32, Stefano Stabellini wrote: >>>>> At the moment gic_remove_from_queues doesn't handle the case where the >>>>> guest kernel disables an irq on a different vcpu compared to the one >>>>> currently receiving the interrupt. >>>>> Make sure to take the right vcpu lock before removing the irq from >>>>> lr_queue. >>>> >>>> I see the same issue with vgic_enable_irqs. We may inject to the wrong >>>> VCPU >>>> (i.e other than 0). >>>> >>>> I think we should have the same case in vgic_enable_irqs. >>> >>> I think it would make more sense to print a warning in >>> vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs. >> >> IHMO the warning is not enougth. We may screw your state machine. > > That cannot happen: rank->itargets is actually unused at the moment. itargets is not used, but nothing prevent a guest to enabled an IRQ on VCPU1. This can inject the IRQ in VCPU1 while it's already injected in VCPU0 (assuming the IRQ what disable for a little time). > >> BTW, for your todo: >> >>> + /* TODO: evict the irq from LRs */ >> >> We should not evict the IRQ from LRs. The guest may disable the IRQ while he >> is in the IRQ context (and before the IRQ has been EOI). If you drop the IRQs >> from the LRs, this can result to a maintenance interrupt: >> >> "If the specified Interrupt does not exist in the >> List registers, the GICH_HCR.EOIcount field is incremented, potentially >> generating a maintenance interrupt." > > It is still better than the alternative: having an LR busy for no reason. > A maintenance interrupt would be harmless. Our internal representation (in the status field, still inflight) won't be update-to-date for IRQ. We either inject a spurious IRQ (if it's a virtual IRQ), other set active & pending is physical IRQ (which is invalid from the GIC specification). Regards, -- Julien Grall