| Message ID | 1491434362-30310-27-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 6 Apr 2017, Andre Przywara wrote: > The INV command instructs the ITS to update the configuration data for > a given LPI by re-reading its entry from the property table. > We don't need to care so much about the priority value, but enabling > or disabling an LPI has some effect: We remove or push virtual LPIs > to their VCPUs, also check the virtual pending bit if an LPI gets enabled. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3-its.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index 47f2884..0d4b20d 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -376,6 +376,99 @@ static int its_handle_int(struct virt_its *its, uint64_t *cmdptr) > return 0; > } > > +/* > + * For a given virtual LPI read the enabled bit and priority from the virtual > + * property table and update the virtual IRQ's state in the given pending_irq. > + */ > +static int update_lpi_property(struct domain *d, uint32_t vlpi, > + struct pending_irq *p) > +{ > + paddr_t addr; > + uint8_t property; > + int ret; > + > + addr = d->arch.vgic.rdist_propbase & GENMASK_ULL(51, 12); > + > + ret = vgic_access_guest_memory(d, addr + vlpi - LPI_OFFSET, > + &property, sizeof(property), false); > + if ( ret ) > + return ret; > + > + p->lpi_priority = property & LPI_PROP_PRIO_MASK; > + if ( property & LPI_PROP_ENABLED ) > + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + else > + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + > + return 0; > +} > + > +/* > + * For a given virtual LPI read the enabled bit and priority from the virtual > + * property table and update the virtual IRQ's state. > + * This takes care of removing or pushing of virtual LPIs to their VCPUs. > + * Also check if this LPI is due to be injected and do it, if needed. > + */ > +static int update_lpi_enabled_status(struct domain *d, > + struct vcpu *vcpu, uint32_t vlpi) > +{ > + struct pending_irq *p = d->arch.vgic.handler->lpi_to_pending(d, vlpi); > + unsigned long flags; > + int ret; > + > + if ( !p ) > + return -EINVAL; > + > + spin_lock_irqsave(&vcpu->arch.vgic.lock, flags); > + ret = update_lpi_property(d, vlpi, p); > + if ( ret ) { > + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); > + return ret; > + } > + > + if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) > + { > + if ( !list_empty(&p->inflight) && > + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) > + gic_raise_guest_irq(vcpu, vlpi, p->lpi_priority); > + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); Something like this should work for LPIs too. > + /* Check whether the LPI has fired while the guest had it disabled. */ > + if ( test_and_clear_bit(GIC_IRQ_GUEST_LPI_PENDING, &p->status) ) > + vgic_vcpu_inject_irq(vcpu, vlpi); > + } > + else > + { > + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); > + > + gic_remove_from_queues(vcpu, vlpi); > + } > + > + return 0; > +} > + > +static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr) > +{ > + uint32_t devid = its_cmd_get_deviceid(cmdptr); > + uint32_t eventid = its_cmd_get_id(cmdptr); > + struct vcpu *vcpu; > + uint32_t vlpi; > + > + /* Translate the event into a vCPU/vLPI pair. */ > + if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) ) > + return -1; > + > + /* > + * Now read the property table and update our cached status. This > + * also takes care if this LPI now needs to be injected or removed. > + */ > + if ( update_lpi_enabled_status(its->d, vcpu, vlpi) ) > + return -1; > + > + return 0; > +} > + > static int its_handle_mapc(struct virt_its *its, uint64_t *cmdptr) > { > uint32_t collid = its_cmd_get_collection(cmdptr); > @@ -615,6 +708,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its) > case GITS_CMD_INT: > ret = its_handle_int(its, command); > break; > + case GITS_CMD_INV: > + ret = its_handle_inv(its, command); > + break; > case GITS_CMD_MAPC: > ret = its_handle_mapc(its, command); > break; > -- > 2.8.2 >
Hi Andre, On 06/04/17 00:19, Andre Przywara wrote: > The INV command instructs the ITS to update the configuration data for > a given LPI by re-reading its entry from the property table. > We don't need to care so much about the priority value, but enabling > or disabling an LPI has some effect: We remove or push virtual LPIs > to their VCPUs, also check the virtual pending bit if an LPI gets enabled. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3-its.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index 47f2884..0d4b20d 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -376,6 +376,99 @@ static int its_handle_int(struct virt_its *its, uint64_t *cmdptr) > return 0; > } > > +/* > + * For a given virtual LPI read the enabled bit and priority from the virtual > + * property table and update the virtual IRQ's state in the given pending_irq. > + */ > +static int update_lpi_property(struct domain *d, uint32_t vlpi, > + struct pending_irq *p) What is the locking expectation for this function? > +{ > + paddr_t addr; > + uint8_t property; > + int ret; > + > + addr = d->arch.vgic.rdist_propbase & GENMASK_ULL(51, 12); > + > + ret = vgic_access_guest_memory(d, addr + vlpi - LPI_OFFSET, > + &property, sizeof(property), false); > + if ( ret ) > + return ret; > + > + p->lpi_priority = property & LPI_PROP_PRIO_MASK; I don't think this change will be atomic. So what is preventing the lpi_priority to be read incorrectly? > + if ( property & LPI_PROP_ENABLED ) > + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + else > + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + > + return 0; > +} > + > +/* > + * For a given virtual LPI read the enabled bit and priority from the virtual > + * property table and update the virtual IRQ's state. > + * This takes care of removing or pushing of virtual LPIs to their VCPUs. > + * Also check if this LPI is due to be injected and do it, if needed. > + */ > +static int update_lpi_enabled_status(struct domain *d, > + struct vcpu *vcpu, uint32_t vlpi) The use of this function is a bit weird. You are already looking the radix when handling the command INVALL and you can easily get the pending_irq from the struct its_device for INV command. So you could avoid the lookup lpi_to_pending and the check ( !p ) which I think is confusing. > +{ > + struct pending_irq *p = d->arch.vgic.handler->lpi_to_pending(d, vlpi); > + unsigned long flags; > + int ret; > + > + if ( !p ) > + return -EINVAL; > + > + spin_lock_irqsave(&vcpu->arch.vgic.lock, flags); > + ret = update_lpi_property(d, vlpi, p); > + if ( ret ) { Coding style: if ( ... ) { > + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); > + return ret; > + } > + > + if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) > + { > + if ( !list_empty(&p->inflight) && > + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) > + gic_raise_guest_irq(vcpu, vlpi, p->lpi_priority); > + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); > + > + /* Check whether the LPI has fired while the guest had it disabled. */ > + if ( test_and_clear_bit(GIC_IRQ_GUEST_LPI_PENDING, &p->status) ) > + vgic_vcpu_inject_irq(vcpu, vlpi); > + } > + else > + { > + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); > + > + gic_remove_from_queues(vcpu, vlpi); > + } > + > + return 0; > +} > + > +static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr) > +{ > + uint32_t devid = its_cmd_get_deviceid(cmdptr); > + uint32_t eventid = its_cmd_get_id(cmdptr); > + struct vcpu *vcpu; > + uint32_t vlpi; > + > + /* Translate the event into a vCPU/vLPI pair. */ > + if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) ) > + return -1; > + > + /* > + * Now read the property table and update our cached status. This > + * also takes care if this LPI now needs to be injected or removed. > + */ > + if ( update_lpi_enabled_status(its->d, vcpu, vlpi) ) > + return -1; > + > + return 0; > +} > + > static int its_handle_mapc(struct virt_its *its, uint64_t *cmdptr) > { > uint32_t collid = its_cmd_get_collection(cmdptr); > @@ -615,6 +708,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its) > case GITS_CMD_INT: > ret = its_handle_int(its, command); > break; > + case GITS_CMD_INV: > + ret = its_handle_inv(its, command); > + break; > case GITS_CMD_MAPC: > ret = its_handle_mapc(its, command); > break; > Cheers,
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index 47f2884..0d4b20d 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -376,6 +376,99 @@ static int its_handle_int(struct virt_its *its, uint64_t *cmdptr) return 0; } +/* + * For a given virtual LPI read the enabled bit and priority from the virtual + * property table and update the virtual IRQ's state in the given pending_irq. + */ +static int update_lpi_property(struct domain *d, uint32_t vlpi, + struct pending_irq *p) +{ + paddr_t addr; + uint8_t property; + int ret; + + addr = d->arch.vgic.rdist_propbase & GENMASK_ULL(51, 12); + + ret = vgic_access_guest_memory(d, addr + vlpi - LPI_OFFSET, + &property, sizeof(property), false); + if ( ret ) + return ret; + + p->lpi_priority = property & LPI_PROP_PRIO_MASK; + if ( property & LPI_PROP_ENABLED ) + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); + else + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); + + return 0; +} + +/* + * For a given virtual LPI read the enabled bit and priority from the virtual + * property table and update the virtual IRQ's state. + * This takes care of removing or pushing of virtual LPIs to their VCPUs. + * Also check if this LPI is due to be injected and do it, if needed. + */ +static int update_lpi_enabled_status(struct domain *d, + struct vcpu *vcpu, uint32_t vlpi) +{ + struct pending_irq *p = d->arch.vgic.handler->lpi_to_pending(d, vlpi); + unsigned long flags; + int ret; + + if ( !p ) + return -EINVAL; + + spin_lock_irqsave(&vcpu->arch.vgic.lock, flags); + ret = update_lpi_property(d, vlpi, p); + if ( ret ) { + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); + return ret; + } + + if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) + { + if ( !list_empty(&p->inflight) && + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) + gic_raise_guest_irq(vcpu, vlpi, p->lpi_priority); + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); + + /* Check whether the LPI has fired while the guest had it disabled. */ + if ( test_and_clear_bit(GIC_IRQ_GUEST_LPI_PENDING, &p->status) ) + vgic_vcpu_inject_irq(vcpu, vlpi); + } + else + { + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); + spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags); + + gic_remove_from_queues(vcpu, vlpi); + } + + return 0; +} + +static int its_handle_inv(struct virt_its *its, uint64_t *cmdptr) +{ + uint32_t devid = its_cmd_get_deviceid(cmdptr); + uint32_t eventid = its_cmd_get_id(cmdptr); + struct vcpu *vcpu; + uint32_t vlpi; + + /* Translate the event into a vCPU/vLPI pair. */ + if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) ) + return -1; + + /* + * Now read the property table and update our cached status. This + * also takes care if this LPI now needs to be injected or removed. + */ + if ( update_lpi_enabled_status(its->d, vcpu, vlpi) ) + return -1; + + return 0; +} + static int its_handle_mapc(struct virt_its *its, uint64_t *cmdptr) { uint32_t collid = its_cmd_get_collection(cmdptr); @@ -615,6 +708,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its) case GITS_CMD_INT: ret = its_handle_int(its, command); break; + case GITS_CMD_INV: + ret = its_handle_inv(its, command); + break; case GITS_CMD_MAPC: ret = its_handle_mapc(its, command); break;
The INV command instructs the ITS to update the configuration data for a given LPI by re-reading its entry from the property table. We don't need to care so much about the priority value, but enabling or disabling an LPI has some effect: We remove or push virtual LPIs to their VCPUs, also check the virtual pending bit if an LPI gets enabled. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic-v3-its.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)