[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v8 15/27] ARM: vITS: handle CLEAR command
On 12/04/17 15:29, Andre Przywara wrote:
Hi,
On 12/04/17 15:10, Julien Grall wrote:
Hi Andre,
On 12/04/17 01:44, 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.
As read_itte() is now eventually used, we add the static keyword.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
xen/arch/arm/vgic-v3-its.c | 52
++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 632ab84..e24ab60 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -227,8 +227,8 @@ static bool read_itte_locked(struct virt_its *its,
uint32_t devid,
* This function takes care of the locking by taking the its_lock
itself, so
* a caller shall not hold this. Before returning, the lock is
dropped again.
*/
-bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
- struct vcpu **vcpu_ptr, uint32_t *vlpi_ptr)
+static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t
evid,
+ struct vcpu **vcpu_ptr, uint32_t *vlpi_ptr)
{
bool ret;
@@ -297,6 +297,51 @@ static uint64_t its_cmd_mask_field(uint64_t
*its_cmd, unsigned int word,
#define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2,
63, 1)
#define its_cmd_get_ittaddr(cmd) (its_cmd_mask_field(cmd, 2,
8, 44) << 8)
+/*
+ * CLEAR removes the pending state from an LPI. */
+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 *p;
+ struct vcpu *vcpu;
+ uint32_t vlpi;
+ unsigned long flags;
+
+ /* Translate the DevID/EvID pair into a vCPU/vLPI pair. */
+ if ( !read_itte(its, devid, eventid, &vcpu, &vlpi) )
So the vCPU ID is retrieved from guest memory, therefore you cannot
trust the value for taking a lock.
Indeed, a malicious guest could rewrite the value with another vCPU ID
and you would take the wrong vCPU vGIC lock.
What is the plan to fix that?
To (write-)protect the tables. I will mention that in the cover letter.
+ return -1;
+
+ spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
I don't think this lock will protect correctly the pending_irq because
if you do a MOVI before hand you will use the new vCPU ID and not the
old one (see my explanation on patch #2).
+
+ p = its->d->arch.vgic.handler->lpi_to_pending(its->d, vlpi);
+ if ( !p )
Please detail what means NULL here.
+ {
+ spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
+ return -1;
+ }
+
+ /*
+ * If the LPI is already visible on the guest, it is too late to
+ * clear the pending state. However this is a benign race that can
+ * happen on real hardware, too: If the LPI has already been
forwarded
+ * to a CPU interface, a CLEAR request reaching the redistributor
has
+ * no effect on that LPI anymore. Since LPIs are edge triggered and
+ * have no active state, we don't need to care about this here.
+ */
+ if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+ {
+ /* Remove a pending, but not yet injected guest IRQ. */
+ clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+ list_del_init(&p->inflight);
+ list_del_init(&p->lr_queue);
I am not sure to understand why you open-code gic_remove_from_queues
rather than reworking it.
Well, actually all that gic_remove_from_queues does is:
list_del_init(&p->lr_queue);
under the VGIC lock with getting the pending_irq first.
I have the struct pending_irq already, also the lock. So there is no
point in calling that function (which would block anyway).
And since I wanted to keep changes to the existing code to a minimum, I
decided to just not touch this function, instead just put that *one*
line in here, next to the other list_del_init().
Does that sound sensible?
I am sorry but it does not. If one day someone decides to update
gic_remove_from_queues, he will have to remember that we open-coded in
MOVI because you didn't want to touch common code.
Common code is not set in stone, the goal is to abstract all the issues
to make easier to propagate change. I am always in favor of reworking
the common code.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
