| Message ID | 20180305160415.16760-40-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 05/03/18 16:03, Andre Przywara wrote: > The active register handlers are shared between the v2 and v3 emulation, > so their implementation goes into vgic-mmio.c, to be easily referenced > from the v3 emulation as well later. > Since activation/deactivation of an interrupt may happen entirely in the > guest without it ever exiting, we need some extra logic to properly track > the active state. > For clearing the active state, we would basically have to halt the guest > to make sure this is properly propagated into the respective VCPUs. > This is not yet implemented in Xen. > Fortunately this feature is mostly used to reset a just in initialised > GIC, so chances are we are tasked to clear bits that are already zero. > Add some simple check to avoid a pointless warning in this case. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog RFC ... v1: > - remove premature "proper ACTIVE" handler stub > - avoid unnecessary warnings on NO-OP register writes > - extend comments > > xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- > xen/arch/arm/vgic/vgic-mmio.c | 103 +++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic/vgic-mmio.h | 11 +++++ > 3 files changed, 116 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index efdd73301d..c93455fbb2 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -92,10 +92,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, > + vgic_mmio_read_active, vgic_mmio_write_sactive, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, > + vgic_mmio_read_active, vgic_mmio_write_cactive, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, > vgic_mmio_read_raz, vgic_mmio_write_wi, 8, > diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c > index 2e939d5e39..c44d67082f 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.c > +++ b/xen/arch/arm/vgic/vgic-mmio.c > @@ -281,6 +281,109 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, > } > } > > +/* > + * The actual active bit for a virtual IRQ is held in the LR. Our shadow > + * copy in struct vgic_irq is only synced when needed and may not be > + * up-to-date all of the time. > + * Returning the actual active state is quite costly (stopping all > + * VCPUs processing any affected vIRQs), so we use a simple implementation > + * to get the best possible answer. > + */ > +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); > + uint32_t value = 0; > + unsigned int i; > + > + /* Loop over all IRQs affected by this read */ > + for ( i = 0; i < len * 8; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + if ( irq->active ) > + value |= (1U << i); > + > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return value; > +} > + > +/* > + * We don't actually support clearing the active state of an IRQ (yet). > + * However there is a chance that most guests use this for initialization. > + * We check whether this MMIO access would actually affect any active IRQ, > + * and only print our warning in this case. So clearing already non-active > + * IRQs would not be moaned about in the logs. > + */ > +void vgic_mmio_write_cactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); > + unsigned int i; > + bool bail_out = false; > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + /* > + * If we know that the IRQ is active or we can't be sure about > + * it (because it is currently in a CPU), log the not properly > + * emulated MMIO access. > + */ > + if ( irq->active || irq->vcpu ) > + { > + gdprintk(XENLOG_ERR, > + "%pv: vGICD: IRQ%d: clearing active state not supported\n", s/%d/%u/ > + vcpu, irq->intid); gdprintk will always print the vCPU. Thought it is the current which might be different from vcpu (mostly in the re-dist case). So I would use dprintk(XENLOG_G_ERR, "%pv: ..."). I would even be tempt to use printk(....) so we can spot potential issue on non-debug build. > + bail_out = true; I admit the bailout is a bit weird here. You would only print the warning for the first activated IRQ and give the impression it is fine for the rest. So maybe you want to drop IRQ%d? > + } > + > + vgic_put_irq(vcpu->domain, irq); > + if ( bail_out ) > + return; > + } > +} > + > +/* > + * We don't actually support setting the active state of an IRQ (yet). > + * We check whether this MMIO access would actually affect any non-active IRQ, > + * and only print our warning in this case. > + */ > +void vgic_mmio_write_sactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) See my comments on cactive. > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); > + unsigned int i; > + bool bail_out = false; > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + /* > + * If we know that the IRQ is not active or we can't be sure about > + * it (because it is currently in a CPU), log the not properly > + * emulated MMIO access. > + */ > + if ( !irq->active || irq->vcpu ) > + { > + gdprintk(XENLOG_ERR, > + "%pv: vGICD: IRQ%d: setting active state not supported\n", > + vcpu, irq->intid); > + bail_out = true; > + } > + > + vgic_put_irq(vcpu->domain, irq); > + if ( bail_out ) > + return; > + } > +} > + > static int match_region(const void *key, const void *elt) > { > const unsigned int offset = (unsigned long)key; > diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h > index 4465f3b7e5..8604720628 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.h > +++ b/xen/arch/arm/vgic/vgic-mmio.h > @@ -118,6 +118,17 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, > paddr_t addr, unsigned int len, > unsigned long val); > > +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, > + paddr_t addr, unsigned int len); > + > +void vgic_mmio_write_cactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val); > + > +void vgic_mmio_write_sactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val); > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); > > #endif > Cheers,
Hi, On 08/03/18 15:39, Julien Grall wrote: > Hi Andre, > > On 05/03/18 16:03, Andre Przywara wrote: >> The active register handlers are shared between the v2 and v3 emulation, >> so their implementation goes into vgic-mmio.c, to be easily referenced >> from the v3 emulation as well later. >> Since activation/deactivation of an interrupt may happen entirely in the >> guest without it ever exiting, we need some extra logic to properly track >> the active state. >> For clearing the active state, we would basically have to halt the guest >> to make sure this is properly propagated into the respective VCPUs. >> This is not yet implemented in Xen. >> Fortunately this feature is mostly used to reset a just in initialised >> GIC, so chances are we are tasked to clear bits that are already zero. >> Add some simple check to avoid a pointless warning in this case. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> Changelog RFC ... v1: >> - remove premature "proper ACTIVE" handler stub >> - avoid unnecessary warnings on NO-OP register writes >> - extend comments >> >> xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- >> xen/arch/arm/vgic/vgic-mmio.c | 103 >> +++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vgic/vgic-mmio.h | 11 +++++ >> 3 files changed, 116 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c >> b/xen/arch/arm/vgic/vgic-mmio-v2.c >> index efdd73301d..c93455fbb2 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c >> @@ -92,10 +92,10 @@ static const struct vgic_register_region >> vgic_v2_dist_registers[] = { >> vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, >> + vgic_mmio_read_active, vgic_mmio_write_sactive, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, >> + vgic_mmio_read_active, vgic_mmio_write_cactive, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, >> vgic_mmio_read_raz, vgic_mmio_write_wi, 8, >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c >> b/xen/arch/arm/vgic/vgic-mmio.c >> index 2e939d5e39..c44d67082f 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.c >> +++ b/xen/arch/arm/vgic/vgic-mmio.c >> @@ -281,6 +281,109 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, >> } >> } >> +/* >> + * The actual active bit for a virtual IRQ is held in the LR. Our shadow >> + * copy in struct vgic_irq is only synced when needed and may not be >> + * up-to-date all of the time. >> + * Returning the actual active state is quite costly (stopping all >> + * VCPUs processing any affected vIRQs), so we use a simple >> implementation >> + * to get the best possible answer. >> + */ >> +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len) >> +{ >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); >> + uint32_t value = 0; >> + unsigned int i; >> + >> + /* Loop over all IRQs affected by this read */ >> + for ( i = 0; i < len * 8; i++ ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >> + i); >> + >> + if ( irq->active ) >> + value |= (1U << i); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + } >> + >> + return value; >> +} >> + >> +/* >> + * We don't actually support clearing the active state of an IRQ (yet). >> + * However there is a chance that most guests use this for >> initialization. >> + * We check whether this MMIO access would actually affect any active >> IRQ, >> + * and only print our warning in this case. So clearing already >> non-active >> + * IRQs would not be moaned about in the logs. >> + */ >> +void vgic_mmio_write_cactive(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); >> + unsigned int i; >> + bool bail_out = false; >> + >> + for_each_set_bit( i, &val, len * 8 ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >> + i); >> + >> + /* >> + * If we know that the IRQ is active or we can't be sure about >> + * it (because it is currently in a CPU), log the not properly >> + * emulated MMIO access. >> + */ >> + if ( irq->active || irq->vcpu ) >> + { >> + gdprintk(XENLOG_ERR, >> + "%pv: vGICD: IRQ%d: clearing active state not >> supported\n", > > s/%d/%u/ > >> + vcpu, irq->intid); > > gdprintk will always print the vCPU. Thought it is the current which > might be different from vcpu (mostly in the re-dist case). Ah, thanks. I always get confused about what which version of *printk does. > So I would use dprintk(XENLOG_G_ERR, "%pv: ..."). I would even be tempt > to use printk(....) so we can spot potential issue on non-debug build. Well, in the true spirit of Xen paranoia ;-) I wanted to avoid a guest spamming the console. And in the end there is nothing a administrator could really do about it. In my experience those messages tend to really scare users ("I could boot Dom0 but I see those error messages ...."). >> + bail_out = true; > > I admit the bailout is a bit weird here. You would only print the > warning for the first activated IRQ and give the impression it is fine > for the rest. So maybe you want to drop IRQ%d? For the above reasons I wanted to keep them concise, so that we see that the issue has happened, but avoid getting tons of error messages about the same problem (as this may affect up to 32 IRQs). But for debugging it might be good to know which IRQ was affected. I see two use cases for a guest: - (De-)activating a single IRQ: we get one message and know which IRQ it was, so an admin can chase this down to a certain device (driver). - (De-)activating *every* IRQ in this range (~0): we still get one message per 32 IRQs, but can see whether it covers SPIs only (IRQ>=32) and which ones. So what about a compromise: I use dprintk(XENLOG_G_ERR, "%pv ...), print the (first) IRQ and the *value* to be written. So a knowledgeable admin can tell whether it's a single IRQ or a "clear/set-all" case. That should also give enough info for debugging, but keeps it short. Does that sound OK? Cheers, Andre. >> + } >> + >> + vgic_put_irq(vcpu->domain, irq); >> + if ( bail_out ) >> + return; >> + } >> +} >> + >> +/* >> + * We don't actually support setting the active state of an IRQ (yet). >> + * We check whether this MMIO access would actually affect any >> non-active IRQ, >> + * and only print our warning in this case. >> + */ >> +void vgic_mmio_write_sactive(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) > > See my comments on cactive. > >> +{ >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); >> + unsigned int i; >> + bool bail_out = false; >> + >> + for_each_set_bit( i, &val, len * 8 ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >> + i); >> + >> + /* >> + * If we know that the IRQ is not active or we can't be sure >> about >> + * it (because it is currently in a CPU), log the not properly >> + * emulated MMIO access. >> + */ >> + if ( !irq->active || irq->vcpu ) >> + { >> + gdprintk(XENLOG_ERR, >> + "%pv: vGICD: IRQ%d: setting active state not >> supported\n", >> + vcpu, irq->intid); >> + bail_out = true; >> + } >> + >> + vgic_put_irq(vcpu->domain, irq); >> + if ( bail_out ) >> + return; >> + } >> +} >> + >> static int match_region(const void *key, const void *elt) >> { >> const unsigned int offset = (unsigned long)key; >> diff --git a/xen/arch/arm/vgic/vgic-mmio.h >> b/xen/arch/arm/vgic/vgic-mmio.h >> index 4465f3b7e5..8604720628 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.h >> +++ b/xen/arch/arm/vgic/vgic-mmio.h >> @@ -118,6 +118,17 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, >> paddr_t addr, unsigned int len, >> unsigned long val); >> +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len); >> + >> +void vgic_mmio_write_cactive(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val); >> + >> +void vgic_mmio_write_sactive(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val); >> + >> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); >> #endif >> > > Cheers, >
Hi Andre, On 13/03/18 17:02, Andre Przywara wrote: > On 08/03/18 15:39, Julien Grall wrote: >> On 05/03/18 16:03, Andre Przywara wrote: >>> +/* >>> + * We don't actually support clearing the active state of an IRQ (yet). >>> + * However there is a chance that most guests use this for >>> initialization. >>> + * We check whether this MMIO access would actually affect any active >>> IRQ, >>> + * and only print our warning in this case. So clearing already >>> non-active >>> + * IRQs would not be moaned about in the logs. >>> + */ >>> +void vgic_mmio_write_cactive(struct vcpu *vcpu, >>> + paddr_t addr, unsigned int len, >>> + unsigned long val) >>> +{ >>> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); >>> + unsigned int i; >>> + bool bail_out = false; >>> + >>> + for_each_set_bit( i, &val, len * 8 ) >>> + { >>> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >>> + i); >>> + >>> + /* >>> + * If we know that the IRQ is active or we can't be sure about >>> + * it (because it is currently in a CPU), log the not properly >>> + * emulated MMIO access. >>> + */ >>> + if ( irq->active || irq->vcpu ) >>> + { >>> + gdprintk(XENLOG_ERR, >>> + "%pv: vGICD: IRQ%d: clearing active state not >>> supported\n", >> >> s/%d/%u/ >> >>> + vcpu, irq->intid); >> >> gdprintk will always print the vCPU. Thought it is the current which >> might be different from vcpu (mostly in the re-dist case). > > Ah, thanks. I always get confused about what which version of *printk does. > >> So I would use dprintk(XENLOG_G_ERR, "%pv: ..."). I would even be tempt >> to use printk(....) so we can spot potential issue on non-debug build. > > Well, in the true spirit of Xen paranoia ;-) I wanted to avoid a guest > spamming the console. The guests messages are rate limited. > And in the end there is nothing a administrator > could really do about it. In my experience those messages tend to really > scare users ("I could boot Dom0 but I see those error messages ...."). Xen message are not only here for the administrator, they are also here to help for the developer to get log to dissect. I think that particular message should be printed in non-debug build because if the interrupt was active and can't clear it. Then something will go wrong later on. > >>> + bail_out = true; >> >> I admit the bailout is a bit weird here. You would only print the >> warning for the first activated IRQ and give the impression it is fine >> for the rest. So maybe you want to drop IRQ%d? > > For the above reasons I wanted to keep them concise, so that we see that > the issue has happened, but avoid getting tons of error messages about > the same problem (as this may affect up to 32 IRQs). > But for debugging it might be good to know which IRQ was affected. I see > two use cases for a guest: > - (De-)activating a single IRQ: we get one message and know which IRQ it > was, so an admin can chase this down to a certain device (driver). > - (De-)activating *every* IRQ in this range (~0): we still get one > message per 32 IRQs, but can see whether it covers SPIs only (IRQ>=32) > and which ones. > > So what about a compromise: I use dprintk(XENLOG_G_ERR, "%pv ...), print > the (first) IRQ and the *value* to be written. So a knowledgeable admin > can tell whether it's a single IRQ or a "clear/set-all" case. That > should also give enough info for debugging, but keeps it short. I can't see how a knowledgeable admin will be able to know that IRQ 2 is active with just the register value. > > Does that sound OK? I would still prefer the one per IRQ and using printk(XENLOG_G_*). I don't much care about the spam, see why above. Cheers,
On 13/03/18 17:14, Julien Grall wrote: > On 13/03/18 17:02, Andre Przywara wrote: >> On 08/03/18 15:39, Julien Grall wrote: >>> On 05/03/18 16:03, Andre Przywara wrote: >>> I admit the bailout is a bit weird here. You would only print the >>> warning for the first activated IRQ and give the impression it is fine >>> for the rest. So maybe you want to drop IRQ%d? >> >> For the above reasons I wanted to keep them concise, so that we see that >> the issue has happened, but avoid getting tons of error messages about >> the same problem (as this may affect up to 32 IRQs). >> But for debugging it might be good to know which IRQ was affected. I see >> two use cases for a guest: >> - (De-)activating a single IRQ: we get one message and know which IRQ it >> was, so an admin can chase this down to a certain device (driver). >> - (De-)activating *every* IRQ in this range (~0): we still get one >> message per 32 IRQs, but can see whether it covers SPIs only (IRQ>=32) >> and which ones. >> >> So what about a compromise: I use dprintk(XENLOG_G_ERR, "%pv ...), print >> the (first) IRQ and the *value* to be written. So a knowledgeable admin >> can tell whether it's a single IRQ or a "clear/set-all" case. That >> should also give enough info for debugging, but keeps it short. > > I can't see how a knowledgeable admin will be able to know that IRQ 2 is > active with just the register value. > >> >> Does that sound OK? > > I would still prefer the one per IRQ and using printk(XENLOG_G_*). I > don't much care about the spam, see why above. XENLOG_G_DEBUG is a good candidate actually. Cheers,
Hi, On 13/03/18 17:14, Julien Grall wrote: > Hi Andre, > > On 13/03/18 17:02, Andre Przywara wrote: >> On 08/03/18 15:39, Julien Grall wrote: >>> On 05/03/18 16:03, Andre Przywara wrote: >>>> +/* >>>> + * We don't actually support clearing the active state of an IRQ >>>> (yet). >>>> + * However there is a chance that most guests use this for >>>> initialization. >>>> + * We check whether this MMIO access would actually affect any active >>>> IRQ, >>>> + * and only print our warning in this case. So clearing already >>>> non-active >>>> + * IRQs would not be moaned about in the logs. >>>> + */ >>>> +void vgic_mmio_write_cactive(struct vcpu *vcpu, >>>> + paddr_t addr, unsigned int len, >>>> + unsigned long val) >>>> +{ >>>> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); >>>> + unsigned int i; >>>> + bool bail_out = false; >>>> + >>>> + for_each_set_bit( i, &val, len * 8 ) >>>> + { >>>> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >>>> + i); >>>> + >>>> + /* >>>> + * If we know that the IRQ is active or we can't be sure about >>>> + * it (because it is currently in a CPU), log the not properly >>>> + * emulated MMIO access. >>>> + */ >>>> + if ( irq->active || irq->vcpu ) >>>> + { >>>> + gdprintk(XENLOG_ERR, >>>> + "%pv: vGICD: IRQ%d: clearing active state not >>>> supported\n", >>> >>> s/%d/%u/ >>> >>>> + vcpu, irq->intid); >>> >>> gdprintk will always print the vCPU. Thought it is the current which >>> might be different from vcpu (mostly in the re-dist case). >> >> Ah, thanks. I always get confused about what which version of *printk >> does. >> >>> So I would use dprintk(XENLOG_G_ERR, "%pv: ..."). I would even be tempt >>> to use printk(....) so we can spot potential issue on non-debug build. >> >> Well, in the true spirit of Xen paranoia ;-) I wanted to avoid a guest >> spamming the console. > > The guests messages are rate limited. Ah ... >> And in the end there is nothing a administrator >> could really do about it. In my experience those messages tend to really >> scare users ("I could boot Dom0 but I see those error messages ...."). > > Xen message are not only here for the administrator, they are also here > to help for the developer to get log to dissect. Sure, see below ... > I think that particular message should be printed in non-debug build > because if the interrupt was active and can't clear it. Then something > will go wrong later on. Fair enough, if it's rate limited ... >>>> + bail_out = true; >>> >>> I admit the bailout is a bit weird here. You would only print the >>> warning for the first activated IRQ and give the impression it is fine >>> for the rest. So maybe you want to drop IRQ%d? >> >> For the above reasons I wanted to keep them concise, so that we see that >> the issue has happened, but avoid getting tons of error messages about >> the same problem (as this may affect up to 32 IRQs). >> But for debugging it might be good to know which IRQ was affected. I see >> two use cases for a guest: >> - (De-)activating a single IRQ: we get one message and know which IRQ it >> was, so an admin can chase this down to a certain device (driver). >> - (De-)activating *every* IRQ in this range (~0): we still get one >> message per 32 IRQs, but can see whether it covers SPIs only (IRQ>=32) >> and which ones. >> >> So what about a compromise: I use dprintk(XENLOG_G_ERR, "%pv ...), print >> the (first) IRQ and the *value* to be written. So a knowledgeable admin >> can tell whether it's a single IRQ or a "clear/set-all" case. That >> should also give enough info for debugging, but keeps it short. > > I can't see how a knowledgeable admin will be able to know that IRQ 2 is > active with just the register value. Well, I was assuming that a really knowledgeable admin would somehow forward the error message either to the ML or at least to $search_engine. ... >> Does that sound OK? > > I would still prefer the one per IRQ and using printk(XENLOG_G_*). I really don't think one per IRQ is too useful. A developer however can easily deal with "IRQ45, value: 0x00802000" from a log. And can deduce from there that it's about IRQ45 and IRQ55. Following the example above you would either see one "IRQ32, value: 0xffffffff" or "IRQ 45, value: 0x00002000". That looks like a good compromise between readability (having the IRQ number for admins) and brevity. I changed it now to output: %pv: vGICD: clearing active state not supported (IRQ%u, value: 0x%08lx) > I don't much care about the spam, see why above. Having them on the console between Dom0 messages is really scary, but not helpful if it's *more* than one. Since it's a known limitation of the VGIC emulation, not a real "error" in that sense. Cheers, Andre.
Hi, On 13/03/18 17:34, Andre Przywara wrote: > On 13/03/18 17:14, Julien Grall wrote: >> On 13/03/18 17:02, Andre Przywara wrote: >>> On 08/03/18 15:39, Julien Grall wrote: >>>> On 05/03/18 16:03, Andre Przywara wrote: >> I can't see how a knowledgeable admin will be able to know that IRQ 2 is >> active with just the register value. > > Well, I was assuming that a really knowledgeable admin would somehow > forward the error message either to the ML or at least to $search_engine. > ... Surely, but it does not mean the message should be clueless for the developer. I would rather no spent 10 min to try to find out what's going on where reading logs... > >>> Does that sound OK? >> >> I would still prefer the one per IRQ and using printk(XENLOG_G_*). > > I really don't think one per IRQ is too useful. A developer however can > easily deal with "IRQ45, value: 0x00802000" from a log. And can deduce > from there that it's about IRQ45 and IRQ55. Following the example above > you would either see one "IRQ32, value: 0xffffffff" or "IRQ 45, value: > 0x00002000". I still can't see how the developer would know the IRQ55 is active or not. That's the whole purpose of the per IRQ. > That looks like a good compromise between readability (having the IRQ > number for admins) and brevity. You may save 10 characters on the logs, you likely going to waste 10 min of the developer to understand what that messages really mean. > > I changed it now to output: > %pv: vGICD: clearing active state not supported (IRQ%u, value: 0x%08lx) > >> I don't much care about the spam, see why above. > > Having them on the console between Dom0 messages is really scary, but > not helpful if it's *more* than one. Since it's a known limitation of > the VGIC emulation, not a real "error" in that sense. It is the same as having any Xen messages interleaved with Dom0 messages. If the user is not happy with that, then it can divert Dom0 console to another UART. Cheers,
Hi, On 13/03/18 17:42, Julien Grall wrote: > Hi, > > On 13/03/18 17:34, Andre Przywara wrote: >> On 13/03/18 17:14, Julien Grall wrote: >>> On 13/03/18 17:02, Andre Przywara wrote: >>>> On 08/03/18 15:39, Julien Grall wrote: >>>>> On 05/03/18 16:03, Andre Przywara wrote: >>> I can't see how a knowledgeable admin will be able to know that IRQ 2 is >>> active with just the register value. >> >> Well, I was assuming that a really knowledgeable admin would somehow >> forward the error message either to the ML or at least to $search_engine. >> ... > > Surely, but it does not mean the message should be clueless for the > developer. I would rather no spent 10 min to try to find out what's > going on where reading logs... > >> >>>> Does that sound OK? >>> >>> I would still prefer the one per IRQ and using printk(XENLOG_G_*). >> >> I really don't think one per IRQ is too useful. A developer however can >> easily deal with "IRQ45, value: 0x00802000" from a log. And can deduce >> from there that it's about IRQ45 and IRQ55. Following the example above >> you would either see one "IRQ32, value: 0xffffffff" or "IRQ 45, value: >> 0x00002000". > > I still can't see how the developer would know the IRQ55 is active or > not. That's the whole purpose of the per IRQ. > >> That looks like a good compromise between readability (having the IRQ >> number for admins) and brevity. > > You may save 10 characters on the logs, you likely going to waste 10 min > of the developer to understand what that messages really mean. It's not about 10 characters, it's about 31 *lines*. At the moment a write to I[CS]ACTIVER triggers *one* line in the log: %pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d Now a write to this register would potentially trigger *32* lines: %pv: vGICD: IRQ%u: setting active state not supported By dumping the line as I proposed, I basically mimic the current line, plus give some information about one IRQ affected: %pv: vGICD: setting active state not supported (IRQ%u, value: 0x%08lx) So this is not a regression, but an improvement. Cheers, Andre. >> I changed it now to output: >> %pv: vGICD: clearing active state not supported (IRQ%u, value: 0x%08lx) >> >>> I don't much care about the spam, see why above. >> >> Having them on the console between Dom0 messages is really scary, but >> not helpful if it's *more* than one. Since it's a known limitation of >> the VGIC emulation, not a real "error" in that sense. > > It is the same as having any Xen messages interleaved with Dom0 > messages. If the user is not happy with that, then it can divert Dom0 > console to another UART. > > Cheers, >
Hi, On 03/14/2018 02:30 PM, Andre Przywara wrote: > Hi, > > On 13/03/18 17:42, Julien Grall wrote: >> Hi, >> >> On 13/03/18 17:34, Andre Przywara wrote: >>> On 13/03/18 17:14, Julien Grall wrote: >>>> On 13/03/18 17:02, Andre Przywara wrote: >>>>> On 08/03/18 15:39, Julien Grall wrote: >>>>>> On 05/03/18 16:03, Andre Przywara wrote: >>>> I can't see how a knowledgeable admin will be able to know that IRQ 2 is >>>> active with just the register value. >>> >>> Well, I was assuming that a really knowledgeable admin would somehow >>> forward the error message either to the ML or at least to $search_engine. >>> ... >> >> Surely, but it does not mean the message should be clueless for the >> developer. I would rather no spent 10 min to try to find out what's >> going on where reading logs... >> >>> >>>>> Does that sound OK? >>>> >>>> I would still prefer the one per IRQ and using printk(XENLOG_G_*). >>> >>> I really don't think one per IRQ is too useful. A developer however can >>> easily deal with "IRQ45, value: 0x00802000" from a log. And can deduce >>> from there that it's about IRQ45 and IRQ55. Following the example above >>> you would either see one "IRQ32, value: 0xffffffff" or "IRQ 45, value: >>> 0x00002000". >> >> I still can't see how the developer would know the IRQ55 is active or >> not. That's the whole purpose of the per IRQ. >> >>> That looks like a good compromise between readability (having the IRQ >>> number for admins) and brevity. >> >> You may save 10 characters on the logs, you likely going to waste 10 min >> of the developer to understand what that messages really mean. > > It's not about 10 characters, it's about 31 *lines*. > At the moment a write to I[CS]ACTIVER triggers *one* line in the log: > %pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d > > Now a write to this register would potentially trigger *32* lines: > %pv: vGICD: IRQ%u: setting active state not supported Very likely there will 0 lines printed because clearing an active interrupt should never happen in Xen guest today. So if there is 32 lines printed, then having 32 lines in the log is your last of your concern. > > By dumping the line as I proposed, I basically mimic the current line, > plus give some information about one IRQ affected: > %pv: vGICD: setting active state not supported (IRQ%u, value: 0x%08lx) > > So this is not a regression, but an improvement. I never said it was a regression. I said your new message and bail out is counter-intuitive because the developer can't guess if there are other IRQs active with just "value". If you want to make an improvement, do it properly. In that case, I am only asking to drop the counter-intuitive bail_out. Cheers.
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c index efdd73301d..c93455fbb2 100644 --- a/xen/arch/arm/vgic/vgic-mmio-v2.c +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c @@ -92,10 +92,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, + vgic_mmio_read_active, vgic_mmio_write_sactive, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, + vgic_mmio_read_active, vgic_mmio_write_cactive, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, vgic_mmio_read_raz, vgic_mmio_write_wi, 8, diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c index 2e939d5e39..c44d67082f 100644 --- a/xen/arch/arm/vgic/vgic-mmio.c +++ b/xen/arch/arm/vgic/vgic-mmio.c @@ -281,6 +281,109 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, } } +/* + * The actual active bit for a virtual IRQ is held in the LR. Our shadow + * copy in struct vgic_irq is only synced when needed and may not be + * up-to-date all of the time. + * Returning the actual active state is quite costly (stopping all + * VCPUs processing any affected vIRQs), so we use a simple implementation + * to get the best possible answer. + */ +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); + uint32_t value = 0; + unsigned int i; + + /* Loop over all IRQs affected by this read */ + for ( i = 0; i < len * 8; i++ ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + if ( irq->active ) + value |= (1U << i); + + vgic_put_irq(vcpu->domain, irq); + } + + return value; +} + +/* + * We don't actually support clearing the active state of an IRQ (yet). + * However there is a chance that most guests use this for initialization. + * We check whether this MMIO access would actually affect any active IRQ, + * and only print our warning in this case. So clearing already non-active + * IRQs would not be moaned about in the logs. + */ +void vgic_mmio_write_cactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); + unsigned int i; + bool bail_out = false; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + /* + * If we know that the IRQ is active or we can't be sure about + * it (because it is currently in a CPU), log the not properly + * emulated MMIO access. + */ + if ( irq->active || irq->vcpu ) + { + gdprintk(XENLOG_ERR, + "%pv: vGICD: IRQ%d: clearing active state not supported\n", + vcpu, irq->intid); + bail_out = true; + } + + vgic_put_irq(vcpu->domain, irq); + if ( bail_out ) + return; + } +} + +/* + * We don't actually support setting the active state of an IRQ (yet). + * We check whether this MMIO access would actually affect any non-active IRQ, + * and only print our warning in this case. + */ +void vgic_mmio_write_sactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); + unsigned int i; + bool bail_out = false; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + /* + * If we know that the IRQ is not active or we can't be sure about + * it (because it is currently in a CPU), log the not properly + * emulated MMIO access. + */ + if ( !irq->active || irq->vcpu ) + { + gdprintk(XENLOG_ERR, + "%pv: vGICD: IRQ%d: setting active state not supported\n", + vcpu, irq->intid); + bail_out = true; + } + + vgic_put_irq(vcpu->domain, irq); + if ( bail_out ) + return; + } +} + static int match_region(const void *key, const void *elt) { const unsigned int offset = (unsigned long)key; diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h index 4465f3b7e5..8604720628 100644 --- a/xen/arch/arm/vgic/vgic-mmio.h +++ b/xen/arch/arm/vgic/vgic-mmio.h @@ -118,6 +118,17 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, paddr_t addr, unsigned int len, unsigned long val); +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, + paddr_t addr, unsigned int len); + +void vgic_mmio_write_cactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val); + +void vgic_mmio_write_sactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val); + unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); #endif
The active register handlers are shared between the v2 and v3 emulation, so their implementation goes into vgic-mmio.c, to be easily referenced from the v3 emulation as well later. Since activation/deactivation of an interrupt may happen entirely in the guest without it ever exiting, we need some extra logic to properly track the active state. For clearing the active state, we would basically have to halt the guest to make sure this is properly propagated into the respective VCPUs. This is not yet implemented in Xen. Fortunately this feature is mostly used to reset a just in initialised GIC, so chances are we are tasked to clear bits that are already zero. Add some simple check to avoid a pointless warning in this case. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - remove premature "proper ACTIVE" handler stub - avoid unnecessary warnings on NO-OP register writes - extend comments xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- xen/arch/arm/vgic/vgic-mmio.c | 103 +++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vgic/vgic-mmio.h | 11 +++++ 3 files changed, 116 insertions(+), 2 deletions(-)