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

[Julien Grall <julien.grall@arm.com>]

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