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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

Hi Julien,

Thank you for the feedback, we have a v3 agreement.

On Tue, Apr 24, 2018 at 6:12 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi Mirela,
>
>
> On 04/24/2018 12:02 PM, Mirela Simonovic wrote:
>>
>> 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;
>
>
> Either that or:
>
> if ( r == 0 )
>   goto write_ignore_32;
>
> So you don't need to rework the code too much. Both would probably want some
> comment in the code.
>
>>
>> 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?
>
>
> The way it is done in ICACTIVER is really fragile. The guest may think the
> active bit of the interrupt was cleared but this is not the case.
>
> It is not easy to check if the active bit is set in the current vGIC (should
> be better in the new vGIC). So it was decided to just ignore it to make
> Linux happy. The warning is here to tell the user that some may not work as
> expected.
>
> Regarding the ISACTIVER, you know that if the user write 0 none of the
> active state of the interrupts will be changed. So it is fine to avoid
> printing the warning. However, if there are one bit set then you really want
> to warn the user as the hypervisor will not probably handle it.
>
> So we want to keep the warning in both case.
>
> Cheers,
>
> --
> Julien Grall

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