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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 23 May 2017, Julien Grall wrote:
> 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.

All right, thanks


> [...]
> 
> > > +    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