Re: [XEN PATCH v2 4/5] xen/arm: allow dynamically assigned SGI handlers

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

On Tue, Apr 23, 2024 at 1:05 PM Julien Grall <julien@xxxxxxx> wrote:
>
>
>
> On 23/04/2024 10:35, Jens Wiklander wrote:
> > Hi Julien,
>
> Hi Jens,
>
> > On Mon, Apr 22, 2024 at 12:57 PM Julien Grall <julien@xxxxxxx> wrote:
> >>
> >> Hi Jens,
> >>
> >> On 22/04/2024 08:37, Jens Wiklander wrote:
> >>> Updates so request_irq() can be used with a dynamically assigned SGI irq
> >>> as input. This prepares for a later patch where an FF-A schedule
> >>> receiver interrupt handler is installed for an SGI generated by the
> >>> secure world.
> >>
> >> I would like to understand the use-case a bit more. Who is responsible
> >> to decide the SGI number? Is it Xen or the firmware?
> >>
> >> If the later, how can we ever guarantee the ID is not going to clash
> >> with what the OS/hypervisor is using? Is it described in a
> >> specification? If so, please give a pointer.
> >
> > The firmware decides the SGI number. Given that the firmware doesn't
> > know which SGIs Xen is using it typically needs to donate one of the
> > secure SGIs, but that is transparent to Xen.
>
> Right this is my concern. The firmware decides the number, but at the
> same time Xen thinks that all the SGIs are available (AFAIK there is
> only one set).
>
> What I would like to see is some wording from a spec indicating that the
> SGIs ID reserved by the firmware will not be clashing with the one used
> by Xen.
>
> >
> >
> >>
> >>>
> >>> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they are
> >>> always edge triggered.
> >>>
> >>> gic_interrupt() is updated to route the dynamically assigned SGIs to
> >>> do_IRQ() instead of do_sgi(). The latter still handles the statically
> >>> assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION.
> >>>
> >>> Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> >>> ---
> >>> v1->v2
> >>> - Update patch description as requested
> >>> ---
> >>>    xen/arch/arm/gic.c | 5 +++--
> >>>    xen/arch/arm/irq.c | 7 +++++--
> >>
> >> I am not sure where to write the comment. But I think the comment on top
> >> of irq_set_affinity() in setup_irq() should also be updated.
> >>
> >>>    2 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>> index 44c40e86defe..e9aeb7138455 100644
> >>> --- a/xen/arch/arm/gic.c
> >>> +++ b/xen/arch/arm/gic.c
> >>> @@ -117,7 +117,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, 
> >>> unsigned int priority)
> >>>
> >>>        desc->handler = gic_hw_ops->gic_host_irq_type;
> >>>
> >>> -    gic_set_irq_type(desc, desc->arch.type);
> >>> +    if ( desc->irq >= NR_GIC_SGI)
> >>> +        gic_set_irq_type(desc, desc->arch.type);
> >>
> >> So above, you say that the SGIs are always edge-triggered interrupt. So
> >> I assume desc->arch.type. So are you skipping the call because it is
> >> unnessary or it could do the wrong thing?
> >>
> >> Ideally, the outcome of the answer be part of the comment on top of the
> >> check.
> >
> > gic_set_irq_type() has an assert "ASSERT(type != IRQ_TYPE_INVALID)"
> > which is triggered without this check.
> > So it's both unnecessary and wrong. I suppose we could update the
> > bookkeeping of all SGIs to be edge-triggered instead of
> > IRQ_TYPE_INVALID. It would still be unnecessary though. What do you
> > suggest?
>
> I would rather prefer if we update the book-keeping for all the SGIs.

I'll update the code.

>
> [...]
>
> >>
> >>>            {
> >>>                isb();
> >>>                do_IRQ(regs, irq, is_fiq);
> >>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> >>> index bcce80a4d624..fdb214560978 100644
> >>> --- a/xen/arch/arm/irq.c
> >>> +++ b/xen/arch/arm/irq.c
> >>> @@ -224,9 +224,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int 
> >>> irq, int is_fiq)
> >>>
> >>>        perfc_incr(irqs);
> >>>
> >>> -    ASSERT(irq >= 16); /* SGIs do not come down this path */
> >>> +    /* Statically assigned SGIs do not come down this path */
> >>> +    ASSERT(irq >= GIC_SGI_MAX);
> >>
> >>
> >> With this change, I think the path with vgic_inject_irq() now needs to
> >> gain an ASSERT(irq >= NR_GIC_SGI) because the path is not supposed to be
> >> taken for SGIs.
> >
> > I'm sorry, I don't see the connection. If I add
> > ASSERT(virq >= NR_GIC_SGI);
> > at the top of vgic_inject_irq() it will panic when injecting a
> > Schedule Receiver or Notification Pending Interrupt for a guest.
>
> If you look at do_IRQ(), we have the following code:
>
>      if ( test_bit(_IRQ_GUEST, &desc->status) )
>      {
>          struct irq_guest *info = irq_get_guest_info(desc);
>
>          perfc_incr(guest_irqs);
>          desc->handler->end(desc);
>
>          set_bit(_IRQ_INPROGRESS, &desc->status);
>
>          /*
>           * The irq cannot be a PPI, we only support delivery of SPIs to
>           * guests.
>           */
>          vgic_inject_irq(info->d, NULL, info->virq, true);
>          goto out_no_end;
>      }
>
> What I suggesting is to add an ASSERT(irq >= NR_GIC_SGI) just before the
> call because now do_IRQ() can be called with SGIs yet we don't allow HW
> SGIs to assigned to a guest.

Got it, thanks for the explanation. I'll add the assert.

Thanks,
Jens