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(®, 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@lists.xen.org
https://lists.xen.org/xen-devel
