| Message ID | 20180209143937.28866-31-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: > As the enable register handlers are shared between the v2 and v3 > emulation, their implementation goes into vgic-mmio.c, to be easily > referenced from the v3 emulation as well later. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- > xen/arch/arm/vgic/vgic-mmio.c | 114 +++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic/vgic-mmio.h | 11 ++++ > 3 files changed, 127 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index 0926b3243e..eca6840ff9 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -74,10 +74,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, > + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, > + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR, > vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, > diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c > index 59703a6909..3d9fa02a10 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.c > +++ b/xen/arch/arm/vgic/vgic-mmio.c > @@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, > /* Ignore */ > } > > +/* > + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value > + * of the enabled bit, so there is only one function for both here. > + */ > +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) Indentation. > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); uint32_t here please. > + u32 value = 0; Same here. > + 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->enabled ) > + value |= (1U << i); > + > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return value; > +} > + > +static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type, Looking below irq_type should a enum vgic_irq_config and not an int. > + bool enable) Indentation. > +{ > + unsigned long flags; > + > +// irq_set_affinity(desc, cpumask_of(v_target->processor)); Why is that commented? > + spin_lock_irqsave(&desc->lock, flags); > + if ( enable ) > + { > + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? > + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); Indentation and I would prefer a helper to convert between the vgic value and the IRQ_TYPE. This would make the code easier to read. Also, this code does not replicate correctly the current vGIC. gic_set_irq_type is only allowed to be used when irq_set_type_by_domain(d) returns true. If you consider this change valid, then I would like to know why. > + desc->handler->enable(desc); > + } > + else > + desc->handler->disable(desc); > + spin_unlock_irqrestore(&desc->lock, flags); > +} > + > +void vgic_mmio_write_senable(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) Indentation. > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); uint32_t. > + irq_desc_t *desc; > + int i; > + unsigned long flags; > + enum vgic_irq_config config; > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq; > + > + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + spin_lock_irqsave(&irq->irq_lock, flags); > + irq->enabled = true; > + if ( irq->hw ) > + { > + /* > + * The irq cannot be a PPI, we only support delivery > + * of SPIs to guests. > + */ > + ASSERT(irq->hwintid >= 32); > + > + desc = irq_to_desc(irq->hwintid); What is the rationale behind storing hwintid rather than the irq_desc directly? > + config = irq->config; > + } > + else > + desc = NULL; > + vgic_queue_irq_unlock(vcpu->domain, irq, flags); > + > + vgic_put_irq(vcpu->domain, irq); > + > + if ( desc ) > + vgic_handle_hardware_irq(desc, config, true); This is slightly strange. You handle the hardware IRQ outside the virtual IRQ lock. It means that the hardware IRQ may end up enabled but the virtual IRQ disabled. > + } > +} > + > +void vgic_mmio_write_cenable(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + int i; > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq; > + unsigned long flags; > + irq_desc_t *desc; > + > + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + spin_lock_irqsave(&irq->irq_lock, flags); > + > + irq->enabled = false; > + > + if ( irq->hw ) > + desc = irq_to_desc(irq->hwintid); > + else > + desc = NULL; > + > + spin_unlock_irqrestore(&irq->irq_lock, flags); > + vgic_put_irq(vcpu->domain, irq); > + > + if ( desc ) > + vgic_handle_hardware_irq(desc, 0, false); Same remark here. > + } > +} > + > 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 10ac682296..9f34bd1aec 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.h > +++ b/xen/arch/arm/vgic/vgic-mmio.h > @@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu, > void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, > unsigned int len, unsigned long val); > > +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, > + paddr_t addr, unsigned int len); Indentation. > + > +void vgic_mmio_write_senable(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val); Ditto. > + > +void vgic_mmio_write_cenable(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val); Ditto. > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); > > /* Find the proper register handler entry given a certain address offset */ > Cheers,
Hi, On 16/02/18 16:57, Julien Grall wrote: > Hi Andre, > > On 09/02/18 14:39, Andre Przywara wrote: >> As the enable register handlers are shared between the v2 and v3 >> emulation, their implementation goes into vgic-mmio.c, to be easily >> referenced from the v3 emulation as well later. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- >> xen/arch/arm/vgic/vgic-mmio.c | 114 >> +++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vgic/vgic-mmio.h | 11 ++++ >> 3 files changed, 127 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c >> b/xen/arch/arm/vgic/vgic-mmio-v2.c >> index 0926b3243e..eca6840ff9 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c >> @@ -74,10 +74,10 @@ static const struct vgic_register_region >> vgic_v2_dist_registers[] = { >> vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR, >> vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c >> b/xen/arch/arm/vgic/vgic-mmio.c >> index 59703a6909..3d9fa02a10 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.c >> +++ b/xen/arch/arm/vgic/vgic-mmio.c >> @@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t >> addr, >> /* Ignore */ >> } >> +/* >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the >> value >> + * of the enabled bit, so there is only one function for both here. >> + */ >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len) > > Indentation. > >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > > uint32_t here please. > >> + u32 value = 0; > > Same here. > >> + 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->enabled ) >> + value |= (1U << i); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + } >> + >> + return value; >> +} >> + >> +static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type, > > Looking below irq_type should a enum vgic_irq_config and not an int. > >> + bool enable) > > Indentation. > >> +{ >> + unsigned long flags; >> + >> +// irq_set_affinity(desc, cpumask_of(v_target->processor)); > > Why is that commented? That should indeed be a TODO:, actually. As we already discussed, KVM does not implement this hardware-follows-virtual affinity. This line is just copied from the old VGIC, to remind me to address this. But I need to check the locking order here first and if there are any other side effects of changing the hardware affinity in this particular context. >> + spin_lock_irqsave(&desc->lock, flags); >> + if ( enable ) >> + { >> + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? >> + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); > > Indentation and I would prefer a helper to convert between the vgic > value and the IRQ_TYPE. This would make the code easier to read. > > Also, this code does not replicate correctly the current vGIC. > gic_set_irq_type is only allowed to be used when > irq_set_type_by_domain(d) returns true. If you consider this change > valid, then I would like to know why. So what is/was the rationale for not allowing IRQ type changes for non-privileged guests? If you allow to pass through an hardware IRQ to a guest (which is the case this function handles), then I don't see why a guest would not be allowed to change the configuration? It seems rather odd, I guess it's up to the guest to know which type of IRQ this is? >> + desc->handler->enable(desc); >> + } >> + else >> + desc->handler->disable(desc); >> + spin_unlock_irqrestore(&desc->lock, flags); >> +} >> + >> +void vgic_mmio_write_senable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) > > Indentation. > >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > > uint32_t. > >> + irq_desc_t *desc; >> + int i; >> + unsigned long flags; >> + enum vgic_irq_config config; >> + >> + for_each_set_bit( i, &val, len * 8 ) >> + { >> + struct vgic_irq *irq; >> + >> + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); >> + >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + irq->enabled = true; >> + if ( irq->hw ) >> + { >> + /* >> + * The irq cannot be a PPI, we only support delivery >> + * of SPIs to guests. >> + */ >> + ASSERT(irq->hwintid >= 32); >> + >> + desc = irq_to_desc(irq->hwintid); > > What is the rationale behind storing hwintid rather than the irq_desc > directly? > >> + config = irq->config; >> + } >> + else >> + desc = NULL; >> + vgic_queue_irq_unlock(vcpu->domain, irq, flags); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + >> + if ( desc ) >> + vgic_handle_hardware_irq(desc, config, true); > > This is slightly strange. You handle the hardware IRQ outside the > virtual IRQ lock. It means that the hardware IRQ may end up enabled but > the virtual IRQ disabled. Yeah, good catch. This can't be easily called before dropping the IRQ lock, as this would violate the locking order. But I can try to re-take the IRQ lock after having acquired the desc lock, then use the enabled (and config) value from struct vgic_irq directly. Cheers, Andre. > >> + } >> +} >> + >> +void vgic_mmio_write_cenable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); >> + int i; >> + >> + for_each_set_bit( i, &val, len * 8 ) >> + { >> + struct vgic_irq *irq; >> + unsigned long flags; >> + irq_desc_t *desc; >> + >> + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + >> + irq->enabled = false; >> + >> + if ( irq->hw ) >> + desc = irq_to_desc(irq->hwintid); >> + else >> + desc = NULL; >> + >> + spin_unlock_irqrestore(&irq->irq_lock, flags); >> + vgic_put_irq(vcpu->domain, irq); >> + >> + if ( desc ) >> + vgic_handle_hardware_irq(desc, 0, false); > > Same remark here. > >> + } >> +} >> + >> 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 10ac682296..9f34bd1aec 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.h >> +++ b/xen/arch/arm/vgic/vgic-mmio.h >> @@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu, >> void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, >> unsigned int len, unsigned long val); >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len); > > Indentation. > >> + >> +void vgic_mmio_write_senable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val); > > Ditto. > >> + >> +void vgic_mmio_write_cenable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val); > > Ditto. > >> + >> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); >> /* Find the proper register handler entry given a certain address >> offset */ >> > > Cheers, >
On 19/02/18 12:41, Andre Przywara wrote: > Hi, Hi, > On 16/02/18 16:57, Julien Grall wrote: >> On 09/02/18 14:39, Andre Przywara wrote: >>> + spin_lock_irqsave(&desc->lock, flags); >>> + if ( enable ) >>> + { >>> + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? >>> + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); >> >> Indentation and I would prefer a helper to convert between the vgic >> value and the IRQ_TYPE. This would make the code easier to read. >> >> Also, this code does not replicate correctly the current vGIC. >> gic_set_irq_type is only allowed to be used when >> irq_set_type_by_domain(d) returns true. If you consider this change >> valid, then I would like to know why. > > So what is/was the rationale for not allowing IRQ type changes for > non-privileged guests? If you allow to pass through an hardware IRQ to a > guest (which is the case this function handles), then I don't see why a > guest would not be allowed to change the configuration? It seems rather > odd, I guess it's up to the guest to know which type of IRQ this is? If you can answer the question on top of irq_type_set_by_domain (i.e "See whether it is possible to let any domain configure the type) then we can remove it. We decided to only allow for the hardware domain because we trust it. Cheers,
Hi, On 16/02/18 16:57, Julien Grall wrote: > Hi Andre, > > On 09/02/18 14:39, Andre Przywara wrote: >> As the enable register handlers are shared between the v2 and v3 >> emulation, their implementation goes into vgic-mmio.c, to be easily >> referenced from the v3 emulation as well later. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- >> xen/arch/arm/vgic/vgic-mmio.c | 114 >> +++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vgic/vgic-mmio.h | 11 ++++ >> 3 files changed, 127 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c >> b/xen/arch/arm/vgic/vgic-mmio-v2.c >> index 0926b3243e..eca6840ff9 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c >> @@ -74,10 +74,10 @@ static const struct vgic_register_region >> vgic_v2_dist_registers[] = { >> vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >> + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER, >> - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >> + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1, >> VGIC_ACCESS_32bit), >> REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR, >> vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c >> b/xen/arch/arm/vgic/vgic-mmio.c >> index 59703a6909..3d9fa02a10 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.c >> +++ b/xen/arch/arm/vgic/vgic-mmio.c >> @@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t >> addr, >> /* Ignore */ >> } >> +/* >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the >> value >> + * of the enabled bit, so there is only one function for both here. >> + */ >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len) > > Indentation. > >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > > uint32_t here please. > >> + u32 value = 0; > > Same here. > >> + 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->enabled ) >> + value |= (1U << i); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + } >> + >> + return value; >> +} >> + >> +static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type, > > Looking below irq_type should a enum vgic_irq_config and not an int. > >> + bool enable) > > Indentation. > >> +{ >> + unsigned long flags; >> + >> +// irq_set_affinity(desc, cpumask_of(v_target->processor)); > > Why is that commented? > >> + spin_lock_irqsave(&desc->lock, flags); >> + if ( enable ) >> + { >> + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? >> + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); > > Indentation and I would prefer a helper to convert between the vgic > value and the IRQ_TYPE. This would make the code easier to read. > > Also, this code does not replicate correctly the current vGIC. > gic_set_irq_type is only allowed to be used when > irq_set_type_by_domain(d) returns true. If you consider this change > valid, then I would like to know why. > >> + desc->handler->enable(desc); >> + } >> + else >> + desc->handler->disable(desc); >> + spin_unlock_irqrestore(&desc->lock, flags); >> +} >> + >> +void vgic_mmio_write_senable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) > > Indentation. > >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > > uint32_t. > >> + irq_desc_t *desc; >> + int i; >> + unsigned long flags; >> + enum vgic_irq_config config; >> + >> + for_each_set_bit( i, &val, len * 8 ) >> + { >> + struct vgic_irq *irq; >> + >> + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); >> + >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + irq->enabled = true; >> + if ( irq->hw ) >> + { >> + /* >> + * The irq cannot be a PPI, we only support delivery >> + * of SPIs to guests. >> + */ >> + ASSERT(irq->hwintid >= 32); >> + >> + desc = irq_to_desc(irq->hwintid); > > What is the rationale behind storing hwintid rather than the irq_desc > directly? Well, this is because KVM does it this way, for abstraction reasons, mostly. Looking over the users I see that mostly we are indeed after the struct irq_desc. But it would also increase struct vgic_irq by 4 bytes ;-) I could try to make to make the change, but am not fully convinced. What are your arguments for that change? Cheers, Andre. >> + config = irq->config; >> + } >> + else >> + desc = NULL; >> + vgic_queue_irq_unlock(vcpu->domain, irq, flags); >> + >> + vgic_put_irq(vcpu->domain, irq); >> + >> + if ( desc ) >> + vgic_handle_hardware_irq(desc, config, true); > > This is slightly strange. You handle the hardware IRQ outside the > virtual IRQ lock. It means that the hardware IRQ may end up enabled but > the virtual IRQ disabled. > >> + } >> +} >> + >> +void vgic_mmio_write_cenable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val) >> +{ >> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); >> + int i; >> + >> + for_each_set_bit( i, &val, len * 8 ) >> + { >> + struct vgic_irq *irq; >> + unsigned long flags; >> + irq_desc_t *desc; >> + >> + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + >> + irq->enabled = false; >> + >> + if ( irq->hw ) >> + desc = irq_to_desc(irq->hwintid); >> + else >> + desc = NULL; >> + >> + spin_unlock_irqrestore(&irq->irq_lock, flags); >> + vgic_put_irq(vcpu->domain, irq); >> + >> + if ( desc ) >> + vgic_handle_hardware_irq(desc, 0, false); > > Same remark here. > >> + } >> +} >> + >> 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 10ac682296..9f34bd1aec 100644 >> --- a/xen/arch/arm/vgic/vgic-mmio.h >> +++ b/xen/arch/arm/vgic/vgic-mmio.h >> @@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu, >> void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, >> unsigned int len, unsigned long val); >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len); > > Indentation. > >> + >> +void vgic_mmio_write_senable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val); > > Ditto. > >> + >> +void vgic_mmio_write_cenable(struct vcpu *vcpu, >> + paddr_t addr, unsigned int len, >> + unsigned long val); > > Ditto. > >> + >> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); >> /* Find the proper register handler entry given a certain address >> offset */ >> > > Cheers, >
Hi Andre, On 23/02/18 15:18, Andre Przywara wrote: >>> + irq_desc_t *desc; >>> + int i; >>> + unsigned long flags; >>> + enum vgic_irq_config config; >>> + >>> + for_each_set_bit( i, &val, len * 8 ) >>> + { >>> + struct vgic_irq *irq; >>> + >>> + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); >>> + >>> + spin_lock_irqsave(&irq->irq_lock, flags); >>> + irq->enabled = true; >>> + if ( irq->hw ) >>> + { >>> + /* >>> + * The irq cannot be a PPI, we only support delivery >>> + * of SPIs to guests. >>> + */ >>> + ASSERT(irq->hwintid >= 32); >>> + >>> + desc = irq_to_desc(irq->hwintid); >> >> What is the rationale behind storing hwintid rather than the irq_desc >> directly? > > Well, this is because KVM does it this way, for abstraction reasons, > mostly. Looking over the users I see that mostly we are indeed after the > struct irq_desc. But it would also increase struct vgic_irq by 4 bytes ;-) > > I could try to make to make the change, but am not fully convinced. > > What are your arguments for that change? To be honest, I don't have much arguments :). My main concern is using irq_to_desc wrongly when we will add support for routing PPI to domains. This would useful to support the virtual timer without the hack we currently have. In the PPI context, irq_to_desc would always return the PPI irq_desc of the current CPU. I am not entirely if this will always be ok for us. But I might be over cautious :). So I guess, we can keep it like that for now. Cheers,
Hi, On 19/02/18 14:13, Julien Grall wrote: > > > On 19/02/18 12:41, Andre Przywara wrote: >> Hi, > > Hi, > >> On 16/02/18 16:57, Julien Grall wrote: >>> On 09/02/18 14:39, Andre Przywara wrote: >>>> + spin_lock_irqsave(&desc->lock, flags); >>>> + if ( enable ) >>>> + { >>>> + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? >>>> + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); >>> >>> Indentation and I would prefer a helper to convert between the vgic >>> value and the IRQ_TYPE. This would make the code easier to read. >>> >>> Also, this code does not replicate correctly the current vGIC. >>> gic_set_irq_type is only allowed to be used when >>> irq_set_type_by_domain(d) returns true. If you consider this change >>> valid, then I would like to know why. >> >> So what is/was the rationale for not allowing IRQ type changes for >> non-privileged guests? If you allow to pass through an hardware IRQ to a >> guest (which is the case this function handles), then I don't see why a >> guest would not be allowed to change the configuration? It seems rather >> odd, I guess it's up to the guest to know which type of IRQ this is? > > If you can answer the question on top of irq_type_set_by_domain (i.e > "See whether it is possible to let any domain configure the type) then > we can remove it. We decided to only allow for the hardware domain > because we trust it. But why would you mistrust a DomU in this respect? The only point I see is that a guest has *some* influence on a hardware access, but I fail to see how a single MMIO read-modify-write sequence would actually impact the host. Especially since we do it only on enabling an IRQ. Looking more closely at the existing VGIC code we might want to check if the hardware IRQ was already enabled before entering the "if ( p->desc != NULL )" branch, btw. So is this the concern? Commit b0003bdd690 wasn't really enlightening in this respect. Cheers, Andre.
On 27/02/18 13:54, Andre Przywara wrote: > Hi, > > On 19/02/18 14:13, Julien Grall wrote: >> >> >> On 19/02/18 12:41, Andre Przywara wrote: >>> Hi, >> >> Hi, >> >>> On 16/02/18 16:57, Julien Grall wrote: >>>> On 09/02/18 14:39, Andre Przywara wrote: >>>>> + spin_lock_irqsave(&desc->lock, flags); >>>>> + if ( enable ) >>>>> + { >>>>> + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? >>>>> + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); >>>> >>>> Indentation and I would prefer a helper to convert between the vgic >>>> value and the IRQ_TYPE. This would make the code easier to read. >>>> >>>> Also, this code does not replicate correctly the current vGIC. >>>> gic_set_irq_type is only allowed to be used when >>>> irq_set_type_by_domain(d) returns true. If you consider this change >>>> valid, then I would like to know why. >>> >>> So what is/was the rationale for not allowing IRQ type changes for >>> non-privileged guests? If you allow to pass through an hardware IRQ to a >>> guest (which is the case this function handles), then I don't see why a >>> guest would not be allowed to change the configuration? It seems rather >>> odd, I guess it's up to the guest to know which type of IRQ this is? >> >> If you can answer the question on top of irq_type_set_by_domain (i.e >> "See whether it is possible to let any domain configure the type) then >> we can remove it. We decided to only allow for the hardware domain >> because we trust it. > > But why would you mistrust a DomU in this respect? > The only point I see is that a guest has *some* influence on a hardware > access, but I fail to see how a single MMIO read-modify-write sequence > would actually impact the host. Especially since we do it only on > enabling an IRQ. > Looking more closely at the existing VGIC code we might want to check if > the hardware IRQ was already enabled before entering the > "if ( p->desc != NULL )" branch, btw. That's not an issue here. You can only enter in vgic_enable_irqs if the virtual interrupt was previously disabled. Because the physical interrupt is routed to the guest, it will also be disabled at that time. > > So is this the concern? Commit b0003bdd690 wasn't really enlightening in > this respect. It was not really clear if it would be an issue when I wrote the patch. We trust the hardware domain so it is fine to let him configure the interrupt. For the guests, this will be taken from the DT (see gic_route_irq_to_guest). So this is likely to get configured correctly for the guest. What I was worry about is whether we need to sanitize the ICFGR when the interrupt is routed to another domain. But if you can clear that, then I guess it should be ok. However, I would prefer to do this in a separate patch and keep irq_type_set_by_domain around. This is to match the current vGIC and not changing too much Xen behavior. Cheers,
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c index 0926b3243e..eca6840ff9 100644 --- a/xen/arch/arm/vgic/vgic-mmio-v2.c +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c @@ -74,10 +74,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER, - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, + vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER, - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, + vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR, vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c index 59703a6909..3d9fa02a10 100644 --- a/xen/arch/arm/vgic/vgic-mmio.c +++ b/xen/arch/arm/vgic/vgic-mmio.c @@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, /* Ignore */ } +/* + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value + * of the enabled bit, so there is only one function for both here. + */ +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + u32 value = 0; + 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->enabled ) + value |= (1U << i); + + vgic_put_irq(vcpu->domain, irq); + } + + return value; +} + +static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type, + bool enable) +{ + unsigned long flags; + +// irq_set_affinity(desc, cpumask_of(v_target->processor)); + spin_lock_irqsave(&desc->lock, flags); + if ( enable ) + { + gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ? + IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING); + desc->handler->enable(desc); + } + else + desc->handler->disable(desc); + spin_unlock_irqrestore(&desc->lock, flags); +} + +void vgic_mmio_write_senable(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + irq_desc_t *desc; + int i; + unsigned long flags; + enum vgic_irq_config config; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq; + + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + spin_lock_irqsave(&irq->irq_lock, flags); + irq->enabled = true; + if ( irq->hw ) + { + /* + * The irq cannot be a PPI, we only support delivery + * of SPIs to guests. + */ + ASSERT(irq->hwintid >= 32); + + desc = irq_to_desc(irq->hwintid); + config = irq->config; + } + else + desc = NULL; + vgic_queue_irq_unlock(vcpu->domain, irq, flags); + + vgic_put_irq(vcpu->domain, irq); + + if ( desc ) + vgic_handle_hardware_irq(desc, config, true); + } +} + +void vgic_mmio_write_cenable(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + int i; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq; + unsigned long flags; + irq_desc_t *desc; + + irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + spin_lock_irqsave(&irq->irq_lock, flags); + + irq->enabled = false; + + if ( irq->hw ) + desc = irq_to_desc(irq->hwintid); + else + desc = NULL; + + spin_unlock_irqrestore(&irq->irq_lock, flags); + vgic_put_irq(vcpu->domain, irq); + + if ( desc ) + vgic_handle_hardware_irq(desc, 0, false); + } +} + 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 10ac682296..9f34bd1aec 100644 --- a/xen/arch/arm/vgic/vgic-mmio.h +++ b/xen/arch/arm/vgic/vgic-mmio.h @@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu, void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr, unsigned int len, unsigned long val); +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu, + paddr_t addr, unsigned int len); + +void vgic_mmio_write_senable(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val); + +void vgic_mmio_write_cenable(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val); + unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); /* Find the proper register handler entry given a certain address offset */
As the enable register handlers are shared between the v2 and v3 emulation, their implementation goes into vgic-mmio.c, to be easily referenced from the v3 emulation as well later. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- xen/arch/arm/vgic/vgic-mmio.c | 114 +++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vgic/vgic-mmio.h | 11 ++++ 3 files changed, 127 insertions(+), 2 deletions(-)