From mboxrd@z Thu Jan 1 00:00:00 1970 From: Parth Dixit Subject: Re: [PATCH v2 39/41] arm : acpi configure interrupts dynamically Date: Sun, 5 Jul 2015 19:06:47 +0530 Message-ID: References: <1431893048-5214-1-git-send-email-parth.dixit@linaro.org> <1431893048-5214-40-git-send-email-parth.dixit@linaro.org> <5575D36C.4030709@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5575D36C.4030709@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: Julien Grall , shannon.zhao@linaro.org Cc: keir@xen.org, Ian Campbell , andrew.cooper3@citrix.com, tim@xen.org, xen-devel , Stefano Stabellini , Jan Beulich , Christoffer Dall List-Id: xen-devel@lists.xenproject.org +shannon On 8 June 2015 at 23:09, Julien Grall wrote: > Hi Parth, > > On 17/05/2015 21:04, Parth Dixit wrote: >> >> Interrupt information is described in DSDT and is not available at >> the time of booting. Configure the interrupts dynamically when requested >> by Dom0 > > > Missing "." > > Also, I'm sure we talked about it multiple time. I'd like to keep the ACPI > changes very contained to Xen boot. Your change is not ACPI specific and > could be used for DT. > > >> >> Signed-off-by: Parth Dixit >> --- >> xen/arch/arm/vgic.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 73a6f7e..f63deb4 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> >> @@ -285,6 +286,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int >> n) >> } >> } >> >> +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) ) >> + >> void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) >> { >> struct domain *d = v->domain; >> @@ -296,7 +299,22 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int >> n) >> struct vcpu *v_target; >> >> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) { >> +#ifdef CONFIG_ACPI >> + struct vgic_irq_rank *vr = vgic_get_rank(v, n); >> + uint32_t tr; > > > New line. > >> + irq = i + (32 * n); >> + if( ( !acpi_disabled ) && ( n != 0 ) && is_hardware_domain(d) ) > > > You need to add a comment explaining the ( n != 0 ) i.e we don't SGIs and > PPIs are RO. It's implementation defined for PPI but it's preferable to let > Xen take care of it. > > Also, we should set the type only for IRQ assigned to DOM0 (i.e p->desc != > NULL). With your current solution, DOM0 may change the configuration of the > serial IRQ by mistake and take down Xen because the physical IRQ is enabled > and the behavior will be unpredictable. > > Furthermore, during passthrough, the IRQ may not have been configured by > DOM0. So we have to let the guest configure the IRQ. > >> + { >> + tr = vr->icfg[i >> 4] ; >> + >> + if( ( tr & VGIC_ICFG_MASK(i) ) ) >> + set_irq_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); >> + else >> + set_irq_type(irq, ACPI_IRQ_TYPE_LEVEL_MASK); > > > Given that only SPI can be configured it would have been better to call > irq_set_type. > > Although, those 2 functions don't do what you think. They are setting the > type internally in Xen but don't change the GIC interrupt configuration > register. > > Lastly, they may fail because the configuration as been set earlier (as you > did in patch #41. > > Regards, > > -- > Julien Grall