Re: [Xen-devel] [PATCH v2 02/10] xen/arm: Ignore write to GICD_ISACTIVERn registers (vgic-v2)

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

On Mon, Apr 23, 2018 at 1:15 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Mirela,
>
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> Guests attempt to write into these registers on resume (for example
>> Linux).
>> Without this patch a data abort exception will be raised to the guest.
>> This patch handles the write access by ignoring it, but only if the value
>> to be written is zero. This should be fine because reading these registers
>> is already handled as 'read as zero'.
>>
>> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>>
>> ---
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> Changes in v2:
>> - Write should be ignored only if the value to be written is zero
>>   (in v1 the write was ignored regardless of the value)
>> ---
>>   xen/arch/arm/vgic-v2.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 646d1f3d12..afd3e89883 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -488,7 +488,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
>> mmio_info_t *info,
>>           printk(XENLOG_G_ERR
>>                  "%pv: vGICD: unhandled word write %#"PRIregister" to
>> ISACTIVER%d\n",
>>                  v, r, gicd_reg - GICD_ISACTIVER);
>> -        return 0;
>> +        if ( r != 0 )
>> +            return 0;
>
>
> It would be better to move the check before the printk. So a warning is
> avoided when the guest is writing 0.
>

If we want to avoid printing a warning when the guest is writing 0
then the printk needs to be moved within the check. I guess this is
what you meant:
        if ( r != 0 )
        {
            printk(XENLOG_G_ERR
            "%pv: vGICD: unhandled word write %#"PRIregister" to ISACTIVER%d\n",
            v, r, gicd_reg - GICD_ISACTIVER);
            return 0;
        }
        goto write_ignore_32;

Please note that in the original patch where I ignored the write
regardless of the value I just followed how it is already done for
GICD_ICACTIVER.
For existing GICD_ICACTIVER case there is no check for the value to be
written and there is a warning printed.

Not checking the value seems fine to me but why is then a warning
printed? Should we suppress that print as well?


> Cheers,
>
>> +        goto write_ignore_32;
>>         case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>>           printk(XENLOG_G_ERR
>>
>
> Cheers,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel