| Message ID | 20200304203330.4967-24-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v4: GICv4.1 architecture support | expand |
Hi Marc, On 2020/3/5 4:33, Marc Zyngier wrote: > The vgic-state debugfs file could do with showing the pending state > of the HW-backed SGIs. Plug it into the low-level code. > > Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com> Thanks > --- > virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c > index cc12fe9b2df3..b13a9e3f99dd 100644 > --- a/virt/kvm/arm/vgic/vgic-debug.c > +++ b/virt/kvm/arm/vgic/vgic-debug.c > @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, > struct kvm_vcpu *vcpu) > { > char *type; > + bool pending; > + > if (irq->intid < VGIC_NR_SGIS) > type = "SGI"; > else if (irq->intid < VGIC_NR_PRIVATE_IRQS) > @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, > if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS) > print_header(s, irq, vcpu); > > + pending = irq->pending_latch; > + if (irq->hw && vgic_irq_is_sgi(irq->intid)) { > + int err; > + > + err = irq_get_irqchip_state(irq->host_irq, > + IRQCHIP_STATE_PENDING, > + &pending); > + WARN_ON_ONCE(err); > + } > + > seq_printf(s, " %s %4d " > " %2d " > "%d%d%d%d%d%d%d " > @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, > "\n", > type, irq->intid, > (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1, > - irq->pending_latch, > + pending, > irq->line_level, > irq->active, > irq->enabled, >
Hi Marc, On 3/4/20 9:33 PM, Marc Zyngier wrote: > The vgic-state debugfs file could do with showing the pending state > of the HW-backed SGIs. Plug it into the low-level code. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c > index cc12fe9b2df3..b13a9e3f99dd 100644 > --- a/virt/kvm/arm/vgic/vgic-debug.c > +++ b/virt/kvm/arm/vgic/vgic-debug.c > @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, > struct kvm_vcpu *vcpu) > { > char *type; > + bool pending; nit: can be directly initialized to irq->pending_latch > + > if (irq->intid < VGIC_NR_SGIS) > type = "SGI"; > else if (irq->intid < VGIC_NR_PRIVATE_IRQS) > @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, > if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS) > print_header(s, irq, vcpu); > > + pending = irq->pending_latch; > + if (irq->hw && vgic_irq_is_sgi(irq->intid)) { > + int err; > + > + err = irq_get_irqchip_state(irq->host_irq, > + IRQCHIP_STATE_PENDING, > + &pending); > + WARN_ON_ONCE(err); > + } > + > seq_printf(s, " %s %4d " > " %2d " > "%d%d%d%d%d%d%d " > @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, > "\n", > type, irq->intid, > (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1, > - irq->pending_latch, > + pending, > irq->line_level, > irq->active, > irq->enabled, > The patch looks good to me but I am now lost about how we retrieve the pending stat of other hw mapped interrupts. Looks we use irq->pending_latch always. Is that correct? For the patch: Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
Hi Eric, On 2020-03-19 15:05, Auger Eric wrote: > Hi Marc, > > On 3/4/20 9:33 PM, Marc Zyngier wrote: >> The vgic-state debugfs file could do with showing the pending state >> of the HW-backed SGIs. Plug it into the low-level code. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-debug.c >> b/virt/kvm/arm/vgic/vgic-debug.c >> index cc12fe9b2df3..b13a9e3f99dd 100644 >> --- a/virt/kvm/arm/vgic/vgic-debug.c >> +++ b/virt/kvm/arm/vgic/vgic-debug.c >> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, >> struct vgic_irq *irq, >> struct kvm_vcpu *vcpu) >> { >> char *type; >> + bool pending; > nit: can be directly initialized to irq->pending_latch >> + >> if (irq->intid < VGIC_NR_SGIS) >> type = "SGI"; >> else if (irq->intid < VGIC_NR_PRIVATE_IRQS) >> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, >> struct vgic_irq *irq, >> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS) >> print_header(s, irq, vcpu); >> >> + pending = irq->pending_latch; >> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) { >> + int err; >> + >> + err = irq_get_irqchip_state(irq->host_irq, >> + IRQCHIP_STATE_PENDING, >> + &pending); >> + WARN_ON_ONCE(err); >> + } >> + >> seq_printf(s, " %s %4d " >> " %2d " >> "%d%d%d%d%d%d%d " >> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, >> struct vgic_irq *irq, >> "\n", >> type, irq->intid, >> (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1, >> - irq->pending_latch, >> + pending, >> irq->line_level, >> irq->active, >> irq->enabled, >> > The patch looks good to me but I am now lost about how we retrieve the > pending stat of other hw mapped interrupts. Looks we use > irq->pending_latch always. Is that correct? Correct. GICv4.0 doesn't give us an architectural way to look at the vLPI pending state (there isn't even a guarantee about when the GIC will stop writing to memory, if it ever does). With GICv4.1, you can introspect the HW state for SGIs. You can also look at the vLPI state by peeking at the virtual pending table, but you'd need to unmap the VPE first, which I obviously don't want to do for this debug interface, specially as it can be used whilst the guest is up and running. In the future, we'll have to implement that in order to support guest save/restore from a GICv4.1 system. I haven't given much thought to it though. > For the patch: > Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks, M.
Hi Marc, On 3/19/20 4:21 PM, Marc Zyngier wrote: > Hi Eric, > > On 2020-03-19 15:05, Auger Eric wrote: >> Hi Marc, >> >> On 3/4/20 9:33 PM, Marc Zyngier wrote: >>> The vgic-state debugfs file could do with showing the pending state >>> of the HW-backed SGIs. Plug it into the low-level code. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++- >>> 1 file changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-debug.c >>> b/virt/kvm/arm/vgic/vgic-debug.c >>> index cc12fe9b2df3..b13a9e3f99dd 100644 >>> --- a/virt/kvm/arm/vgic/vgic-debug.c >>> +++ b/virt/kvm/arm/vgic/vgic-debug.c >>> @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, >>> struct vgic_irq *irq, >>> struct kvm_vcpu *vcpu) >>> { >>> char *type; >>> + bool pending; >> nit: can be directly initialized to irq->pending_latch >>> + >>> if (irq->intid < VGIC_NR_SGIS) >>> type = "SGI"; >>> else if (irq->intid < VGIC_NR_PRIVATE_IRQS) >>> @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, >>> struct vgic_irq *irq, >>> if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS) >>> print_header(s, irq, vcpu); >>> >>> + pending = irq->pending_latch; >>> + if (irq->hw && vgic_irq_is_sgi(irq->intid)) { >>> + int err; >>> + >>> + err = irq_get_irqchip_state(irq->host_irq, >>> + IRQCHIP_STATE_PENDING, >>> + &pending); >>> + WARN_ON_ONCE(err); >>> + } >>> + >>> seq_printf(s, " %s %4d " >>> " %2d " >>> "%d%d%d%d%d%d%d " >>> @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, >>> struct vgic_irq *irq, >>> "\n", >>> type, irq->intid, >>> (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1, >>> - irq->pending_latch, >>> + pending, >>> irq->line_level, >>> irq->active, >>> irq->enabled, >>> >> The patch looks good to me but I am now lost about how we retrieve the >> pending stat of other hw mapped interrupts. Looks we use >> irq->pending_latch always. Is that correct? > > Correct. GICv4.0 doesn't give us an architectural way to look at the > vLPI pending state (there isn't even a guarantee about when the GIC > will stop writing to memory, if it ever does). > > With GICv4.1, you can introspect the HW state for SGIs. You can also > look at the vLPI state by peeking at the virtual pending table, but > you'd need to unmap the VPE first, which I obviously don't want to do > for this debug interface, specially as it can be used whilst the guest > is up and running. OK for vLPIs, what about other HW mapped IRQs (arch timer?) Eric > > In the future, we'll have to implement that in order to support guest > save/restore from a GICv4.1 system. I haven't given much thought to it > though. > >> For the patch: >> Reviewed-by: Eric Auger <eric.auger@redhat.com> > > Thanks, > > M.
Hi Eric, On 2020-03-19 15:43, Auger Eric wrote: > Hi Marc, > > On 3/19/20 4:21 PM, Marc Zyngier wrote: >> Hi Eric, [...] >>> The patch looks good to me but I am now lost about how we retrieve >>> the >>> pending stat of other hw mapped interrupts. Looks we use >>> irq->pending_latch always. Is that correct? >> >> Correct. GICv4.0 doesn't give us an architectural way to look at the >> vLPI pending state (there isn't even a guarantee about when the GIC >> will stop writing to memory, if it ever does). >> >> With GICv4.1, you can introspect the HW state for SGIs. You can also >> look at the vLPI state by peeking at the virtual pending table, but >> you'd need to unmap the VPE first, which I obviously don't want to do >> for this debug interface, specially as it can be used whilst the guest >> is up and running. > OK for vLPIs, what about other HW mapped IRQs (arch timer?) Different kind of HW. With those, the injection is still virtual, so the SW pending bit is still very much valid. You can actually try and make the timer interrupt pending, it should show up. What the irq->hw bit means is "this virtual interrupt is somehow related to the host_irq". How this is interpreted is completely context-dependent. Thanks, M.
Hi, On 3/19/20 5:16 PM, Marc Zyngier wrote: > Hi Eric, > > On 2020-03-19 15:43, Auger Eric wrote: >> Hi Marc, >> >> On 3/19/20 4:21 PM, Marc Zyngier wrote: >>> Hi Eric, > > [...] > >>>> The patch looks good to me but I am now lost about how we retrieve the >>>> pending stat of other hw mapped interrupts. Looks we use >>>> irq->pending_latch always. Is that correct? >>> >>> Correct. GICv4.0 doesn't give us an architectural way to look at the >>> vLPI pending state (there isn't even a guarantee about when the GIC >>> will stop writing to memory, if it ever does). >>> >>> With GICv4.1, you can introspect the HW state for SGIs. You can also >>> look at the vLPI state by peeking at the virtual pending table, but >>> you'd need to unmap the VPE first, which I obviously don't want to do >>> for this debug interface, specially as it can be used whilst the guest >>> is up and running. >> OK for vLPIs, what about other HW mapped IRQs (arch timer?) > > Different kind of HW. With those, the injection is still virtual, so the > SW pending bit is still very much valid. You can actually try and make > the timer interrupt pending, it should show up. > > What the irq->hw bit means is "this virtual interrupt is somehow related > to the host_irq". How this is interpreted is completely context-dependent. OK thank you for refreshing my memories ;-) Eric > > Thanks, > > M.
Hi Marc, On 2020/3/19 23:21, Marc Zyngier wrote: > With GICv4.1, you can introspect the HW state for SGIs. You can also > look at the vLPI state by peeking at the virtual pending table, but > you'd need to unmap the VPE first, Out of curiosity, could you please point me to the "unmap the VPE" requirement in the v4.1 spec? I'd like to have a look. Thanks! Zenghui
Hi Zenghui, On 2020-03-20 04:38, Zenghui Yu wrote: > Hi Marc, > > On 2020/3/19 23:21, Marc Zyngier wrote: >> With GICv4.1, you can introspect the HW state for SGIs. You can also >> look at the vLPI state by peeking at the virtual pending table, but >> you'd need to unmap the VPE first, > > Out of curiosity, could you please point me to the "unmap the VPE" > requirement in the v4.1 spec? I'd like to have a look. Sure. See IHI0069F, 5.3.19 (VMAPP GICv4.1), "Caching of virtual LPI data structures", and the bit that says: "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of the Virtual Pending Table and Virtual Configuration Table associated with the vPEID held in the GIC" which is what was crucially missing from the GICv4.0 spec (it doesn't say when the GIC is done writing to memory). Side note: it'd be good to know what the rules are for your own GICv4 implementations, so that we can at least make sure the current code is safe. Thanks, M.
Hi Marc, On 2020/3/20 17:09, Marc Zyngier wrote: > Hi Zenghui, > > On 2020-03-20 04:38, Zenghui Yu wrote: >> Hi Marc, >> >> On 2020/3/19 23:21, Marc Zyngier wrote: >>> With GICv4.1, you can introspect the HW state for SGIs. You can also >>> look at the vLPI state by peeking at the virtual pending table, but >>> you'd need to unmap the VPE first, >> >> Out of curiosity, could you please point me to the "unmap the VPE" >> requirement in the v4.1 spec? I'd like to have a look. > > Sure. See IHI0069F, 5.3.19 (VMAPP GICv4.1), "Caching of virtual LPI data > structures", and the bit that says: > > "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of the > Virtual Pending Table and Virtual Configuration Table associated with the > vPEID held in the GIC" > > which is what was crucially missing from the GICv4.0 spec (it doesn't say > when the GIC is done writing to memory). OK. Thanks for the pointer! > > Side note: it'd be good to know what the rules are for your own GICv4 > implementations, so that we can at least make sure the current code is > safe. As far as I know, there will be some clean and invalidate operations when v4.0 VPENDBASER.Valid gets programmed. But not sure about behaviors on VMAPP (unmap), it may be a totally v4.1 stuff. I'll have a talk with our SOC team. But how can the current code be unsafe? Is anywhere in the current code will peek/poke the vpt (whilst GIC continues writing things into it)? Thanks, Zenghui
On 2020-03-20 11:35, Zenghui Yu wrote: > Hi Marc, > > On 2020/3/20 17:09, Marc Zyngier wrote: >> Hi Zenghui, >> >> On 2020-03-20 04:38, Zenghui Yu wrote: >>> Hi Marc, >>> >>> On 2020/3/19 23:21, Marc Zyngier wrote: >>>> With GICv4.1, you can introspect the HW state for SGIs. You can also >>>> look at the vLPI state by peeking at the virtual pending table, but >>>> you'd need to unmap the VPE first, >>> >>> Out of curiosity, could you please point me to the "unmap the VPE" >>> requirement in the v4.1 spec? I'd like to have a look. >> >> Sure. See IHI0069F, 5.3.19 (VMAPP GICv4.1), "Caching of virtual LPI >> data >> structures", and the bit that says: >> >> "A VMAPP with {V,Alloc}=={0,1} cleans and invalidates any caching of >> the >> Virtual Pending Table and Virtual Configuration Table associated with >> the >> vPEID held in the GIC" >> >> which is what was crucially missing from the GICv4.0 spec (it doesn't >> say >> when the GIC is done writing to memory). > > OK. Thanks for the pointer! > >> >> Side note: it'd be good to know what the rules are for your own GICv4 >> implementations, so that we can at least make sure the current code is >> safe. > > As far as I know, there will be some clean and invalidate operations > when v4.0 VPENDBASER.Valid gets programmed. Interesting. The ideal behaviour would be that the VPT is up-to-date and the caches clean when Valid is cleared (and once Dirty flips to 0). > But not sure about behaviors > on VMAPP (unmap), it may be a totally v4.1 stuff. I'll have a talk with > our SOC team. The VMAPP stuff is purely v4.1. > But how can the current code be unsafe? Is anywhere in the current code > will peek/poke the vpt (whilst GIC continues writing things into it)? No. But on VM termination, the memory will be freed, and will eventually be reallocated. If the GIC can still write to that memory after it has been freed, you end-up with memory corruption... Which is why I'm curious of what ensures that on your implementation. I'd also like to know the same thing about the QC implementation, but there's nobody left to find out... Thanks, M.
Hi Marc, On 2020/3/20 19:46, Marc Zyngier wrote: >>> Side note: it'd be good to know what the rules are for your own GICv4 >>> implementations, so that we can at least make sure the current code >>> is safe. >> >> As far as I know, there will be some clean and invalidate operations >> when v4.0 VPENDBASER.Valid gets programmed. > > Interesting. The ideal behaviour would be that the VPT is up-to-date and > the caches clean when Valid is cleared (and once Dirty flips to 0). > >> But not sure about behaviors >> on VMAPP (unmap), it may be a totally v4.1 stuff. I'll have a talk with >> our SOC team. > > The VMAPP stuff is purely v4.1. > >> But how can the current code be unsafe? Is anywhere in the current code >> will peek/poke the vpt (whilst GIC continues writing things into it)? > > No. But on VM termination, the memory will be freed, and will eventually be > reallocated. If the GIC can still write to that memory after it has been > freed, you end-up with memory corruption... Which is why I'm curious of > what ensures that on your implementation. Ah, I got it. I will check it with HiSilicon people next week and go back to you if the code becomes unsafe due to the incomplete GICv4. Thanks, Zenghui
diff --git a/virt/kvm/arm/vgic/vgic-debug.c b/virt/kvm/arm/vgic/vgic-debug.c index cc12fe9b2df3..b13a9e3f99dd 100644 --- a/virt/kvm/arm/vgic/vgic-debug.c +++ b/virt/kvm/arm/vgic/vgic-debug.c @@ -178,6 +178,8 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, struct kvm_vcpu *vcpu) { char *type; + bool pending; + if (irq->intid < VGIC_NR_SGIS) type = "SGI"; else if (irq->intid < VGIC_NR_PRIVATE_IRQS) @@ -190,6 +192,16 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, if (irq->intid ==0 || irq->intid == VGIC_NR_PRIVATE_IRQS) print_header(s, irq, vcpu); + pending = irq->pending_latch; + if (irq->hw && vgic_irq_is_sgi(irq->intid)) { + int err; + + err = irq_get_irqchip_state(irq->host_irq, + IRQCHIP_STATE_PENDING, + &pending); + WARN_ON_ONCE(err); + } + seq_printf(s, " %s %4d " " %2d " "%d%d%d%d%d%d%d " @@ -201,7 +213,7 @@ static void print_irq_state(struct seq_file *s, struct vgic_irq *irq, "\n", type, irq->intid, (irq->target_vcpu) ? irq->target_vcpu->vcpu_id : -1, - irq->pending_latch, + pending, irq->line_level, irq->active, irq->enabled,
The vgic-state debugfs file could do with showing the pending state of the HW-backed SGIs. Plug it into the low-level code. Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/arm/vgic/vgic-debug.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)