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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 03/04/2020 20:41, Stefano Stabellini wrote:
> On Thu, 2 Apr 2020, Julien Grall wrote:
> 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?
I don't particularly like it, but it is not going to make any difference 
for Linux < 5.4. So I am not opposed to it.
However, I am a bit worry the vGIC is still going to be a pile of hack.
So I think we should continue the discussion about making it better. 
This includes how to implement I{C, S}ACTIVER properly and what sort of 
use-cases we want to support.
Cheers,

--
Julien Grall