[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH 13/24] ARM: vITS: handle CLEAR command
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@xxxxxxx>
---
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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
