Hi Eric,

On 06/04/16 10:36, Eric Auger wrote:

Hi Andre,
On 03/26/2016 03:14 AM, Andre Przywara wrote:

quoted

Add emulation for some basic MMIO registers used in the ITS emulation.

...

quoted

+
+/*
+ * By writing to CWRITER the guest announces new commands to be processed.
+ * Since we cannot read from guest memory inside the ITS spinlock, we
+ * iterate over the command buffer (with the lock dropped) until the read
+ * pointer matches the write pointer. Other VCPUs writing this register in the
+ * meantime will just update the write pointer, leaving the command
+ * processing to the first instance of the function.
+ */
+static int vgic_mmio_write_its_cwriter(struct kvm_vcpu *vcpu,
+				       struct kvm_io_device *this,
+				       gpa_t addr, int len, const void *val)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	struct vgic_its *its = &dist->its;
+	gpa_t cbaser = its_cmd_buffer_base(vcpu->kvm);
+	u64 cmd_buf[4];
+	u32 reg;
+	bool finished;
+
+	reg = mask64(its->cwriter & 0xfffe0, addr & 7, len, val);
+	reg &= 0xfffe0;
+	if (reg > its_cmd_buffer_size(vcpu->kvm))
+		return 0;
+
+	spin_lock(&its->lock);
+
+	/*
+	 * If there is still another VCPU handling commands, let this
+	 * one pick up the new CWRITER and process "our" new commands as well.
+	 */
+	finished = (its->cwriter != its->creadr);

empty?

Maybe that is indeed less misleading ...

quoted

+	its->cwriter = reg;
+
+	spin_unlock(&its->lock);

Assuming we have 2 vcpus attempting a cwriter write at the same moment I
don't understand what does guarantee they are not going to execute the
following loop concurrently and possibly execute vits_handle_command
twice on the same cmd from the same its->creadr start?

So does my explanation of the algorithm in that other mail today explain
it? Or do you still see a hole in the locking scheme here?

quoted

+
+	while (!finished) {
+		int ret = kvm_read_guest(vcpu->kvm, cbaser + its->creadr,
+					 cmd_buf, 32);
+		if (ret) {
+			/*
+			 * Gah, we are screwed. Reset CWRITER to that command
+			 * that we have finished processing and return.
+			 */
+			spin_lock(&its->lock);
+			its->cwriter = its->creadr;

don't get why do you set the queue empty?

So from a guest's point of view this is something like an ITS internal
error which shouldn't happen. So I just bail out, but leave the queue in
a consistent (read: empty) state to allow possible future commands to be
processed at best effort.
I guess this is the best we can come up with, since there is no feasible
way of communicating errors back(?)

Cheers,
Andre.