| Message ID | 1403271316-21635-1-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 20/06/14 14:35, Stefano Stabellini wrote: > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > interrupts and perform EOIs in the hypervisor as a workaround. > Trigger this behaviour with a per platform option. > > No need to find the pcpu that needs to write to GICC_DIR, because after > "physical irq follow virtual irq" we always inject the virtual irq on > the vcpu that is running on the pcpu that received the interrupt. Even without the patch "physical irq follow virtual irq" we can EOI an SPIs on any physical CPUs. Otherwise, even with this patch, you will have to find pcpu when the VCPU has been migrated by EOI the IRQ :). I would rework this last paragraph to explain this is how the GIC behave. > static void gic_update_one_lr(struct vcpu *v, int i) > { > struct pending_irq *p; > @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i) > clear_bit(i, &this_cpu(lr_mask)); > > if ( p->desc != NULL ) > + { > p->desc->status &= ~IRQ_INPROGRESS; > + if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) ) > + gic_irq_eoi(p->irq); > + } > clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); > p->lr = GIC_INVALID_LR; With this change, shouldn't we clear the bit and the LR before EOI the IRQ? Regards,
On Fri, 20 Jun 2014, Julien Grall wrote: > Hi Stefano, > > On 20/06/14 14:35, Stefano Stabellini wrote: > > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > > interrupts and perform EOIs in the hypervisor as a workaround. > > Trigger this behaviour with a per platform option. > > > > No need to find the pcpu that needs to write to GICC_DIR, because after > > "physical irq follow virtual irq" we always inject the virtual irq on > > the vcpu that is running on the pcpu that received the interrupt. > > Even without the patch "physical irq follow virtual irq" we can EOI an SPIs on > any physical CPUs. Can you point me to the paragraph of the GIC spec that says that? > Otherwise, even with this patch, you will have to find pcpu when the VCPU has > been migrated by EOI the IRQ :). > I would rework this last paragraph to explain this is how the GIC behave. > > > static void gic_update_one_lr(struct vcpu *v, int i) > > { > > struct pending_irq *p; > > @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i) > > clear_bit(i, &this_cpu(lr_mask)); > > > > if ( p->desc != NULL ) > > + { > > p->desc->status &= ~IRQ_INPROGRESS; > > + if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) ) > > + gic_irq_eoi(p->irq); > > + } > > clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > > clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); > > p->lr = GIC_INVALID_LR; > > With this change, shouldn't we clear the bit and the LR before EOI the IRQ? Conceptually you might be right but it is all happening with the vgic lock taken, so it shouldn't make any difference.
On 20/06/14 15:27, Stefano Stabellini wrote: > On Fri, 20 Jun 2014, Julien Grall wrote: >> Hi Stefano, >> >> On 20/06/14 14:35, Stefano Stabellini wrote: >>> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance >>> interrupts and perform EOIs in the hypervisor as a workaround. >>> Trigger this behaviour with a per platform option. >>> >>> No need to find the pcpu that needs to write to GICC_DIR, because after >>> "physical irq follow virtual irq" we always inject the virtual irq on >>> the vcpu that is running on the pcpu that received the interrupt. >> >> Even without the patch "physical irq follow virtual irq" we can EOI an SPIs on >> any physical CPUs. > > Can you point me to the paragraph of the GIC spec that says that? While it's clearly specified in the spec that the GICC_EOIR must be done in the same physical CPU. Nothing has been specified for GICC_DIR. I'm assuming this is the case, because even with the HW bit the physical interrupt may be EOIed on a different CPU. Think about the VCPU has been migrated when the guest is still handling the interrupt... Your patch "physical irq follow virtual irq" won't even solve this problem if the maintenance interrupt is request to EOI the IRQ. > >> Otherwise, even with this patch, you will have to find pcpu when the VCPU has >> been migrated by EOI the IRQ :). >> I would rework this last paragraph to explain this is how the GIC behave. >> >>> static void gic_update_one_lr(struct vcpu *v, int i) >>> { >>> struct pending_irq *p; >>> @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i) >>> clear_bit(i, &this_cpu(lr_mask)); >>> >>> if ( p->desc != NULL ) >>> + { >>> p->desc->status &= ~IRQ_INPROGRESS; >>> + if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) ) >>> + gic_irq_eoi(p->irq); >>> + } >>> clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); >>> clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); >>> p->lr = GIC_INVALID_LR; >> >> With this change, shouldn't we clear the bit and the LR before EOI the IRQ? > > Conceptually you might be right but it is all happening with the vgic > lock taken, so it shouldn't make any difference. The VGIC lock is per-vcpu. If this is happening while we migrate, nothing protect p->lr and the different bit anymore. Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon as we EOI the IRQ. I'm not sure why you didn't decide to have a dist lock for SPIs. It would have make interrupt handling easier.
On Fri, 20 Jun 2014, Julien Grall wrote: > On 20/06/14 15:27, Stefano Stabellini wrote: > > On Fri, 20 Jun 2014, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 20/06/14 14:35, Stefano Stabellini wrote: > > > > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance > > > > interrupts and perform EOIs in the hypervisor as a workaround. > > > > Trigger this behaviour with a per platform option. > > > > > > > > No need to find the pcpu that needs to write to GICC_DIR, because after > > > > "physical irq follow virtual irq" we always inject the virtual irq on > > > > the vcpu that is running on the pcpu that received the interrupt. > > > > > > Even without the patch "physical irq follow virtual irq" we can EOI an > > > SPIs on > > > any physical CPUs. > > > > Can you point me to the paragraph of the GIC spec that says that? > > While it's clearly specified in the spec that the GICC_EOIR must be done in > the same physical CPU. Nothing has been specified for GICC_DIR. That's a pity. I suspect that you are right but I don't like to make an assumption like that without the spec clearly supporting it. > I'm assuming this is the case, because even with the HW bit the physical > interrupt may be EOIed on a different CPU. Think about the VCPU has been > migrated when the guest is still handling the interrupt... I also think it should be OK, but can we be sure that all the SOC vendors are going to use a GICv2 that supports this behaviour? > Your patch "physical irq follow virtual irq" won't even solve this problem if > the maintenance interrupt is request to EOI the IRQ. I don't understand this statement. The maintenance interrupt is requested to make sure that the vcpu is interrupted as soon as it EOIs the virtual irq, so that gic_update_one_lr can run. > > > Otherwise, even with this patch, you will have to find pcpu when the VCPU > > > has > > > been migrated by EOI the IRQ :). > > > I would rework this last paragraph to explain this is how the GIC behave. > > > > > > > static void gic_update_one_lr(struct vcpu *v, int i) > > > > { > > > > struct pending_irq *p; > > > > @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int > > > > i) > > > > clear_bit(i, &this_cpu(lr_mask)); > > > > > > > > if ( p->desc != NULL ) > > > > + { > > > > p->desc->status &= ~IRQ_INPROGRESS; > > > > + if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) ) > > > > + gic_irq_eoi(p->irq); > > > > + } > > > > clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > > > > clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); > > > > p->lr = GIC_INVALID_LR; > > > > > > With this change, shouldn't we clear the bit and the LR before EOI the > > > IRQ? > > > > Conceptually you might be right but it is all happening with the vgic > > lock taken, so it shouldn't make any difference. > > The VGIC lock is per-vcpu. > > If this is happening while we migrate, nothing protect p->lr and the different > bit anymore. The patch series alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes care of vcpu migration and locking. > Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon as > we EOI the IRQ. Yes, but at that point we have also already migrated the corresponding physical irq to the new pcpu. However looking back again at my series, if the guest kernel keeps writing to GICD_ITARGETSR while the interrupt is firing, there is a small window of time when it could be possible to read GICC_IAR in Xen on a different pcpu than the one we are injecting the virtual irq. It involves migrating an interrupt more than once before EOIing the one that is currently INPROGRESS.
On 20/06/14 17:48, Stefano Stabellini wrote: > >> Your patch "physical irq follow virtual irq" won't even solve this problem if >> the maintenance interrupt is request to EOI the IRQ. > > I don't understand this statement. > The maintenance interrupt is requested to make sure that the vcpu is > interrupted as soon as it EOIs the virtual irq, so that > gic_update_one_lr can run. I agree with that but ... the following step can happen 1) Xen receives the interrupt 2) Xen writes EOIR 3) Xen inject the IRQ to the guest 4) The guest will receive the IRQ (i.e read IAR) 5) Xen migrates the VCPU to another physical CPU 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. In a such case, the IRQ will be EOIed to another physical CPU. If we assume that we should always write to GICC_DIR of the physical CPU that receive the interrupt, then even your patch "physcial irq follow virtual irq" won't solve the problem. >> The VGIC lock is per-vcpu. >> >> If this is happening while we migrate, nothing protect p->lr and the different >> bit anymore. > > The patch series > alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes > care of vcpu migration and locking. > > >> Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon as >> we EOI the IRQ. > > Yes, but at that point we have also already migrated the corresponding > physical irq to the new pcpu. Even tho we the migration has been done, the 2 clear bit are not atomic. Let's imagine that the IRQ has fired between the two clear bit. In this case we may clear the ACTIVE bit by mistake. I don't see anything in this code which prevents a such configuration. Regards,
On Fri, 20 Jun 2014, Julien Grall wrote: > On 20/06/14 17:48, Stefano Stabellini wrote: > > > > > Your patch "physical irq follow virtual irq" won't even solve this problem > > > if > > > the maintenance interrupt is request to EOI the IRQ. > > > > I don't understand this statement. > > The maintenance interrupt is requested to make sure that the vcpu is > > interrupted as soon as it EOIs the virtual irq, so that > > gic_update_one_lr can run. > > I agree with that but ... the following step can happen > > 1) Xen receives the interrupt > 2) Xen writes EOIR > 3) Xen inject the IRQ to the guest > 4) The guest will receive the IRQ (i.e read IAR) > 5) Xen migrates the VCPU to another physical CPU > 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. > > In a such case, the IRQ will be EOIed to another physical CPU. If we assume > that we should always write to GICC_DIR of the physical CPU that receive the > interrupt, then even your patch "physcial irq follow virtual irq" won't solve > the problem. That's true. > > > The VGIC lock is per-vcpu. > > > > > > If this is happening while we migrate, nothing protect p->lr and the > > > different > > > bit anymore. > > > > The patch series > > alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes > > care of vcpu migration and locking. > > > > > > > Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon > > > as > > > we EOI the IRQ. > > > > Yes, but at that point we have also already migrated the corresponding > > physical irq to the new pcpu. > > Even tho we the migration has been done, the 2 clear bit are not atomic. Let's > imagine that the IRQ has fired between the two clear bit. In this case we may > clear the ACTIVE bit by mistake. > > I don't see anything in this code which prevents a such configuration. Which 2 clear bit?
On 20 Jun 2014 18:41, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com> wrote: > > On Fri, 20 Jun 2014, Julien Grall wrote: > > On 20/06/14 17:48, Stefano Stabellini wrote: > > > > > > > Your patch "physical irq follow virtual irq" won't even solve this problem > > > > if > > > > the maintenance interrupt is request to EOI the IRQ. > > > > > > I don't understand this statement. > > > The maintenance interrupt is requested to make sure that the vcpu is > > > interrupted as soon as it EOIs the virtual irq, so that > > > gic_update_one_lr can run. > > > > I agree with that but ... the following step can happen > > > > 1) Xen receives the interrupt > > 2) Xen writes EOIR > > 3) Xen inject the IRQ to the guest > > 4) The guest will receive the IRQ (i.e read IAR) > > 5) Xen migrates the VCPU to another physical CPU > > 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. > > > > In a such case, the IRQ will be EOIed to another physical CPU. If we assume > > that we should always write to GICC_DIR of the physical CPU that receive the > > interrupt, then even your patch "physcial irq follow virtual irq" won't solve > > the problem. > > That's true. > > > > > > > The VGIC lock is per-vcpu. > > > > > > > > If this is happening while we migrate, nothing protect p->lr and the > > > > different > > > > bit anymore. > > > > > > The patch series > > > alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes > > > care of vcpu migration and locking. > > > > > > > > > > Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon > > > > as > > > > we EOI the IRQ. > > > > > > Yes, but at that point we have also already migrated the corresponding > > > physical irq to the new pcpu. > > > > Even tho we the migration has been done, the 2 clear bit are not atomic. Let's > > imagine that the IRQ has fired between the two clear bit. In this case we may > > clear the ACTIVE bit by mistake. > > > > I don't see anything in this code which prevents a such configuration. > > Which 2 clear bit? GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq.
On Fri, 20 Jun 2014, Julien Grall wrote: > On 20 Jun 2014 18:41, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com> wrote: > > > > On Fri, 20 Jun 2014, Julien Grall wrote: > > > On 20/06/14 17:48, Stefano Stabellini wrote: > > > > > > > > > Your patch "physical irq follow virtual irq" won't even solve this problem > > > > > if > > > > > the maintenance interrupt is request to EOI the IRQ. > > > > > > > > I don't understand this statement. > > > > The maintenance interrupt is requested to make sure that the vcpu is > > > > interrupted as soon as it EOIs the virtual irq, so that > > > > gic_update_one_lr can run. > > > > > > I agree with that but ... the following step can happen > > > > > > 1) Xen receives the interrupt > > > 2) Xen writes EOIR > > > 3) Xen inject the IRQ to the guest > > > 4) The guest will receive the IRQ (i.e read IAR) > > > 5) Xen migrates the VCPU to another physical CPU > > > 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. > > > > > > In a such case, the IRQ will be EOIed to another physical CPU. If we assume > > > that we should always write to GICC_DIR of the physical CPU that receive the > > > interrupt, then even your patch "physcial irq follow virtual irq" won't solve > > > the problem. > > > > That's true. > > > > > > > > > > > The VGIC lock is per-vcpu. > > > > > > > > > > If this is happening while we migrate, nothing protect p->lr and the > > > > > different > > > > > bit anymore. > > > > > > > > The patch series > > > > alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes > > > > care of vcpu migration and locking. > > > > > > > > > > > > > Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon > > > > > as > > > > > we EOI the IRQ. > > > > > > > > Yes, but at that point we have also already migrated the corresponding > > > > physical irq to the new pcpu. > > > > > > Even tho we the migration has been done, the 2 clear bit are not atomic. Let's > > > imagine that the IRQ has fired between the two clear bit. In this case we may > > > clear the ACTIVE bit by mistake. > > > > > > I don't see anything in this code which prevents a such configuration. > > > > Which 2 clear bit? > > GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq. The code is protected by the vgic lock.
On 21/06/14 15:43, Stefano Stabellini wrote: > On Fri, 20 Jun 2014, Julien Grall wrote: >> On 20 Jun 2014 18:41, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com> wrote: >>> >>> On Fri, 20 Jun 2014, Julien Grall wrote: >>>> On 20/06/14 17:48, Stefano Stabellini wrote: >>>>> >>>>>> Your patch "physical irq follow virtual irq" won't even solve this problem >>>>>> if >>>>>> the maintenance interrupt is request to EOI the IRQ. >>>>> >>>>> I don't understand this statement. >>>>> The maintenance interrupt is requested to make sure that the vcpu is >>>>> interrupted as soon as it EOIs the virtual irq, so that >>>>> gic_update_one_lr can run. >>>> >>>> I agree with that but ... the following step can happen >>>> >>>> 1) Xen receives the interrupt >>>> 2) Xen writes EOIR >>>> 3) Xen inject the IRQ to the guest >>>> 4) The guest will receive the IRQ (i.e read IAR) >>>> 5) Xen migrates the VCPU to another physical CPU >>>> 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. >>>> >>>> In a such case, the IRQ will be EOIed to another physical CPU. If we assume >>>> that we should always write to GICC_DIR of the physical CPU that receive the >>>> interrupt, then even your patch "physcial irq follow virtual irq" won't solve >>>> the problem. >>> >>> That's true. >>> >>> >>> >>>>>> The VGIC lock is per-vcpu. >>>>>> >>>>>> If this is happening while we migrate, nothing protect p->lr and the >>>>>> different >>>>>> bit anymore. >>>>> >>>>> The patch series >>>>> alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes >>>>> care of vcpu migration and locking. >>>>> >>>>> >>>>>> Indeed, the vgic_inject_irq will use the new VCPU. This can happen as soon >>>>>> as >>>>>> we EOI the IRQ. >>>>> >>>>> Yes, but at that point we have also already migrated the corresponding >>>>> physical irq to the new pcpu. >>>> >>>> Even tho we the migration has been done, the 2 clear bit are not atomic. Let's >>>> imagine that the IRQ has fired between the two clear bit. In this case we may >>>> clear the ACTIVE bit by mistake. >>>> >>>> I don't see anything in this code which prevents a such configuration. >>> >>> Which 2 clear bit? >> >> GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq. > > The code is protected by the vgic lock. If the IRQ has been migrating while it's injected, the maintenance interrupt will use the previous VCPU and vgic_inject_irq will use the new one. 2 different lock protecting the same thing we but not mutually exclusive. As soon as we write in GICC_DIR, this case can happen.
On Sat, 21 Jun 2014, Julien Grall wrote: > On 21/06/14 15:43, Stefano Stabellini wrote: > > On Fri, 20 Jun 2014, Julien Grall wrote: > > > On 20 Jun 2014 18:41, "Stefano Stabellini" > > > <stefano.stabellini@eu.citrix.com> wrote: > > > > > > > > On Fri, 20 Jun 2014, Julien Grall wrote: > > > > > On 20/06/14 17:48, Stefano Stabellini wrote: > > > > > > > > > > > > > Your patch "physical irq follow virtual irq" won't even solve this > > > > > > > problem > > > > > > > if > > > > > > > the maintenance interrupt is request to EOI the IRQ. > > > > > > > > > > > > I don't understand this statement. > > > > > > The maintenance interrupt is requested to make sure that the vcpu is > > > > > > interrupted as soon as it EOIs the virtual irq, so that > > > > > > gic_update_one_lr can run. > > > > > > > > > > I agree with that but ... the following step can happen > > > > > > > > > > 1) Xen receives the interrupt > > > > > 2) Xen writes EOIR > > > > > 3) Xen inject the IRQ to the guest > > > > > 4) The guest will receive the IRQ (i.e read IAR) > > > > > 5) Xen migrates the VCPU to another physical CPU > > > > > 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. > > > > > > > > > > In a such case, the IRQ will be EOIed to another physical CPU. If we > > > > > assume > > > > > that we should always write to GICC_DIR of the physical CPU that > > > > > receive the > > > > > interrupt, then even your patch "physcial irq follow virtual irq" > > > > > won't solve > > > > > the problem. > > > > > > > > That's true. > > > > > > > > > > > > > > > > > > > The VGIC lock is per-vcpu. > > > > > > > > > > > > > > If this is happening while we migrate, nothing protect p->lr and > > > > > > > the > > > > > > > different > > > > > > > bit anymore. > > > > > > > > > > > > The patch series > > > > > > alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes > > > > > > care of vcpu migration and locking. > > > > > > > > > > > > > > > > > > > Indeed, the vgic_inject_irq will use the new VCPU. This can happen > > > > > > > as soon > > > > > > > as > > > > > > > we EOI the IRQ. > > > > > > > > > > > > Yes, but at that point we have also already migrated the > > > > > > corresponding > > > > > > physical irq to the new pcpu. > > > > > > > > > > Even tho we the migration has been done, the 2 clear bit are not > > > > > atomic. Let's > > > > > imagine that the IRQ has fired between the two clear bit. In this case > > > > > we may > > > > > clear the ACTIVE bit by mistake. > > > > > > > > > > I don't see anything in this code which prevents a such configuration. > > > > > > > > Which 2 clear bit? > > > > > > GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq. > > > > The code is protected by the vgic lock. > > If the IRQ has been migrating while it's injected, the maintenance interrupt > will use the previous VCPU and vgic_inject_irq will use the new one. 2 > different lock protecting the same thing we but not mutually exclusive. > > As soon as we write in GICC_DIR, this case can happen. I don't think this case can happen: the maintenance interrupt is synchronous, so it can happen either before the migration, or after the vcpu has been fully migrated and its execution has been resumed. GICC_DIR is written in response to a maintenance interrupt being fired, so GICC_DIR will be written either before the vcpu has been migrated or after.
On 21/06/14 16:26, Stefano Stabellini wrote: > On Sat, 21 Jun 2014, Julien Grall wrote: >> On 21/06/14 15:43, Stefano Stabellini wrote: >>> On Fri, 20 Jun 2014, Julien Grall wrote: >>>> On 20 Jun 2014 18:41, "Stefano Stabellini" >>>> <stefano.stabellini@eu.citrix.com> wrote: >>>>> >>>>> On Fri, 20 Jun 2014, Julien Grall wrote: >>>>>> On 20/06/14 17:48, Stefano Stabellini wrote: >>>>>>> >>>>>>>> Your patch "physical irq follow virtual irq" won't even solve this >>>>>>>> problem >>>>>>>> if >>>>>>>> the maintenance interrupt is request to EOI the IRQ. >>>>>>> >>>>>>> I don't understand this statement. >>>>>>> The maintenance interrupt is requested to make sure that the vcpu is >>>>>>> interrupted as soon as it EOIs the virtual irq, so that >>>>>>> gic_update_one_lr can run. >>>>>> >>>>>> I agree with that but ... the following step can happen >>>>>> >>>>>> 1) Xen receives the interrupt >>>>>> 2) Xen writes EOIR >>>>>> 3) Xen inject the IRQ to the guest >>>>>> 4) The guest will receive the IRQ (i.e read IAR) >>>>>> 5) Xen migrates the VCPU to another physical CPU >>>>>> 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. >>>>>> >>>>>> In a such case, the IRQ will be EOIed to another physical CPU. If we >>>>>> assume >>>>>> that we should always write to GICC_DIR of the physical CPU that >>>>>> receive the >>>>>> interrupt, then even your patch "physcial irq follow virtual irq" >>>>>> won't solve >>>>>> the problem. >>>>> >>>>> That's true. >>>>> >>>>> >>>>> >>>>>>>> The VGIC lock is per-vcpu. >>>>>>>> >>>>>>>> If this is happening while we migrate, nothing protect p->lr and >>>>>>>> the >>>>>>>> different >>>>>>>> bit anymore. >>>>>>> >>>>>>> The patch series >>>>>>> alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com takes >>>>>>> care of vcpu migration and locking. >>>>>>> >>>>>>> >>>>>>>> Indeed, the vgic_inject_irq will use the new VCPU. This can happen >>>>>>>> as soon >>>>>>>> as >>>>>>>> we EOI the IRQ. >>>>>>> >>>>>>> Yes, but at that point we have also already migrated the >>>>>>> corresponding >>>>>>> physical irq to the new pcpu. >>>>>> >>>>>> Even tho we the migration has been done, the 2 clear bit are not >>>>>> atomic. Let's >>>>>> imagine that the IRQ has fired between the two clear bit. In this case >>>>>> we may >>>>>> clear the ACTIVE bit by mistake. >>>>>> >>>>>> I don't see anything in this code which prevents a such configuration. >>>>> >>>>> Which 2 clear bit? >>>> >>>> GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq. >>> >>> The code is protected by the vgic lock. >> >> If the IRQ has been migrating while it's injected, the maintenance interrupt >> will use the previous VCPU and vgic_inject_irq will use the new one. 2 >> different lock protecting the same thing we but not mutually exclusive. >> >> As soon as we write in GICC_DIR, this case can happen. > > I don't think this case can happen: the maintenance interrupt is > synchronous, so it can happen either before the migration, or after the > vcpu has been fully migrated and its execution has been resumed. > GICC_DIR is written in response to a maintenance interrupt being fired, > so GICC_DIR will be written either before the vcpu has been migrated or > after. I agree that the maintenance interrupt synchronous... but for this part I was talking talking about VIRQ migration (i.e the guest is writing in ITARGETSR). 1) The guest is receiving the IRQ 2) The guest is writing in ITARGETSR to move the IRQ to another VCPU 3) The guest is EOIing the IRQ and Xen receive a maintenance interrupt. You set the affinity of the physical IRQ directly in the VGIC ITARGETSR emulation. So as soon as we write in GICC_DIR, this IRQ may fired again on another physical CPU (assuming the new VCPU is currently running). In this case, vgic_inject_irq will use the new VGIC lock and nothing will prevent concurrent access to clear/set the bit GUEST_VISIBLE and GUEST_ACTIVE.
On Sat, 21 Jun 2014, Julien Grall wrote: > On 21/06/14 16:26, Stefano Stabellini wrote: > > On Sat, 21 Jun 2014, Julien Grall wrote: > > > On 21/06/14 15:43, Stefano Stabellini wrote: > > > > On Fri, 20 Jun 2014, Julien Grall wrote: > > > > > On 20 Jun 2014 18:41, "Stefano Stabellini" > > > > > <stefano.stabellini@eu.citrix.com> wrote: > > > > > > > > > > > > On Fri, 20 Jun 2014, Julien Grall wrote: > > > > > > > On 20/06/14 17:48, Stefano Stabellini wrote: > > > > > > > > > > > > > > > > > Your patch "physical irq follow virtual irq" won't even solve > > > > > > > > > this > > > > > > > > > problem > > > > > > > > > if > > > > > > > > > the maintenance interrupt is request to EOI the IRQ. > > > > > > > > > > > > > > > > I don't understand this statement. > > > > > > > > The maintenance interrupt is requested to make sure that the > > > > > > > > vcpu is > > > > > > > > interrupted as soon as it EOIs the virtual irq, so that > > > > > > > > gic_update_one_lr can run. > > > > > > > > > > > > > > I agree with that but ... the following step can happen > > > > > > > > > > > > > > 1) Xen receives the interrupt > > > > > > > 2) Xen writes EOIR > > > > > > > 3) Xen inject the IRQ to the guest > > > > > > > 4) The guest will receive the IRQ (i.e read IAR) > > > > > > > 5) Xen migrates the VCPU to another physical CPU > > > > > > > 6) The guest writes into GIC_DIR and a maintenance IRQ is fired. > > > > > > > > > > > > > > In a such case, the IRQ will be EOIed to another physical CPU. If > > > > > > > we > > > > > > > assume > > > > > > > that we should always write to GICC_DIR of the physical CPU that > > > > > > > receive the > > > > > > > interrupt, then even your patch "physcial irq follow virtual irq" > > > > > > > won't solve > > > > > > > the problem. > > > > > > > > > > > > That's true. > > > > > > > > > > > > > > > > > > > > > > > > > > > The VGIC lock is per-vcpu. > > > > > > > > > > > > > > > > > > If this is happening while we migrate, nothing protect p->lr > > > > > > > > > and > > > > > > > > > the > > > > > > > > > different > > > > > > > > > bit anymore. > > > > > > > > > > > > > > > > The patch series > > > > > > > > alpine.DEB.2.02.1406111607450.13771@kaball.uk.xensource.com > > > > > > > > takes > > > > > > > > care of vcpu migration and locking. > > > > > > > > > > > > > > > > > > > > > > > > > Indeed, the vgic_inject_irq will use the new VCPU. This can > > > > > > > > > happen > > > > > > > > > as soon > > > > > > > > > as > > > > > > > > > we EOI the IRQ. > > > > > > > > > > > > > > > > Yes, but at that point we have also already migrated the > > > > > > > > corresponding > > > > > > > > physical irq to the new pcpu. > > > > > > > > > > > > > > Even tho we the migration has been done, the 2 clear bit are not > > > > > > > atomic. Let's > > > > > > > imagine that the IRQ has fired between the two clear bit. In this > > > > > > > case > > > > > > > we may > > > > > > > clear the ACTIVE bit by mistake. > > > > > > > > > > > > > > I don't see anything in this code which prevents a such > > > > > > > configuration. > > > > > > > > > > > > Which 2 clear bit? > > > > > > > > > > GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq. > > > > > > > > The code is protected by the vgic lock. > > > > > > If the IRQ has been migrating while it's injected, the maintenance > > > interrupt > > > will use the previous VCPU and vgic_inject_irq will use the new one. 2 > > > different lock protecting the same thing we but not mutually exclusive. > > > > > > As soon as we write in GICC_DIR, this case can happen. > > > > I don't think this case can happen: the maintenance interrupt is > > synchronous, so it can happen either before the migration, or after the > > vcpu has been fully migrated and its execution has been resumed. > > GICC_DIR is written in response to a maintenance interrupt being fired, > > so GICC_DIR will be written either before the vcpu has been migrated or > > after. > > I agree that the maintenance interrupt synchronous... but for this part I was > talking talking about VIRQ migration (i.e the guest is writing in ITARGETSR). > > 1) The guest is receiving the IRQ > 2) The guest is writing in ITARGETSR to move the IRQ to another VCPU > 3) The guest is EOIing the IRQ and Xen receive a maintenance interrupt. > > You set the affinity of the physical IRQ directly in the VGIC ITARGETSR > emulation. So as soon as we write in GICC_DIR, this IRQ may fired again on > another physical CPU (assuming the new VCPU is currently running). In this > case, vgic_inject_irq will use the new VGIC lock and nothing will prevent > concurrent access to clear/set the bit GUEST_VISIBLE and GUEST_ACTIVE. This is a valid concern. I thought about this concurrency problem and I think I managed to solve it correctly. The solution I used was introducing a new flag called "GIC_IRQ_GUEST_MIGRATING". See: 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com
On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote: > I thought about this concurrency problem and I think I managed to > solve it correctly. The solution I used was introducing a new flag > called "GIC_IRQ_GUEST_MIGRATING". See: > > 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com So, I'm not sure whether I should apply this patch now or wait for that series to go in first. What should I do? Ian.
On 06/27/2014 04:51 PM, Ian Campbell wrote: > On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote: >> I thought about this concurrency problem and I think I managed to >> solve it correctly. The solution I used was introducing a new flag >> called "GIC_IRQ_GUEST_MIGRATING". See: >> >> 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com > > So, I'm not sure whether I should apply this patch now or wait for that > series to go in first. What should I do? IIRC, I've asked Stefano to rewrote a bit the following paragraph. " > No need to find the pcpu that needs to write to GICC_DIR, because after > "physical irq follow virtual irq" we always inject the virtual irq on > the vcpu that is running on the pcpu that received the interrupt. " The "physical irq follow virtual irq" is part of the migration series but this is not the reason that make GICC_DIR working. This is GIC specific (even though, I can't find a clearly define behavior in the specification). Regards,
Hi Stefano, The commit title doesn't reflect the patch. You don't add a new platform_need_explicit_eoi but a new quirk. On 06/20/2014 02:35 PM, Stefano Stabellini wrote: > +/* > + * Quirk for platforms where GICH_LR_HW does not work as expected. > + */ > +#define PLATFORM_QUIRK_NEED_EOI (1 << 1) > + I would rename to PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI. So people will directly know from the name that it's only for guest pirq. Regards,
On Wed, 2 Jul 2014, Julien Grall wrote: > Hi Stefano, > > The commit title doesn't reflect the patch. You don't add a new > platform_need_explicit_eoi but a new quirk. OK > On 06/20/2014 02:35 PM, Stefano Stabellini wrote: > > +/* > > + * Quirk for platforms where GICH_LR_HW does not work as expected. > > + */ > > +#define PLATFORM_QUIRK_NEED_EOI (1 << 1) > > + > > I would rename to PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI. So people will > directly know from the name that it's only for guest pirq. OK
On Mon, 30 Jun 2014, Julien Grall wrote: > On 06/27/2014 04:51 PM, Ian Campbell wrote: > > On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote: > >> I thought about this concurrency problem and I think I managed to > >> solve it correctly. The solution I used was introducing a new flag > >> called "GIC_IRQ_GUEST_MIGRATING". See: > >> > >> 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com > > > > So, I'm not sure whether I should apply this patch now or wait for that > > series to go in first. What should I do? > > IIRC, I've asked Stefano to rewrote a bit the following paragraph. > > " > > No need to find the pcpu that needs to write to GICC_DIR, because after > > "physical irq follow virtual irq" we always inject the virtual irq on > > the vcpu that is running on the pcpu that received the interrupt. > " > > The "physical irq follow virtual irq" is part of the migration series > but this is not the reason that make GICC_DIR working. This is GIC > specific (even though, I can't find a clearly define behavior in the > specification). Yes, I'll rewrite and resend.
On Fri, 27 Jun 2014, Ian Campbell wrote: > On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote: > > I thought about this concurrency problem and I think I managed to > > solve it correctly. The solution I used was introducing a new flag > > called "GIC_IRQ_GUEST_MIGRATING". See: > > > > 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com > > So, I'm not sure whether I should apply this patch now or wait for that > series to go in first. What should I do? You can go ahead and apply it. I have just sent v2 with a better commit message: <1404315229-31047-1-git-send-email-stefano.stabellini@eu.citrix.com>
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 58233cc..8695078 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -577,7 +577,9 @@ static inline void gic_set_lr(int lr, struct pending_irq *p, lr_val = state | (GIC_PRI_TO_GUEST(p->priority) << GICH_LR_PRIORITY_SHIFT) | ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT); - if ( p->desc != NULL ) + if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) ) + lr_val |= GICH_LR_MAINTENANCE_IRQ; + else if ( p->desc != NULL ) lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT); GICH[GICH_LR + lr] = lr_val; @@ -656,6 +658,11 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq, gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq)); } +static void gic_irq_eoi(int irq) +{ + GICC[GICC_DIR] = irq; +} + static void gic_update_one_lr(struct vcpu *v, int i) { struct pending_irq *p; @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i) clear_bit(i, &this_cpu(lr_mask)); if ( p->desc != NULL ) + { p->desc->status &= ~IRQ_INPROGRESS; + if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) ) + gic_irq_eoi(p->irq); + } clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); p->lr = GIC_INVALID_LR; diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index c9dd63c..5396c46 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -37,7 +37,7 @@ static bool reset_vals_valid = false; static uint32_t xgene_storm_quirks(void) { - return PLATFORM_QUIRK_GIC_64K_STRIDE; + return PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_NEED_EOI; } static int map_one_mmio(struct domain *d, const char *what, diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h index bcd2097..572f5b1 100644 --- a/xen/include/asm-arm/platform.h +++ b/xen/include/asm-arm/platform.h @@ -55,6 +55,11 @@ struct platform_desc { */ #define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0) +/* + * Quirk for platforms where GICH_LR_HW does not work as expected. + */ +#define PLATFORM_QUIRK_NEED_EOI (1 << 1) + void __init platform_init(void); int __init platform_init_time(void); int __init platform_specific_mapping(struct domain *d);
GICH_LR_HW doesn't work as expected on X-Gene: request maintenance interrupts and perform EOIs in the hypervisor as a workaround. Trigger this behaviour with a per platform option. No need to find the pcpu that needs to write to GICC_DIR, because after "physical irq follow virtual irq" we always inject the virtual irq on the vcpu that is running on the pcpu that received the interrupt. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: psawargaonkar@apm.com CC: apatel@apm.com --- xen/arch/arm/gic.c | 13 ++++++++++++- xen/arch/arm/platforms/xgene-storm.c | 2 +- xen/include/asm-arm/platform.h | 5 +++++ 3 files changed, 18 insertions(+), 2 deletions(-)