| Message ID | 20180209143937.28866-36-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 09/02/18 14:39, Andre Przywara wrote: > The target register handlers are v2 emulation specific, so their > implementation lives entirely in vgic-mmio-v2.c. > We copy the old VGIC behaviour of assigning an IRQ to the first VCPU > set in the target mask instead of making it possibly pending on > multiple VCPUs. > > This is based on Linux commit 2c234d6f1826, written by Andre Przywara. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic/vgic-mmio-v2.c | 52 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index c0b88b347e..c59f2c1ba7 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -66,6 +66,56 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu, > } > } > > +static unsigned long vgic_mmio_read_target(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) Indentation > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); uint32_t > + int i; unsigned int > + u64 val = 0; Why 64-bit? IIRC, the target register is 32-bit. > + > + for ( i = 0; i < len; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, > + vcpu, intid + i); Indenation. > + > + val |= (u64)irq->targets << (i * 8); See above regarding u64. > + > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return val; > +} > + > +static void vgic_mmio_write_target(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) Indentation > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); uint32_t > + u8 cpu_mask = GENMASK(vcpu->domain->max_vcpus - 1, 0); uint32_t > + int i; unsigned int > + unsigned long flags; > + > + /* GICD_ITARGETSR[0-7] are read-only */ > + if ( intid < VGIC_NR_PRIVATE_IRQS ) > + return; > + > + for ( i = 0; i < len; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, > + NULL, intid + i); > + int target; unsigned int > + > + spin_lock_irqsave(&irq->irq_lock, flags); > + > + irq->targets = (val >> (i * 8)) & cpu_mask; > + target = irq->targets ? (ffs(irq->targets) - 1) : 0; Here you will route the IRQ to vCPU 0 if the mask is invalid. Is it intended? Do you have a pointer in the spec? > + irq->target_vcpu = vcpu->domain->vcpu[target]; > + > + spin_unlock_irqrestore(&irq->irq_lock, flags); > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > static const struct vgic_register_region vgic_v2_dist_registers[] = { > REGISTER_DESC_WITH_LENGTH(GICD_CTLR, > vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12, > @@ -95,7 +145,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL, > 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, > - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, > + vgic_mmio_read_target, vgic_mmio_write_target, NULL, NULL, 8, > VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICFGR, > vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2, > Cheers,
Hi, On 09/02/18 14:39, Andre Przywara wrote: > The target register handlers are v2 emulation specific, so their > implementation lives entirely in vgic-mmio-v2.c. > We copy the old VGIC behaviour of assigning an IRQ to the first VCPU > set in the target mask instead of making it possibly pending on > multiple VCPUs. > > This is based on Linux commit 2c234d6f1826, written by Andre Przywara. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic/vgic-mmio-v2.c | 52 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index c0b88b347e..c59f2c1ba7 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -66,6 +66,56 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu, > } > } > > +static unsigned long vgic_mmio_read_target(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); > + int i; > + u64 val = 0; > + > + for ( i = 0; i < len; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, > + vcpu, intid + i); > + > + val |= (u64)irq->targets << (i * 8); > + > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return val; > +} > + > +static void vgic_mmio_write_target(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); > + u8 cpu_mask = GENMASK(vcpu->domain->max_vcpus - 1, 0); > + int i; > + unsigned long flags; > + > + /* GICD_ITARGETSR[0-7] are read-only */ > + if ( intid < VGIC_NR_PRIVATE_IRQS ) > + return; > + > + for ( i = 0; i < len; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, > + NULL, intid + i); > + int target; > + > + spin_lock_irqsave(&irq->irq_lock, flags); > + > + irq->targets = (val >> (i * 8)) & cpu_mask; > + target = irq->targets ? (ffs(irq->targets) - 1) : 0; > + irq->target_vcpu = vcpu->domain->vcpu[target]; You should modify the affinity of the physical interrupt here. So it will fire on the pCPU where the new vCPU run. > + > + spin_unlock_irqrestore(&irq->irq_lock, flags); > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > static const struct vgic_register_region vgic_v2_dist_registers[] = { > REGISTER_DESC_WITH_LENGTH(GICD_CTLR, > vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12, > @@ -95,7 +145,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL, > 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, > - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, > + vgic_mmio_read_target, vgic_mmio_write_target, NULL, NULL, 8, > VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICFGR, > vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2, > Cheers,
Hi, On 19/02/18 11:53, Julien Grall wrote: > Hi Andre, > > On 09/02/18 14:39, Andre Przywara wrote: >> The target register handlers are v2 emulation specific, so their >> implementation lives entirely in vgic-mmio-v2.c. >> We copy the old VGIC behaviour of assigning an IRQ to the first VCPU >> set in the target mask instead of making it possibly pending on >> multiple VCPUs. >> >> This is based on Linux commit 2c234d6f1826, written by Andre Przywara. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic/vgic-mmio-v2.c | 52 >> +++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c >> b/xen/arch/arm/vgic/vgic-mmio-v2.c >> index c0b88b347e..c59f2c1ba7 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c >> @@ -66,6 +66,56 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu, >> } >> } >> +static unsigned long vgic_mmio_read_target(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len) > > Indentation > >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); > > uint32_t > >> + int i; > > unsigned int > >> + u64 val = 0; > > > Why 64-bit? IIRC, the target register is 32-bit. > > >> + >> + for ( i = 0; i < len; i++ ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, >> + vcpu, intid + i); > > Indenation. > >> + >> + val |= (u64)irq->targets << (i * 8); > > See above regarding u64. > >> + >> + vgic_put_irq(vcpu->domain, irq); >> + } >> + >> + return val; >> +} >> + >> +static void vgic_mmio_write_target(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) > > Indentation > >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); > > uint32_t > >> + u8 cpu_mask = GENMASK(vcpu->domain->max_vcpus - 1, 0); > > uint32_t I'd rather keep this at uint8_t, as this the natural data type for the TARGET register. > >> + int i; > > unsigned int > >> + unsigned long flags; >> + >> + /* GICD_ITARGETSR[0-7] are read-only */ >> + if ( intid < VGIC_NR_PRIVATE_IRQS ) >> + return; >> + >> + for ( i = 0; i < len; i++ ) >> + { >> + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, >> + NULL, intid + i); >> + int target; > > unsigned int > >> + >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + >> + irq->targets = (val >> (i * 8)) & cpu_mask; >> + target = irq->targets ? (ffs(irq->targets) - 1) : 0; > > Here you will route the IRQ to vCPU 0 if the mask is invalid. Is it > intended? Do you have a pointer in the spec? I think this was more of a KVM design decision to do so. Might be some GICv2 speciality. I tossed this question over to the KVM guys. For now I checked the code and AFAICS we handle target_vcpu being NULL properly (in terms of not forwarding the interrupt). So I would try to change this to properly use a NULL pointer here. Fixed the rest. Cheers, Andre >> + irq->target_vcpu = vcpu->domain->vcpu[target]; >> + >> + spin_unlock_irqrestore(&irq->irq_lock, flags); >> + vgic_put_irq(vcpu->domain, irq); >> + } >> +} >> + >> static const struct vgic_register_region vgic_v2_dist_registers[] = { >> REGISTER_DESC_WITH_LENGTH(GICD_CTLR, >> vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12, >> @@ -95,7 +145,7 @@ static const struct vgic_register_region >> vgic_v2_dist_registers[] = { >> vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL, >> 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, >> + vgic_mmio_read_target, vgic_mmio_write_target, NULL, NULL, 8, >> VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICFGR, >> vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2, >> > > Cheers, >
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c index c0b88b347e..c59f2c1ba7 100644 --- a/xen/arch/arm/vgic/vgic-mmio-v2.c +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c @@ -66,6 +66,56 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu, } } +static unsigned long vgic_mmio_read_target(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); + int i; + u64 val = 0; + + for ( i = 0; i < len; i++ ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, + vcpu, intid + i); + + val |= (u64)irq->targets << (i * 8); + + vgic_put_irq(vcpu->domain, irq); + } + + return val; +} + +static void vgic_mmio_write_target(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 8); + u8 cpu_mask = GENMASK(vcpu->domain->max_vcpus - 1, 0); + int i; + unsigned long flags; + + /* GICD_ITARGETSR[0-7] are read-only */ + if ( intid < VGIC_NR_PRIVATE_IRQS ) + return; + + for ( i = 0; i < len; i++ ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, + NULL, intid + i); + int target; + + spin_lock_irqsave(&irq->irq_lock, flags); + + irq->targets = (val >> (i * 8)) & cpu_mask; + target = irq->targets ? (ffs(irq->targets) - 1) : 0; + irq->target_vcpu = vcpu->domain->vcpu[target]; + + spin_unlock_irqrestore(&irq->irq_lock, flags); + vgic_put_irq(vcpu->domain, irq); + } +} + static const struct vgic_register_region vgic_v2_dist_registers[] = { REGISTER_DESC_WITH_LENGTH(GICD_CTLR, vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12, @@ -95,7 +145,7 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL, 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR, - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, + vgic_mmio_read_target, vgic_mmio_write_target, NULL, NULL, 8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICFGR, vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
The target register handlers are v2 emulation specific, so their implementation lives entirely in vgic-mmio-v2.c. We copy the old VGIC behaviour of assigning an IRQ to the first VCPU set in the target mask instead of making it possibly pending on multiple VCPUs. This is based on Linux commit 2c234d6f1826, written by Andre Przywara. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic/vgic-mmio-v2.c | 52 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)