| Message ID | 1447753261-7552-61-git-send-email-shannon.zhao@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 17 Nov 2015, shannon.zhao@linaro.org wrote: > From: Parth Dixit <parth.dixit@linaro.org> > > Interrupt information is described in DSDT and is not available at > the time of booting. Configure the interrupts dynamically when requested > by Dom0 > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > xen/arch/arm/vgic.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 7bb4570..d05abde 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -25,6 +25,7 @@ > #include <xen/irq.h> > #include <xen/sched.h> > #include <xen/perfc.h> > +#include <xen/acpi.h> > > #include <asm/current.h> > > @@ -310,6 +311,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) > } > } > > +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) ) Coding style. > void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) > { > struct domain *d = v->domain; > @@ -321,7 +324,23 @@ 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); The rank is always the same, you can move this out of the while loop. > + uint32_t tr; > + > irq = i + (32 * n); > + if( ( !acpi_disabled ) && ( n != 0 ) && is_hardware_domain(d) ) Instead of n != 0, I would prefer a more obvious irq >= 32 check > + { > + tr = vr->icfg[i >> 4] ; > + > + if( ( tr & VGIC_ICFG_MASK(i) ) ) Coding style. > + irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); > + else > + irq_set_type(irq, ACPI_IRQ_TYPE_LEVEL_MASK); Instead of this ad-hoc ACPI code, I would prefer if we get the irq type and, if it is set to NONE or INVALID, we set it to what is in the r->icfg (only for the hardware_domain of course). If it is set to something other than NONE or INVALID, and it doesn't match what is on r->icfg, then we could print out a warning. > + } > +#else > + irq = i + (32 * n); > +#endif Please leave the irq = i + (32 * n) out of the #ifdef. > v_target = d->arch.vgic.handler->get_target_vcpu(v, irq); > p = irq_to_pending(v_target, irq); > set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > -- > 2.1.0 >
On Fri, 27 Nov 2015, Stefano Stabellini wrote: > On Tue, 17 Nov 2015, shannon.zhao@linaro.org wrote: > > From: Parth Dixit <parth.dixit@linaro.org> > > > > Interrupt information is described in DSDT and is not available at > > the time of booting. Configure the interrupts dynamically when requested > > by Dom0 Actually wouldn't it make more sense to configure the physical interrupts when dom0 writes to the virtual ICFG registers? > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > --- > > xen/arch/arm/vgic.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > > index 7bb4570..d05abde 100644 > > --- a/xen/arch/arm/vgic.c > > +++ b/xen/arch/arm/vgic.c > > @@ -25,6 +25,7 @@ > > #include <xen/irq.h> > > #include <xen/sched.h> > > #include <xen/perfc.h> > > +#include <xen/acpi.h> > > > > #include <asm/current.h> > > > > @@ -310,6 +311,8 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) > > } > > } > > > > +#define VGIC_ICFG_MASK(intr) ( 1 << ( ( 2 * ( intr % 16 ) ) + 1 ) ) > > Coding style. > > > > void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) > > { > > struct domain *d = v->domain; > > @@ -321,7 +324,23 @@ 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); > > The rank is always the same, you can move this out of the while loop. > > > > + uint32_t tr; > > + > > irq = i + (32 * n); > > + if( ( !acpi_disabled ) && ( n != 0 ) && is_hardware_domain(d) ) > > Instead of n != 0, I would prefer a more obvious irq >= 32 check > > > > + { > > + tr = vr->icfg[i >> 4] ; > > + > > + if( ( tr & VGIC_ICFG_MASK(i) ) ) > > Coding style. > > > > + irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); > > + else > > + irq_set_type(irq, ACPI_IRQ_TYPE_LEVEL_MASK); > > Instead of this ad-hoc ACPI code, I would prefer if we get the irq type > and, if it is set to NONE or INVALID, we set it to what is in the > r->icfg (only for the hardware_domain of course). If it is set to > something other than NONE or INVALID, and it doesn't match what is on > r->icfg, then we could print out a warning. > > > > + } > > +#else > > + irq = i + (32 * n); > > +#endif > > Please leave the irq = i + (32 * n) out of the #ifdef. > > > > v_target = d->arch.vgic.handler->get_target_vcpu(v, irq); > > p = irq_to_pending(v_target, irq); > > set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > > -- > > 2.1.0 > > >
Hi Shannon, On 17/11/15 09:40, shannon.zhao@linaro.org wrote: > From: Parth Dixit <parth.dixit@linaro.org> > > Interrupt information is described in DSDT and is not available at > the time of booting. Configure the interrupts dynamically when requested > by Dom0 Missing ".". As said on a previous version of this patch [1], 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 <parth.dixit@linaro.org> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > --- > xen/arch/arm/vgic.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 7bb4570..d05abde 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -25,6 +25,7 @@ > #include <xen/irq.h> > #include <xen/sched.h> > #include <xen/perfc.h> > +#include <xen/acpi.h> > > #include <asm/current.h> > > @@ -310,6 +311,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; > @@ -321,7 +324,23 @@ 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; > + > 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. > + { > + tr = vr->icfg[i >> 4] ; > + > + if( ( tr & VGIC_ICFG_MASK(i) ) ) > + irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); > + else > + irq_set_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 will fail because the configuration has been set earlier (as you did in patch #55) > + } > +#else > + irq = i + (32 * n); > +#endif > v_target = d->arch.vgic.handler->get_target_vcpu(v, irq); > p = irq_to_pending(v_target, irq); > set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > Regards, [1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html
Hi, On 2015/11/30 23:42, Julien Grall wrote: > Hi Shannon, > > On 17/11/15 09:40, shannon.zhao@linaro.org wrote: >> From: Parth Dixit <parth.dixit@linaro.org> >> >> Interrupt information is described in DSDT and is not available at >> the time of booting. Configure the interrupts dynamically when requested >> by Dom0 > > Missing ".". > > As said on a previous version of this patch [1], 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 <parth.dixit@linaro.org> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> --- >> xen/arch/arm/vgic.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 7bb4570..d05abde 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -25,6 +25,7 @@ >> #include <xen/irq.h> >> #include <xen/sched.h> >> #include <xen/perfc.h> >> +#include <xen/acpi.h> >> >> #include <asm/current.h> >> >> @@ -310,6 +311,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; >> @@ -321,7 +324,23 @@ 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; >> + >> 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. > >> + { >> + tr = vr->icfg[i >> 4] ; >> + >> + if( ( tr & VGIC_ICFG_MASK(i) ) ) >> + irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); >> + else >> + irq_set_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. > Here it wants to set the type according to the value of GIC ICFG. I don't know this well. Does it need to set the type here when Dom0 configures the interrupts since I have set them as ACPI_IRQ_TYPE_NONE in patch 55? How about following way? 1) In patch 55, it only permit the access of Dom0 to those irqs, not set the type. 2) Then in this function or in the handler of writing GIC ICFG, check if the irq is permitted and set the irq type. > Lastly, they will fail because the configuration has been set earlier > (as you did in patch #55) > >> + } >> +#else >> + irq = i + (32 * n); >> +#endif >> v_target = d->arch.vgic.handler->get_target_vcpu(v, irq); >> p = irq_to_pending(v_target, irq); >> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); >> > > Regards, > > [1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html >
On Tue, 5 Jan 2016, Shannon Zhao wrote: > Hi, > > On 2015/11/30 23:42, Julien Grall wrote: > > Hi Shannon, > > > > On 17/11/15 09:40, shannon.zhao@linaro.org wrote: > >> From: Parth Dixit <parth.dixit@linaro.org> > >> > >> Interrupt information is described in DSDT and is not available at > >> the time of booting. Configure the interrupts dynamically when requested > >> by Dom0 > > > > Missing ".". > > > > As said on a previous version of this patch [1], 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 <parth.dixit@linaro.org> > >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > >> --- > >> xen/arch/arm/vgic.c | 19 +++++++++++++++++++ > >> 1 file changed, 19 insertions(+) > >> > >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > >> index 7bb4570..d05abde 100644 > >> --- a/xen/arch/arm/vgic.c > >> +++ b/xen/arch/arm/vgic.c > >> @@ -25,6 +25,7 @@ > >> #include <xen/irq.h> > >> #include <xen/sched.h> > >> #include <xen/perfc.h> > >> +#include <xen/acpi.h> > >> > >> #include <asm/current.h> > >> > >> @@ -310,6 +311,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; > >> @@ -321,7 +324,23 @@ 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; > >> + > >> 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. > > > >> + { > >> + tr = vr->icfg[i >> 4] ; > >> + > >> + if( ( tr & VGIC_ICFG_MASK(i) ) ) > >> + irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); > >> + else > >> + irq_set_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. > > > Here it wants to set the type according to the value of GIC ICFG. > > I don't know this well. Does it need to set the type here when Dom0 > configures the interrupts since I have set them as ACPI_IRQ_TYPE_NONE in > patch 55? > > How about following way? > 1) In patch 55, it only permit the access of Dom0 to those irqs, not set > the type. > 2) Then in this function or in the handler of writing GIC ICFG, check if > the irq is permitted and set the irq type. Sounds good > > Lastly, they will fail because the configuration has been set earlier > > (as you did in patch #55) > > > >> + } > >> +#else > >> + irq = i + (32 * n); > >> +#endif > >> v_target = d->arch.vgic.handler->get_target_vcpu(v, irq); > >> p = irq_to_pending(v_target, irq); > >> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > >> > > > > Regards, > > > > [1] http://lists.xen.org/archives/html/xen-devel/2015-06/msg01128.html > > > > -- > Shannon >
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 7bb4570..d05abde 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -25,6 +25,7 @@ #include <xen/irq.h> #include <xen/sched.h> #include <xen/perfc.h> +#include <xen/acpi.h> #include <asm/current.h> @@ -310,6 +311,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; @@ -321,7 +324,23 @@ 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; + irq = i + (32 * n); + if( ( !acpi_disabled ) && ( n != 0 ) && is_hardware_domain(d) ) + { + tr = vr->icfg[i >> 4] ; + + if( ( tr & VGIC_ICFG_MASK(i) ) ) + irq_set_type(irq, ACPI_IRQ_TYPE_EDGE_BOTH); + else + irq_set_type(irq, ACPI_IRQ_TYPE_LEVEL_MASK); + } +#else + irq = i + (32 * n); +#endif v_target = d->arch.vgic.handler->get_target_vcpu(v, irq); p = irq_to_pending(v_target, irq); set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);