Re: [PATCH v4 1/5] xen/arm: vgic-v2: Handle correctly byte write in ITARGETSR

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

Hi Ian,

On 22/10/15 16:53, Ian Campbell wrote:
> On Mon, 2015-10-12 at 15:22 +0100, Julien Grall wrote:
> 
> Subject: "correctly handle" and "writes"
> 
>> During a store, the byte is always in the low part of the register (i.e
>> [0:7]).
>>
>> Although, we are masking the register by using a shift of the
>> byte offset in the ITARGETSR. This will result to get a target list
>> equal to 0 which is ignored by the emulation.
> 
> I'm afraid I can't parse this.
> 
> I think instead of "Although" you might mean "incorrectly" as in "we are
> incorrectly...", but that would really then want the sentence to end
> "instead of <the right thing>". So perhaps:
> 
>     We are incorrectly masking the register by using a shift of the byte
>     offset in the ITARGETSR instead of <...something...>. This will result
>     in a target list equal to 0 which is ignored by the emulation.

Rather than "instead of..." what about "while the byte is always in r[0:7]"?

> (note also s/to get/in a/ in the second sentence)
> 
>> Because of that a guest won't be able to modify the any ITARGETSR using
>> byte access. Note that the first byte of each register will still be
>> writeable.
> 
> "Because of that the guest will only be able to modify the first byte in
> each ITARGETSR"
> 
> In your version the "any ITARGETSR" in the first sentence is immediately
> contradicted by the second sentence with gives an example of an ITARGETSR
> which it can modify.

Right, I will update the commit message.

>>
>> Furthermore, the body of the loop is retrieving the old target list
>> using the index of the byte.
>>
>> To avoid modifying too much the loop, shift the byte stored to the correct
>> offset.
> 
> That might have meant a smaller patch, but it's a lot harder to understand
> either the result or the diff.

The size of the patch would have been the same. Although, it requires to
modify the call to vgic_byte_read in the loop to access the correct
interrupt.

I didn't try to spend to much time to modify the loop because the
follow-up patch (#2) will rewrite the loop.

[...]

>>  xen/arch/arm/vgic-v2.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 2d63e12..665afeb 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -346,11 +346,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>>          /* 8-bit vcpu mask for this domain */
>>          BUG_ON(v->domain->max_vcpus > 8);
>>          target = (1 << v->domain->max_vcpus) - 1;
>> -        if ( dabt.size == 2 )
>> -            target = target | (target << 8) | (target << 16) | (target << 24);
>> +        target = target | (target << 8) | (target << 16) | (target << 24);
>> +        if ( dabt.size == DABT_WORD )
>> +            target &= r;
>>          else
>> -            target = (target << (8 * (gicd_reg & 0x3)));
>> -        target &= r;
>> +            target &= (r << (8 * (gicd_reg & 0x3)));
> 
> At this point do you not now have 3 bytes of
>     (1 << v->domain->max_vcpus) - 1;
> and 1 byte of that masked with the write?
> 
> IOW isn't this modifying the 3 bytes which aren't written?

No, the current loop search for bit set to 1. As the target variable
will only contain one byte with some bit set to 1, only the IRQ
associated to this byte will be updated.

Regards,

-- 
Julien Grall