[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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
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,
                  "%pv: vGICD: unhandled word write %#"PRIregister" to
                  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 )
             "%pv: vGICD: unhandled word write %#"PRIregister" to 
             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
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.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.