Re: [PATCH v2] xen/arm: implement GICD_I[S/C]ACTIVER reads

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 2 Apr 2020, Julien Grall wrote:
> As we discussed on Tuesday, the cost for other vCPUs is only going to be a
> trap to the hypervisor and then back again. The cost is likely smaller than
> receiving and forwarding an interrupt.
> 
> You actually agreed on this analysis. So can you enlighten me as to why
> receiving an interrupt is a not problem for latency but this is?

My answer was that the difference is that an operating system can
disable interrupts, but it cannot disable receiving this special IPI.

 
> The crash only happened when using vGICv3 not vGICv2. But did you look at Xen
> recently? Particularly at the following patch:
> 
> xen/arm: Handle unimplemented VGICv3 registers as RAZ/WI
> 
> Per the ARM Generic Interrupt Controller Architecture Specification (ARM
> IHI 0069E), reserved registers should generally be treated as RAZ/WI.
> To simplify the VGICv3 design and improve guest compatibility, treat the
> default case for GICD and GICR registers as read_as_zero/write_ignore.
> 
> Signed-off-by: Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>
> Acked-by: Julien Grall <julien@xxxxxxx>
> 
> I actually pointed the patch to you during one of our weekly calls. Yet we
> agreed it would still be good to implement the register properly and you said
> you will write a patch.

As you know I cannot reproduce the crash myself, I asked Peng and Wei
for help in that. I cannot be certain Jeff's patch makes a difference,
but looking at the code, if you open
xen/arch/arm/vgic-v3.c:__vgic_v3_distr_common_mmio_read you can see that
the range mistake is still there:


    /* Read the active status of an IRQ via GICD/GICR is not supported */
    case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVER):
    case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
        goto read_as_zero;


So a GICD_ISACTIVER of any register but the first should end up hitting
the default case:

    default:
        printk(XENLOG_G_ERR
               "%pv: %s: unhandled read r%d offset %#08x\n",
               v, name, dabt.reg, reg);
        return 0;
    }

Which returns 0 (IO_ABORT).

Would you be happy to have the range fixed to be:

    case VRANGE32(GICD_ISACTIVER, GICD_ISACTIVERN):

instead?