| Message ID | 20160928182457.12433-14-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 28/09/16 19:24, Andre Przywara wrote: > This introduces the ITS command handler for the CLEAR command, which > clears the pending state of an LPI. > This removes a not-yet injected, but already queued IRQ from a VCPU. > > In addition this patch introduces the lookup function which translates > a given DeviceID/EventID pair into a pointer to our vITTE structure. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-its.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 115 insertions(+) > > diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c > index 875b992..99d9e9c 100644 > --- a/xen/arch/arm/vgic-its.c > +++ b/xen/arch/arm/vgic-its.c > @@ -61,6 +61,73 @@ struct vits_itte > uint64_t collection:16; > }; > > +#define UNMAPPED_COLLECTION ((uint16_t)~0) > + > +/* Must be called with the ITS lock held. */ This comment is a call to have an ASSERT in the function. > +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid) Please use unsigned int. > +{ > + uint16_t vcpu_id; > + > + if ( collid >= its->max_collections ) > + return NULL; > + > + vcpu_id = its->coll_table[collid]; > + if ( vcpu_id == UNMAPPED_COLLECTION || vcpu_id >= its->d->max_vcpus ) > + return NULL; > + > + return its->d->vcpu[vcpu_id]; > +} > + > +#define DEV_TABLE_ITT_ADDR(x) ((x) & GENMASK(51, 8)) > +#define DEV_TABLE_ITT_SIZE(x) (BIT(((x) & GENMASK(7, 0)) + 1)) The layout of dev_table[...] really needs to be explained. It took me quite a while to understand how it works. For instance why you skip the first 8 bits for the address... > +#define DEV_TABLE_ENTRY(addr, bits) \ > + (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(7, 0))) > + > +static paddr_t get_itte_address(struct virt_its *its, > + uint32_t devid, uint32_t evid) > +{ > + paddr_t addr; > + > + if ( devid >= its->max_devices ) > + return ~0; Please use INVALID_PADDR here. > + > + if ( evid >= DEV_TABLE_ITT_SIZE(its->dev_table[devid]) ) > + return ~0; Ditto. > + > + addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]); > + > + return addr + evid * sizeof(struct vits_itte); > +} > + > +/* Looks up a given deviceID/eventID pair on an ITS and returns a pointer to Coding style: /* * Foo > + * the corresponding ITTE. This maps the respective guest page into Xen. > + * Once finished with handling the ITTE, call put_devid_evid() to unmap > + * the page again. > + * Must be called with the ITS lock held. This is a call for an ASSERT in the code. > + */ > +static struct vits_itte *get_devid_evid(struct virt_its *its, > + uint32_t devid, uint32_t evid) The naming of the function is confusing. It doesn't look up a device ID/event ID but an IIT. So I would rename it to find_itte. > +{ > + paddr_t addr = get_itte_address(its, devid, evid); > + struct vits_itte *itte; > + > + if (addr == ~0) Coding style: if ( ... ) And return INVALID_PADDR. > + return NULL; > + > + /* TODO: check locking for map_guest_pages() */ > + itte = map_guest_pages(its->d, addr & PAGE_MASK, 1); > + if ( !itte ) > + return NULL; > + > + return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte); > +} > + > +/* Must be called with the ITS lock held. */ > +static void put_devid_evid(struct virt_its *its, struct vits_itte *itte) > +{ > + unmap_guest_pages(itte, 1); > +} > + > /************************************** > * Functions that handle ITS commands * > **************************************/ > @@ -80,6 +147,51 @@ static uint64_t its_cmd_mask_field(uint64_t *its_cmd, > #define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32) > #define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1) > > +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr) > +{ > + uint32_t devid = its_cmd_get_deviceid(cmdptr); > + uint32_t eventid = its_cmd_get_id(cmdptr); > + struct pending_irq *pirq; > + struct vits_itte *itte; > + struct vcpu *vcpu; > + uint32_t vlpi; > + > + spin_lock(&its->its_lock); > + > + itte = get_devid_evid(its, devid, eventid); > + if ( !itte ) > + { > + spin_unlock(&its->its_lock); > + return -1; > + } > + > + vcpu = get_vcpu_from_collection(its, itte->collection); > + if ( !vcpu ) > + { > + spin_unlock(&its->its_lock); > + return -1; > + } > + > + vlpi = itte->vlpi; > + > + put_devid_evid(its, itte); > + spin_unlock(&its->its_lock); > + > + /* Remove a pending, but not yet injected guest IRQ. */ > + pirq = lpi_to_pending(vcpu, vlpi, false); > + if ( pirq ) > + { > + clear_bit(GIC_IRQ_GUEST_QUEUED, &pirq->status); > + gic_remove_from_queues(vcpu, vlpi); > + > + /* Mark this pending IRQ struct as availabe again. */ NIT: s/availabe/available/ > + if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) ) > + pirq->irq = 0; This code should be in a separate helper. It will be helpful to make the structure available again easily without open coding it. > + } > + > + return 0; > +} > + > #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) > > static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, > @@ -100,6 +212,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, > cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); > switch (its_cmd_get_command(cmdptr)) > { > + case GITS_CMD_CLEAR: > + its_handle_clear(its, cmdptr); Should not you check the return for its_handle_clear? > + break; > case GITS_CMD_SYNC: > /* We handle ITS commands synchronously, so we ignore SYNC. */ > break; > Regards,
On Wed, 28 Sep 2016, Andre Przywara wrote: > This introduces the ITS command handler for the CLEAR command, which > clears the pending state of an LPI. > This removes a not-yet injected, but already queued IRQ from a VCPU. > > In addition this patch introduces the lookup function which translates > a given DeviceID/EventID pair into a pointer to our vITTE structure. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-its.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 115 insertions(+) > > diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c > index 875b992..99d9e9c 100644 > --- a/xen/arch/arm/vgic-its.c > +++ b/xen/arch/arm/vgic-its.c > @@ -61,6 +61,73 @@ struct vits_itte > uint64_t collection:16; > }; > > +#define UNMAPPED_COLLECTION ((uint16_t)~0) > + > +/* Must be called with the ITS lock held. */ > +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid) > +{ > + uint16_t vcpu_id; > + > + if ( collid >= its->max_collections ) > + return NULL; > + > + vcpu_id = its->coll_table[collid]; > + if ( vcpu_id == UNMAPPED_COLLECTION || vcpu_id >= its->d->max_vcpus ) > + return NULL; > + > + return its->d->vcpu[vcpu_id]; > +} > + > +#define DEV_TABLE_ITT_ADDR(x) ((x) & GENMASK(51, 8)) > +#define DEV_TABLE_ITT_SIZE(x) (BIT(((x) & GENMASK(7, 0)) + 1)) > +#define DEV_TABLE_ENTRY(addr, bits) \ > + (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(7, 0))) > + > +static paddr_t get_itte_address(struct virt_its *its, > + uint32_t devid, uint32_t evid) > +{ > + paddr_t addr; > + > + if ( devid >= its->max_devices ) > + return ~0; Please #define the error > + if ( evid >= DEV_TABLE_ITT_SIZE(its->dev_table[devid]) ) > + return ~0; same here > + addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]); > + > + return addr + evid * sizeof(struct vits_itte); > +} > + > +/* Looks up a given deviceID/eventID pair on an ITS and returns a pointer to > + * the corresponding ITTE. This maps the respective guest page into Xen. > + * Once finished with handling the ITTE, call put_devid_evid() to unmap > + * the page again. > + * Must be called with the ITS lock held. > + */ > +static struct vits_itte *get_devid_evid(struct virt_its *its, > + uint32_t devid, uint32_t evid) > +{ > + paddr_t addr = get_itte_address(its, devid, evid); > + struct vits_itte *itte; > + > + if (addr == ~0) > + return NULL; > + > + /* TODO: check locking for map_guest_pages() */ > + itte = map_guest_pages(its->d, addr & PAGE_MASK, 1); > + if ( !itte ) > + return NULL; No need to use the vmap to map 1 page > + return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte); Please use () around the div operation for clarity > +} > + > +/* Must be called with the ITS lock held. */ > +static void put_devid_evid(struct virt_its *its, struct vits_itte *itte) > +{ > + unmap_guest_pages(itte, 1); No need for this, once you use __pa instead of the vmap > +} > + > /************************************** > * Functions that handle ITS commands * > **************************************/ > @@ -80,6 +147,51 @@ static uint64_t its_cmd_mask_field(uint64_t *its_cmd, > #define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32) > #define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1) > > +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr) > +{ > + uint32_t devid = its_cmd_get_deviceid(cmdptr); > + uint32_t eventid = its_cmd_get_id(cmdptr); > + struct pending_irq *pirq; > + struct vits_itte *itte; > + struct vcpu *vcpu; > + uint32_t vlpi; > + > + spin_lock(&its->its_lock); > + > + itte = get_devid_evid(its, devid, eventid); > + if ( !itte ) > + { > + spin_unlock(&its->its_lock); > + return -1; > + } > + > + vcpu = get_vcpu_from_collection(its, itte->collection); > + if ( !vcpu ) > + { > + spin_unlock(&its->its_lock); > + return -1; > + } > + > + vlpi = itte->vlpi; > + > + put_devid_evid(its, itte); > + spin_unlock(&its->its_lock); > + > + /* Remove a pending, but not yet injected guest IRQ. */ We need to check that the vlpi hasn't already been added to an LR register. We can do that with GIC_IRQ_GUEST_VISIBLE. In case GIC_IRQ_GUEST_VISIBLE is set, we need to clear the lr (clear_lr). If we don't handle this case, we should at least print a warning. > + pirq = lpi_to_pending(vcpu, vlpi, false); > + if ( pirq ) > + { > + clear_bit(GIC_IRQ_GUEST_QUEUED, &pirq->status); > + gic_remove_from_queues(vcpu, vlpi); > + > + /* Mark this pending IRQ struct as availabe again. */ > + if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) ) > + pirq->irq = 0; > + } > + > + return 0; > +} > + > #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) > > static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, > @@ -100,6 +212,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, > cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); > switch (its_cmd_get_command(cmdptr)) > { > + case GITS_CMD_CLEAR: > + its_handle_clear(its, cmdptr); > + break; > case GITS_CMD_SYNC: > /* We handle ITS commands synchronously, so we ignore SYNC. */ > break; > -- > 2.9.0 >
Hi, On 09/11/16 00:39, Stefano Stabellini wrote: > On Wed, 28 Sep 2016, Andre Przywara wrote: >> This introduces the ITS command handler for the CLEAR command, which >> clears the pending state of an LPI. >> This removes a not-yet injected, but already queued IRQ from a VCPU. >> >> In addition this patch introduces the lookup function which translates >> a given DeviceID/EventID pair into a pointer to our vITTE structure. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic-its.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 115 insertions(+) >> >> diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c >> index 875b992..99d9e9c 100644 >> --- a/xen/arch/arm/vgic-its.c >> +++ b/xen/arch/arm/vgic-its.c >> @@ -61,6 +61,73 @@ struct vits_itte >> uint64_t collection:16; >> }; >> >> +#define UNMAPPED_COLLECTION ((uint16_t)~0) >> + >> +/* Must be called with the ITS lock held. */ >> +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid) >> +{ >> + uint16_t vcpu_id; >> + >> + if ( collid >= its->max_collections ) >> + return NULL; >> + >> + vcpu_id = its->coll_table[collid]; >> + if ( vcpu_id == UNMAPPED_COLLECTION || vcpu_id >= its->d->max_vcpus ) >> + return NULL; >> + >> + return its->d->vcpu[vcpu_id]; >> +} >> + >> +#define DEV_TABLE_ITT_ADDR(x) ((x) & GENMASK(51, 8)) >> +#define DEV_TABLE_ITT_SIZE(x) (BIT(((x) & GENMASK(7, 0)) + 1)) >> +#define DEV_TABLE_ENTRY(addr, bits) \ >> + (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(7, 0))) >> + >> +static paddr_t get_itte_address(struct virt_its *its, >> + uint32_t devid, uint32_t evid) >> +{ >> + paddr_t addr; >> + >> + if ( devid >= its->max_devices ) >> + return ~0; > > Please #define the error Technically this should be INVALID_PADDR here. > >> + if ( evid >= DEV_TABLE_ITT_SIZE(its->dev_table[devid]) ) >> + return ~0; > > same here Ditto. > > >> + addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]); >> + >> + return addr + evid * sizeof(struct vits_itte); >> +} >> + >> +/* Looks up a given deviceID/eventID pair on an ITS and returns a pointer to >> + * the corresponding ITTE. This maps the respective guest page into Xen. >> + * Once finished with handling the ITTE, call put_devid_evid() to unmap >> + * the page again. >> + * Must be called with the ITS lock held. >> + */ >> +static struct vits_itte *get_devid_evid(struct virt_its *its, >> + uint32_t devid, uint32_t evid) >> +{ >> + paddr_t addr = get_itte_address(its, devid, evid); >> + struct vits_itte *itte; >> + >> + if (addr == ~0) >> + return NULL; >> + >> + /* TODO: check locking for map_guest_pages() */ >> + itte = map_guest_pages(its->d, addr & PAGE_MASK, 1); >> + if ( !itte ) >> + return NULL; > > No need to use the vmap to map 1 page But you do have to translate the IPA to a PA, so you cannot directly use __pa on it. > >> + return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte); > > Please use () around the div operation for clarity > > >> +} >> + >> +/* Must be called with the ITS lock held. */ >> +static void put_devid_evid(struct virt_its *its, struct vits_itte *itte) >> +{ >> + unmap_guest_pages(itte, 1); > > No need for this, once you use __pa instead of the vmap Well, we should at least use map_domain_page/unmap_domain_page even if they are a nop on ARM64. And not directly __pa. Regards,
diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c index 875b992..99d9e9c 100644 --- a/xen/arch/arm/vgic-its.c +++ b/xen/arch/arm/vgic-its.c @@ -61,6 +61,73 @@ struct vits_itte uint64_t collection:16; }; +#define UNMAPPED_COLLECTION ((uint16_t)~0) + +/* Must be called with the ITS lock held. */ +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid) +{ + uint16_t vcpu_id; + + if ( collid >= its->max_collections ) + return NULL; + + vcpu_id = its->coll_table[collid]; + if ( vcpu_id == UNMAPPED_COLLECTION || vcpu_id >= its->d->max_vcpus ) + return NULL; + + return its->d->vcpu[vcpu_id]; +} + +#define DEV_TABLE_ITT_ADDR(x) ((x) & GENMASK(51, 8)) +#define DEV_TABLE_ITT_SIZE(x) (BIT(((x) & GENMASK(7, 0)) + 1)) +#define DEV_TABLE_ENTRY(addr, bits) \ + (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(7, 0))) + +static paddr_t get_itte_address(struct virt_its *its, + uint32_t devid, uint32_t evid) +{ + paddr_t addr; + + if ( devid >= its->max_devices ) + return ~0; + + if ( evid >= DEV_TABLE_ITT_SIZE(its->dev_table[devid]) ) + return ~0; + + addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]); + + return addr + evid * sizeof(struct vits_itte); +} + +/* Looks up a given deviceID/eventID pair on an ITS and returns a pointer to + * the corresponding ITTE. This maps the respective guest page into Xen. + * Once finished with handling the ITTE, call put_devid_evid() to unmap + * the page again. + * Must be called with the ITS lock held. + */ +static struct vits_itte *get_devid_evid(struct virt_its *its, + uint32_t devid, uint32_t evid) +{ + paddr_t addr = get_itte_address(its, devid, evid); + struct vits_itte *itte; + + if (addr == ~0) + return NULL; + + /* TODO: check locking for map_guest_pages() */ + itte = map_guest_pages(its->d, addr & PAGE_MASK, 1); + if ( !itte ) + return NULL; + + return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte); +} + +/* Must be called with the ITS lock held. */ +static void put_devid_evid(struct virt_its *its, struct vits_itte *itte) +{ + unmap_guest_pages(itte, 1); +} + /************************************** * Functions that handle ITS commands * **************************************/ @@ -80,6 +147,51 @@ static uint64_t its_cmd_mask_field(uint64_t *its_cmd, #define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32) #define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1) +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr) +{ + uint32_t devid = its_cmd_get_deviceid(cmdptr); + uint32_t eventid = its_cmd_get_id(cmdptr); + struct pending_irq *pirq; + struct vits_itte *itte; + struct vcpu *vcpu; + uint32_t vlpi; + + spin_lock(&its->its_lock); + + itte = get_devid_evid(its, devid, eventid); + if ( !itte ) + { + spin_unlock(&its->its_lock); + return -1; + } + + vcpu = get_vcpu_from_collection(its, itte->collection); + if ( !vcpu ) + { + spin_unlock(&its->its_lock); + return -1; + } + + vlpi = itte->vlpi; + + put_devid_evid(its, itte); + spin_unlock(&its->its_lock); + + /* Remove a pending, but not yet injected guest IRQ. */ + pirq = lpi_to_pending(vcpu, vlpi, false); + if ( pirq ) + { + clear_bit(GIC_IRQ_GUEST_QUEUED, &pirq->status); + gic_remove_from_queues(vcpu, vlpi); + + /* Mark this pending IRQ struct as availabe again. */ + if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &pirq->status) ) + pirq->irq = 0; + } + + return 0; +} + #define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12) static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, @@ -100,6 +212,9 @@ static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its, cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf)); switch (its_cmd_get_command(cmdptr)) { + case GITS_CMD_CLEAR: + its_handle_clear(its, cmdptr); + break; case GITS_CMD_SYNC: /* We handle ITS commands synchronously, so we ignore SYNC. */ break;
This introduces the ITS command handler for the CLEAR command, which clears the pending state of an LPI. This removes a not-yet injected, but already queued IRQ from a VCPU. In addition this patch introduces the lookup function which translates a given DeviceID/EventID pair into a pointer to our vITTE structure. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic-its.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+)