diff mbox series

[1/1] iommu/riscv: Support sharing irq lines between iommu queues

Message ID 20240701072908.25503-2-luxu.kernel@bytedance.com (mailing list archive)
State Handled Elsewhere
Headers show
Series iommu/riscv: Support sharing irq lines between iommu queues | expand

Checks

Context Check Description
conchuod/vmtest-fixes-PR fail merge-conflict

Commit Message

Xu Lu July 1, 2024, 7:29 a.m. UTC 
When the number of wired interrupt lines is less than the number of
iommu queues, we should assign one irq line for several queues and
configure csr icvec accordingly.

Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
---
 drivers/iommu/riscv/iommu-platform.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Tomasz Jeznach July 1, 2024, 8:47 a.m. UTC | #1 
On Mon, Jul 1, 2024 at 9:29 AM 'Xu Lu' via linux <linux@rivosinc.com> wrote:
>
> When the number of wired interrupt lines is less than the number of
> iommu queues, we should assign one irq line for several queues and
> configure csr icvec accordingly.
>

Is this case not already covered by the code in function
riscv_iommu_init_check(), assigning cause to vector register (ICVEC)
based on available interrupt vectors? Interrupt vector selection is
based on accepted ICVEC configuration (see riscv_iommu_queue_vec()).

Relevant code (patch 'Command and fault queue support'):
iommu->icvec = FIELD_PREP(RISCV_IOMMU_ICVEC_FIV, 1 % iommu->irqs_count) |
       FIELD_PREP(RISCV_IOMMU_ICVEC_PIV, 2 % iommu->irqs_count) |
       FIELD_PREP(RISCV_IOMMU_ICVEC_PMIV, 3 % iommu->irqs_count);

Values in iommu->irqs[] should represent actual interrupts allocated
for the device.

Best,
- Tomasz

> Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
> ---
>  drivers/iommu/riscv/iommu-platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
> index da336863f152..1d0af1260d5b 100644
> --- a/drivers/iommu/riscv/iommu-platform.c
> +++ b/drivers/iommu/riscv/iommu-platform.c
> @@ -60,6 +60,10 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
>         for (vec = 0; vec < iommu->irqs_count; vec++)
>                 iommu->irqs[vec] = platform_get_irq(pdev, vec);
>
> +       for (vec = iommu->irqs_count; vec < RISCV_IOMMU_INTR_COUNT; vec++)
> +               iommu->irqs[vec] = platform_get_irq(pdev,
> +                                                   (vec % iommu->irqs_count));
> +
>         /* Enable wire-signaled interrupts, fctl.WSI */
>         if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
>                 iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> --
> 2.20.1
>
> --
> You received this message because you are subscribed to the Google Groups "linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux+unsubscribe@rivosinc.com.
> To view this discussion on the web visit https://groups.google.com/a/rivosinc.com/d/msgid/linux/20240701072908.25503-2-luxu.kernel%40bytedance.com.
> For more options, visit https://groups.google.com/a/rivosinc.com/d/optout.
Xu Lu July 1, 2024, 9:10 a.m. UTC | #2 
Sorry for not describing it clearly.

Indeed, the hardware interrupt lines have be configured in
riscv_iommu_init_check().

What I mean is that virtual interrupts of irqs[iommu->irqs_count] to
irqs[RISCV_IOMMU_INTR_COUNT - 1] are not set, which will cause
riscv_iommu_queue_enable() fail:
       const unsigned int irq =
iommu->irqs[riscv_iommu_queue_vec(iommu, queue->qid)];
       rc = request_threaded_irq(irq, xxx);
       if (rc) {
                 queue->iommu = NULL;
                 return rc;
       }

The virtual irq of queue id 'q' should be the same as irqs[q %
iommu->irqs_count] as ICVEC is configured in the same way.

Regards,

Xu Lu

On Mon, Jul 1, 2024 at 4:47 PM Tomasz Jeznach <tjeznach@rivosinc.com> wrote:
>
> On Mon, Jul 1, 2024 at 9:29 AM 'Xu Lu' via linux <linux@rivosinc.com> wrote:
> >
> > When the number of wired interrupt lines is less than the number of
> > iommu queues, we should assign one irq line for several queues and
> > configure csr icvec accordingly.
> >
>
> Is this case not already covered by the code in function
> riscv_iommu_init_check(), assigning cause to vector register (ICVEC)
> based on available interrupt vectors? Interrupt vector selection is
> based on accepted ICVEC configuration (see riscv_iommu_queue_vec()).
>
> Relevant code (patch 'Command and fault queue support'):
> iommu->icvec = FIELD_PREP(RISCV_IOMMU_ICVEC_FIV, 1 % iommu->irqs_count) |
>        FIELD_PREP(RISCV_IOMMU_ICVEC_PIV, 2 % iommu->irqs_count) |
>        FIELD_PREP(RISCV_IOMMU_ICVEC_PMIV, 3 % iommu->irqs_count);
>
> Values in iommu->irqs[] should represent actual interrupts allocated
> for the device.
>
> Best,
> - Tomasz
>
> > Signed-off-by: Xu Lu <luxu.kernel@bytedance.com>
> > ---
> >  drivers/iommu/riscv/iommu-platform.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
> > index da336863f152..1d0af1260d5b 100644
> > --- a/drivers/iommu/riscv/iommu-platform.c
> > +++ b/drivers/iommu/riscv/iommu-platform.c
> > @@ -60,6 +60,10 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> >         for (vec = 0; vec < iommu->irqs_count; vec++)
> >                 iommu->irqs[vec] = platform_get_irq(pdev, vec);
> >
> > +       for (vec = iommu->irqs_count; vec < RISCV_IOMMU_INTR_COUNT; vec++)
> > +               iommu->irqs[vec] = platform_get_irq(pdev,
> > +                                                   (vec % iommu->irqs_count));
> > +
> >         /* Enable wire-signaled interrupts, fctl.WSI */
> >         if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> >                 iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> > --
> > 2.20.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "linux" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to linux+unsubscribe@rivosinc.com.
> > To view this discussion on the web visit https://groups.google.com/a/rivosinc.com/d/msgid/linux/20240701072908.25503-2-luxu.kernel%40bytedance.com.
> > For more options, visit https://groups.google.com/a/rivosinc.com/d/optout.
Xu Lu July 1, 2024, 9:14 a.m. UTC | #3 
Oh, I think I understand what you mean now after checking the
implementation of riscv_iommu_queue_vec(). I apologize for not
understanding the meaning of the code before, please ignore my
disturbance.

Best Regards,

Xu Lu
Tomasz Jeznach July 1, 2024, 9:25 a.m. UTC | #4 
On Mon, Jul 1, 2024 at 11:14 AM Xu Lu <luxu.kernel@bytedance.com> wrote:
>
> Oh, I think I understand what you mean now after checking the
> implementation of riscv_iommu_queue_vec(). I apologize for not
> understanding the meaning of the code before, please ignore my
> disturbance.
>
> Best Regards,
>
> Xu Lu

No worries, thanks for checking that path. LMK if there are any other
problems with the driver.

Best,
- Tomasz
diff mbox series

Patch

diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
index da336863f152..1d0af1260d5b 100644
--- a/drivers/iommu/riscv/iommu-platform.c
+++ b/drivers/iommu/riscv/iommu-platform.c
@@ -60,6 +60,10 @@  static int riscv_iommu_platform_probe(struct platform_device *pdev)
 	for (vec = 0; vec < iommu->irqs_count; vec++)
 		iommu->irqs[vec] = platform_get_irq(pdev, vec);
 
+	for (vec = iommu->irqs_count; vec < RISCV_IOMMU_INTR_COUNT; vec++)
+		iommu->irqs[vec] = platform_get_irq(pdev,
+						    (vec % iommu->irqs_count));
+
 	/* Enable wire-signaled interrupts, fctl.WSI */
 	if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
 		iommu->fctl |= RISCV_IOMMU_FCTL_WSI;