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

[Julien Grall <julien.grall@xxxxxxx>]

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