| Message ID | 20180305160415.16760-41-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
On 05/03/18 16:03, Andre Przywara wrote: > The priority 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. > There is a corner case when we change the priority of a pending > interrupt which we don't handle at the moment. I don't believe it is a corner case. The spec (8.9.12 ARM IHI 0069d) says: "Implementations must ensure that an interrupt that is pending at the time of the write uses either the old value or the new value and must ensure that the interrupt is neither lost nor handled more than once. The effect of the change must be visible in finite time." So the current implementation looks compliant to the spec. > > This is based on Linux commit dd238ec2b87b, written by Andre Przywara. Using short commit ID is usually a pretty bad idea because it may not be uniq. For instance, a git show on my Linux tree will not be able to find it. The full commit ID can feel a bit too long, so usually I give the short commit ID and the title. But in that context, the commit message looks wrong. From my tree, it seems it is 055658bf48fcc6afdf90810e7e8f4e98f486c0d2. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog RFC ... v1: > - use 32 bit register types > > xen/arch/arm/vgic/vgic-mmio-v2.c | 2 +- > xen/arch/arm/vgic/vgic-mmio.c | 47 ++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic/vgic-mmio.h | 7 ++++++ > xen/arch/arm/vgic/vgic.h | 2 ++ > 4 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index c93455fbb2..29db9dec6f 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -98,7 +98,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > 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, > + vgic_mmio_read_priority, vgic_mmio_write_priority, 8, > VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, > 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 c44d67082f..538f08bc66 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.c > +++ b/xen/arch/arm/vgic/vgic-mmio.c > @@ -384,6 +384,53 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu, > } > } > > +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8); > + unsigned int i; > + uint32_t val = 0; > + > + for ( i = 0; i < len; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + val |= (uint32_t)irq->priority << (i * 8); > + > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return val; > +} > + > +/* > + * We currently don't handle changing the priority of an interrupt that > + * is already pending on a VCPU. If there is a need for this, we would > + * need to make this VCPU exit and re-evaluate the priorities, potentially > + * leading to this interrupt getting presented now to the guest (if it has > + * been masked by the priority mask before). > + */ > +void vgic_mmio_write_priority(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8); > + unsigned int i; > + unsigned long flags; > + > + for ( i = 0; i < len; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + spin_lock_irqsave(&irq->irq_lock, flags); > + /* Narrow the priority range to what we actually support */ > + irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS); > + spin_unlock_irqrestore(&irq->irq_lock, flags); > + > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > 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 8604720628..e3f9029344 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.h > +++ b/xen/arch/arm/vgic/vgic-mmio.h > @@ -129,6 +129,13 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu, > paddr_t addr, unsigned int len, > unsigned long val); > > +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu, > + paddr_t addr, unsigned int len); > + > +void vgic_mmio_write_priority(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 > diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h > index 68e205d10a..b294b04391 100644 > --- a/xen/arch/arm/vgic/vgic.h > +++ b/xen/arch/arm/vgic/vgic.h > @@ -20,6 +20,8 @@ > #define PRODUCT_ID_XEN 0x58 /* ASCII code X */ > #define IMPLEMENTER_ARM 0x43b > > +#define VGIC_PRI_BITS 5 > + > #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS) > > static inline bool irq_is_pending(struct vgic_irq *irq) > Cheers,
Hi, On 08/03/18 15:48, Julien Grall wrote: > > > On 05/03/18 16:03, Andre Przywara wrote: >> The priority 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. >> There is a corner case when we change the priority of a pending >> interrupt which we don't handle at the moment. > > I don't believe it is a corner case. The spec (8.9.12 ARM IHI 0069d) says: > > "Implementations must ensure that an interrupt that is pending at the > time of the write uses either the old value or the new value and must > ensure that the interrupt is neither lost nor handled more than once. > The effect of the change must be visible in finite time." > > So the current implementation looks compliant to the spec. Interestingly the GICv2 spec doesn't mention anything at all related to that, at least not in the IPRIORITYR description. Anyway, are you saying that I should just change the commit message? >> >> This is based on Linux commit dd238ec2b87b, written by Andre Przywara. > > Using short commit ID is usually a pretty bad idea because it may not be > uniq. For instance, a git show on my Linux tree will not be able to find > it. Indeed somehow the commit ID is wrong. Linux recommends (at least) 12 digits when space is important, plus the commit title, if possible. But this is just for the records, so I just went with the shortened SHA. Even if that clashes in the future, the first hit should be the one. The commit message stem should be the same as in Linux for those commits. Cheers, Andre. > The full commit ID can feel a bit too long, so usually I give the short > commit ID and the title. > > But in that context, the commit message looks wrong. From my tree, it > seems it is 055658bf48fcc6afdf90810e7e8f4e98f486c0d2. > >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> Changelog RFC ... v1: >> - use 32 bit register types >> >> xen/arch/arm/vgic/vgic-mmio-v2.c | 2 +- >> xen/arch/arm/vgic/vgic-mmio.c | 47 >> ++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vgic/vgic-mmio.h | 7 ++++++ >> xen/arch/arm/vgic/vgic.h | 2 ++ >> 4 files changed, 57 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c >> b/xen/arch/arm/vgic/vgic-mmio-v2.c >> index c93455fbb2..29db9dec6f 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c >> @@ -98,7 +98,7 @@ static const struct vgic_register_region >> vgic_v2_dist_registers[] = { >> 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, >> + vgic_mmio_read_priority, vgic_mmio_write_priority, 8, >> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, >> 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 c44d67082f..538f08bc66 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.c >> +++ b/xen/arch/arm/vgic/vgic-mmio.c >> @@ -384,6 +384,53 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu, >> } >> } >> +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len) >> +{ >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8); >> + unsigned int i; >> + uint32_t val = 0; >> + >> + for ( i = 0; i < len; i++ ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >> + i); >> + >> + val |= (uint32_t)irq->priority << (i * 8); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + } >> + >> + return val; >> +} >> + >> +/* >> + * We currently don't handle changing the priority of an interrupt that >> + * is already pending on a VCPU. If there is a need for this, we would >> + * need to make this VCPU exit and re-evaluate the priorities, >> potentially >> + * leading to this interrupt getting presented now to the guest (if >> it has >> + * been masked by the priority mask before). >> + */ >> +void vgic_mmio_write_priority(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8); >> + unsigned int i; >> + unsigned long flags; >> + >> + for ( i = 0; i < len; i++ ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid >> + i); >> + >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + /* Narrow the priority range to what we actually support */ >> + irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - >> VGIC_PRI_BITS); >> + spin_unlock_irqrestore(&irq->irq_lock, flags); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + } >> +} >> + >> 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 8604720628..e3f9029344 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.h >> +++ b/xen/arch/arm/vgic/vgic-mmio.h >> @@ -129,6 +129,13 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu, >> paddr_t addr, unsigned int len, >> unsigned long val); >> +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len); >> + >> +void vgic_mmio_write_priority(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 >> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h >> index 68e205d10a..b294b04391 100644 >> --- a/xen/arch/arm/vgic/vgic.h >> +++ b/xen/arch/arm/vgic/vgic.h >> @@ -20,6 +20,8 @@ >> #define PRODUCT_ID_XEN 0x58 /* ASCII code X */ >> #define IMPLEMENTER_ARM 0x43b >> +#define VGIC_PRI_BITS 5 >> + >> #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS) >> static inline bool irq_is_pending(struct vgic_irq *irq) >> > > Cheers, >
On 08/03/18 16:21, Andre Przywara wrote: > Hi, > > On 08/03/18 15:48, Julien Grall wrote: >> >> >> On 05/03/18 16:03, Andre Przywara wrote: >>> The priority 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. >>> There is a corner case when we change the priority of a pending >>> interrupt which we don't handle at the moment. >> >> I don't believe it is a corner case. The spec (8.9.12 ARM IHI 0069d) says: >> >> "Implementations must ensure that an interrupt that is pending at the >> time of the write uses either the old value or the new value and must >> ensure that the interrupt is neither lost nor handled more than once. >> The effect of the change must be visible in finite time." >> >> So the current implementation looks compliant to the spec. > > Interestingly the GICv2 spec doesn't mention anything at all related to > that, at least not in the IPRIORITYR description. > > Anyway, are you saying that I should just change the commit message? Yes. > >>> >>> This is based on Linux commit dd238ec2b87b, written by Andre Przywara. >> >> Using short commit ID is usually a pretty bad idea because it may not be >> uniq. For instance, a git show on my Linux tree will not be able to find >> it. > > Indeed somehow the commit ID is wrong. > Linux recommends (at least) 12 digits when space is important, plus the > commit title, if possible. But this is just for the records, so I just > went with the shortened SHA. Well, it is not only just for the records. It is also a way for us to know where it comes from so we can find for the signed-off-by/authorship. So we need to easily find the exact commit and therefore the e-mail addresses. > Even if that clashes in the future, the > first hit should be the one. The commit message stem should be the same > as in Linux for those commits. Why the first hit should be the one? It likely means another commit have similar sha1 and likely going to be the new one. And usually git show will tell you "ambiguous". Cheers,
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c index c93455fbb2..29db9dec6f 100644 --- a/xen/arch/arm/vgic/vgic-mmio-v2.c +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c @@ -98,7 +98,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { 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, + vgic_mmio_read_priority, vgic_mmio_write_priority, 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, 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 c44d67082f..538f08bc66 100644 --- a/xen/arch/arm/vgic/vgic-mmio.c +++ b/xen/arch/arm/vgic/vgic-mmio.c @@ -384,6 +384,53 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu, } } +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8); + unsigned int i; + uint32_t val = 0; + + for ( i = 0; i < len; i++ ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + val |= (uint32_t)irq->priority << (i * 8); + + vgic_put_irq(vcpu->domain, irq); + } + + return val; +} + +/* + * We currently don't handle changing the priority of an interrupt that + * is already pending on a VCPU. If there is a need for this, we would + * need to make this VCPU exit and re-evaluate the priorities, potentially + * leading to this interrupt getting presented now to the guest (if it has + * been masked by the priority mask before). + */ +void vgic_mmio_write_priority(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 8); + unsigned int i; + unsigned long flags; + + for ( i = 0; i < len; i++ ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + spin_lock_irqsave(&irq->irq_lock, flags); + /* Narrow the priority range to what we actually support */ + irq->priority = (val >> (i * 8)) & GENMASK(7, 8 - VGIC_PRI_BITS); + spin_unlock_irqrestore(&irq->irq_lock, flags); + + vgic_put_irq(vcpu->domain, irq); + } +} + 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 8604720628..e3f9029344 100644 --- a/xen/arch/arm/vgic/vgic-mmio.h +++ b/xen/arch/arm/vgic/vgic-mmio.h @@ -129,6 +129,13 @@ void vgic_mmio_write_sactive(struct vcpu *vcpu, paddr_t addr, unsigned int len, unsigned long val); +unsigned long vgic_mmio_read_priority(struct vcpu *vcpu, + paddr_t addr, unsigned int len); + +void vgic_mmio_write_priority(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 diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h index 68e205d10a..b294b04391 100644 --- a/xen/arch/arm/vgic/vgic.h +++ b/xen/arch/arm/vgic/vgic.h @@ -20,6 +20,8 @@ #define PRODUCT_ID_XEN 0x58 /* ASCII code X */ #define IMPLEMENTER_ARM 0x43b +#define VGIC_PRI_BITS 5 + #define vgic_irq_is_sgi(intid) ((intid) < VGIC_NR_SGIS) static inline bool irq_is_pending(struct vgic_irq *irq)
The priority 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. There is a corner case when we change the priority of a pending interrupt which we don't handle at the moment. This is based on Linux commit dd238ec2b87b, written by Andre Przywara. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - use 32 bit register types xen/arch/arm/vgic/vgic-mmio-v2.c | 2 +- xen/arch/arm/vgic/vgic-mmio.c | 47 ++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vgic/vgic-mmio.h | 7 ++++++ xen/arch/arm/vgic/vgic.h | 2 ++ 4 files changed, 57 insertions(+), 1 deletion(-)