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

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

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@arm.com> 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@aggios.com>
>>>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> ---
>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel