On Mon, 27 Sep 2021 18:50:40 +0200
Cédric Le Goater <c...@kaod.org> wrote:
> On 9/24/21 19:13, Greg Kurz wrote:
> > On Fri, 24 Sep 2021 16:58:00 +0200
> > Cédric Le Goater <c...@kaod.org> wrote:
> >
> >> [ ... ]
> >>
> >>>> The changes only impact KVM support since we are deferring the IRQ
> >>>> initialization at the KVM level. What we have to be careful about is not
> >>>> accessing an ESB page of an interrupt that would not have been
> >>>> initiliazed
> >>>> in the KVM device, else QEMU gets a sigbus.
> >>>>
> >>>
> >>> Ok, so this is just needed on a path that leads to xive_esb_rw() or
> >>> kvmppc_xive_esb_trigger(), right ?
> >>>
> >>> It seems that
> >>>
> >>> h_int_esb()
> >>> kvmppc_xive_esb_rw()
> >>>
> >>> can get there with an lisn provided by the guest, and I don't see any
> >>> such check on the way : h_int_esb() only checks xive_eas_is_valid().
> >>
> >> This call is for LSI interrupts (device only) and not vCPU IPIs. see hcall
> >> h_int_get_source_info(). I agree a hcall fuzzer could reach it.
> >>
> >
> > Yes we tell the guest to use H_INT_ESB for LSIs only but I don't have
> > the impression that it is forbidden for the guest to call H_INT_ESB
> > for arbitrary interrupts.
> >
> >> An alternative solution would be to claim the vCPU IPIs on demand, like
> >> we do for the MSIs, and not in spapr_irq_init(). It's a much bigger change
> >> tough, because the H_INT hcalls consider that the interrupt numbers have
> >> been claimed.
> >>
> >
> > Maybe it would be simpler to call xive_source_is_initialized() instead of
> > xive_eas_is_valid() in cases like this, e.g. hcall code since it is shared
> > between emulation and KVM ?
>
> Yes but we need a better check than :
>
> if (lisn < SPAPR_XIRQ_BASE) {
> return !!xive_get_field64(EAS_END_INDEX, xive->eat[lisn].w);
> }
>
> This is more an heuristic than a precise test on the "validity" of
> a source at the KVM level.
>
I guess we should be able to get kvmppc_xive_irq_state::valid from
KVM by making the KVM_DEV_XIVE_GRP_SOURCE attribute readable. Would
that be enough ?
>
> Thanks,
>
> C.
