| Message ID | 20240422073708.3663529-5-jens.wiklander@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | FF-A notifications | expand |
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. > > 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@linaro.org> > --- > 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_priority(desc, priority); > } > > @@ -375,7 +376,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) > /* Reading IRQ will ACK it */ > irq = gic_hw_ops->read_irq(); > > - if ( likely(irq >= 16 && irq < 1020) ) > + if ( likely(irq >= GIC_SGI_MAX && irq < 1020) ) This check is now rather confusing as one could think that do_IRQ() would still not be reached for dynamic SGI. I think it would be clearer GIC_SGI_MAX needs to be renamed to GIC_SGI_STATIC_MAX and do_sgi() to do_static_sgi(). > { > 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. > > - if ( irq < 32 ) > + if ( irq < NR_GIC_SGI ) > + perfc_incr(ipis); > + else if ( irq < NR_GIC_LOCAL_IRQS ) > perfc_incr(ppis); > else > perfc_incr(spis); Cheers,
Hi Julien, On Mon, Apr 22, 2024 at 12:57 PM Julien Grall <julien@xen.org> 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. > > > > > 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@linaro.org> > > --- > > 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? > > > gic_set_irq_priority(desc, priority); > > } > > > > @@ -375,7 +376,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) > > /* Reading IRQ will ACK it */ > > irq = gic_hw_ops->read_irq(); > > > > - if ( likely(irq >= 16 && irq < 1020) ) > > + if ( likely(irq >= GIC_SGI_MAX && irq < 1020) ) > > This check is now rather confusing as one could think that do_IRQ() > would still not be reached for dynamic SGI. I think it would be clearer > GIC_SGI_MAX needs to be renamed to GIC_SGI_STATIC_MAX and do_sgi() to > do_static_sgi(). Thanks, I'll update. > > > { > > 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 ( irq < 32 ) > > + if ( irq < NR_GIC_SGI ) > > + perfc_incr(ipis); > > + else if ( irq < NR_GIC_LOCAL_IRQS ) > > perfc_incr(ppis); > > else > > perfc_incr(spis); > Thanks, Jens
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@xen.org> 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@linaro.org> >>> --- >>> 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. [...] >> >>> { >>> 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. Cheers,
Hi Julien, > On 23 Apr 2024, at 13:05, Julien Grall <julien@xen.org> 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@xen.org> 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. The idea is that the only SGI reserved for secure are used by the secure world (in fact it is the SPMC in the secure world who tells us which SGI it will generate). So in theory that means it will always use an SGI between 8 and 15. Now it could make sense in fact to check that the number returned by the firmware (or SPMC) is not clashing with Xen as it is a recommendation in the spec and in fact an implementation might do something different. Right now there is no spec that will say that it will never clash with the one used by Xen as the FF-A spec is not enforcing anything here so it would be a good idea to check and disable FF-A with a proper error message if this happens. Cheers Bertrand > >>> >>>> >>>> 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@linaro.org> >>>> --- >>>> 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. > > [...] > >>> >>>> { >>>> 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. > > Cheers, > > -- > Julien Grall
Hi Julien, > On 23 Apr 2024, at 14:37, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Julien, > >> On 23 Apr 2024, at 13:05, Julien Grall <julien@xen.org> 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@xen.org> 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. > > The idea is that the only SGI reserved for secure are used by the secure world (in fact it is the SPMC in the secure world who tells us which SGI it will generate). > So in theory that means it will always use an SGI between 8 and 15. > > Now it could make sense in fact to check that the number returned by the firmware (or SPMC) is not clashing with Xen as it is a recommendation in the spec and > in fact an implementation might do something different. > > Right now there is no spec that will say that it will never clash with the one used by Xen as the FF-A spec is not enforcing anything here so it would be a good idea > to check and disable FF-A with a proper error message if this happens. After some more digging here is what is recommended by Arm in the Arm Base System Architecture v1.0C [1]: "The system shall implement at least eight Non-secure SGIs, assigned to interrupt IDs 0-7." So basically as long as Xen is using SGIs 0-7 it is safe as those shall never be used by the secure world. Now i do agree that we should check that whatever is returned by the firmware is not conflicting with what is used by Xen. Cheers Bertrand [1] https://developer.arm.com/documentation/den0094/
Hi Bertrand, On 23/04/2024 14:23, Bertrand Marquis wrote: > Hi Julien, > >> On 23 Apr 2024, at 14:37, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: >> >> Hi Julien, >> >>> On 23 Apr 2024, at 13:05, Julien Grall <julien@xen.org> 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@xen.org> 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. >> >> The idea is that the only SGI reserved for secure are used by the secure world (in fact it is the SPMC in the secure world who tells us which SGI it will generate). >> So in theory that means it will always use an SGI between 8 and 15. >> >> Now it could make sense in fact to check that the number returned by the firmware (or SPMC) is not clashing with Xen as it is a recommendation in the spec and >> in fact an implementation might do something different. >> >> Right now there is no spec that will say that it will never clash with the one used by Xen as the FF-A spec is not enforcing anything here so it would be a good idea >> to check and disable FF-A with a proper error message if this happens. > > > After some more digging here is what is recommended by Arm in the Arm Base System Architecture v1.0C [1]: > > "The system shall implement at least eight Non-secure SGIs, assigned to interrupt IDs 0-7." Thanks! Can we provide a link to the specification in the commit message? > > So basically as long as Xen is using SGIs 0-7 it is safe as those shall never be used by the secure world. > Now i do agree that we should check that whatever is returned by the firmware is not conflicting with what > is used by Xen. +1. Cheers,
Hi, On Tue, Apr 23, 2024 at 4:28 PM Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 23/04/2024 14:23, Bertrand Marquis wrote: > > Hi Julien, > > > >> On 23 Apr 2024, at 14:37, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > >> > >> Hi Julien, > >> > >>> On 23 Apr 2024, at 13:05, Julien Grall <julien@xen.org> 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@xen.org> 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. > >> > >> The idea is that the only SGI reserved for secure are used by the secure world (in fact it is the SPMC in the secure world who tells us which SGI it will generate). > >> So in theory that means it will always use an SGI between 8 and 15. > >> > >> Now it could make sense in fact to check that the number returned by the firmware (or SPMC) is not clashing with Xen as it is a recommendation in the spec and > >> in fact an implementation might do something different. > >> > >> Right now there is no spec that will say that it will never clash with the one used by Xen as the FF-A spec is not enforcing anything here so it would be a good idea > >> to check and disable FF-A with a proper error message if this happens. > > > > > > After some more digging here is what is recommended by Arm in the Arm Base System Architecture v1.0C [1]: > > > > "The system shall implement at least eight Non-secure SGIs, assigned to interrupt IDs 0-7." > > Thanks! Can we provide a link to the specification in the commit message? Sure, I'll add a link. > > > > > So basically as long as Xen is using SGIs 0-7 it is safe as those shall never be used by the secure world. > > Now i do agree that we should check that whatever is returned by the firmware is not conflicting with what > > is used by Xen. > +1. That makes sense, I'll add a check. Thanks, Jens
On Tue, Apr 23, 2024 at 1:05 PM Julien Grall <julien@xen.org> 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@xen.org> 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@linaro.org> > >>> --- > >>> 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
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); gic_set_irq_priority(desc, priority); } @@ -375,7 +376,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) /* Reading IRQ will ACK it */ irq = gic_hw_ops->read_irq(); - if ( likely(irq >= 16 && irq < 1020) ) + if ( likely(irq >= GIC_SGI_MAX && irq < 1020) ) { 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); - if ( irq < 32 ) + if ( irq < NR_GIC_SGI ) + perfc_incr(ipis); + else if ( irq < NR_GIC_LOCAL_IRQS ) perfc_incr(ppis); else perfc_incr(spis);
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. 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@linaro.org> --- v1->v2 - Update patch description as requested --- xen/arch/arm/gic.c | 5 +++-- xen/arch/arm/irq.c | 7 +++++-- 2 files changed, 8 insertions(+), 4 deletions(-)