[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v9 13/28] ARM: vITS: add command handling stub and MMIO emulation



Hi Stefano,

On 22/05/17 23:32, Stefano Stabellini wrote:
On Thu, 11 May 2017, Andre Przywara wrote:
+    case VREG64(GITS_CWRITER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        reg = its->cwriter;
+        *r = vgic_reg64_extract(reg, info);

Why is this not protected by a lock? Also from the comment above I
cannot tell if it should be protected by its_lock or by vcmd_lock.

Because if you take the vcmd_lock, the vCPU will spin until we finish to handle the command queue. This means a guest can potentially block multiple pCPUs for a long time.

In this case, cwriter can be read atomically as it was updated by the guest itself ...


+        break;
+    case VREG64(GITS_CREADR):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        reg = its->creadr;
+        *r = vgic_reg64_extract(reg, info);
+        break;

Same here

For here, the command queue handler will write to creader atomically every a command is been executed. Making this lockless also allow a domain keep track where we are on the command queue handling.

This is something we already discussed quite a few times. So we should probably have a comment in the code to avoid this question to come up again.

[...]

+    case VREG64(GITS_CWRITER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        spin_lock(&its->vcmd_lock);
+        reg = ITS_CMD_OFFSET(its->cwriter);
+        vgic_reg64_update(&reg, r, info);
+        its->cwriter = ITS_CMD_OFFSET(reg);
+
+        if ( its->enabled )
+            if ( vgic_its_handle_cmds(d, its) )
+                gdprintk(XENLOG_WARNING, "error handling ITS commands\n");
+
+        spin_unlock(&its->vcmd_lock);

OK, so it looks like the reads should be protected by vcmd_lock

See my comment above.



+        return 1;
+
+    case VREG64(GITS_CREADR):

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.