From: Pierre Gondois <Pierre.Gondois@arm.com>

In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts
can be defined following 2 models.
In the first model, a _SRS object must be described to modify the PCI
interrupt. The _CRS and _PRS object allows to describe the PCI
interrupt (level/edge triggered, active high/low).
In the second model, the PCI interrupt cannot be described with a
similar granularity. PCI interrupts are by default level triggered,
active low.

GicV2 SPI interrupts are level or edge triggered, active high. To
correctly describe PCI interrupts, the first model is used, even though
Arm Base Boot Requirements v1.0 requires to use the second mode.

Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---

Notes:
    v3:
     - New patch. [Pierre]

 .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 47 +++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index 3e22587d4a25..6823a98795c8 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -1,7 +1,7 @@
 /** @file
   SSDT Pcie Table Generator.
 
-  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+  Copyright (c) 2021 - 2022, Arm Limited. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -295,6 +295,10 @@ GeneratePciDeviceInfo (
     Method (_CRS, 0) {
       Return CRS0
       })
+    Method (_PRS, 0) {
+      Return CRS0
+      })
+    Method (_SRS, 1) { }
     Method (_DIS) { }
   }
 
@@ -302,9 +306,12 @@ GeneratePciDeviceInfo (
   PCI Firmware Specification - Revision 3.3,
   s3.5. "Device State at Firmware/Operating System Handoff"
 
-  The _PRS and _SRS are not supported, cf Arm Base Boot Requirements v1.0:
+  Warning: The Arm Base Boot Requirements v1.0 states the following:
   "The _PRS (Possible Resource Settings) and _SRS (Set Resource Settings)
   are not supported."
+  However, the first model to describe PCI legacy interrupts is used (cf. ACPI
+  6.4 s6.2.13) as PCI defaults (Level Triggered, Active Low) are not compatible
+  with GICv2 (must be Active High).
 
   @param [in]       Irq         Interrupt controller interrupt.
   @param [in]       IrqFlags    Interrupt flags.
@@ -416,7 +423,41 @@ GenerateLinkDevice (
     return Status;
   }
 
-  // ASL:Method (_DIS, 1) {}
+  // ASL:
+  // Method (_PRS, 0) {
+  //   Return (CRS0)
+  // }
+  Status = AmlCodeGenMethodRetNameString (
+             "_PRS",
+             "CRS0",
+             0,
+             FALSE,
+             0,
+             LinkNode,
+             NULL
+             );
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  // ASL:Method (_SRS, 1) {}
+  // Not possible to disable interrupts.
+  Status = AmlCodeGenMethodRetNameString (
+             "_SRS",
+             NULL,
+             1,
+             FALSE,
+             0,
+             LinkNode,
+             NULL
+             );
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  // ASL:Method (_DIS, 0) {}
   // Not possible to disable interrupts.
   Status = AmlCodeGenMethodRetNameString (
              "_DIS",
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86170): https://edk2.groups.io/g/devel/message/86170
Mute This Topic: https://groups.io/mt/88746970/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

(+ Marc)

On Fri, 28 Jan 2022 at 16:41, <Pierre.Gondois@arm.com> wrote:
>
> From: Pierre Gondois <Pierre.Gondois@arm.com>
>
> In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts
> can be defined following 2 models.
> In the first model, a _SRS object must be described to modify the PCI
> interrupt. The _CRS and _PRS object allows to describe the PCI
> interrupt (level/edge triggered, active high/low).
> In the second model, the PCI interrupt cannot be described with a
> similar granularity. PCI interrupts are by default level triggered,
> active low.
>
> GicV2 SPI interrupts are level or edge triggered, active high. To
> correctly describe PCI interrupts, the first model is used, even though
> Arm Base Boot Requirements v1.0 requires to use the second mode.
>

There are two different issues here:

- using separate 'interrupt link' device objects with an Interrupt()
resource rather than a simple GSIV number
- whether _PRS and _SRS need to be implemented on those link objects.

The latter is simply not true - _PRS and _SRS are optional, and
pointless if there is only a single possible value, so there is really
no need to add them here.

As for the choice between the link object or the GSIV number: I don't
think INTx interrupts on ARM systems are actually level low, and the
GSIV option is widely used, also in platforms that exist in
edk2-platforms, without any reported issues.

I've cc'ed Marc, perhaps he can shed some light on this, but I'd
prefer to stick to the GSIV approach if we can, as it is much simpler.



> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>
> Notes:
>     v3:
>      - New patch. [Pierre]
>
>  .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c    | 47 +++++++++++++++++--
>  1 file changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index 3e22587d4a25..6823a98795c8 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -1,7 +1,7 @@
>  /** @file
>    SSDT Pcie Table Generator.
>
> -  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
> +  Copyright (c) 2021 - 2022, Arm Limited. All rights reserved.<BR>
>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -295,6 +295,10 @@ GeneratePciDeviceInfo (
>      Method (_CRS, 0) {
>        Return CRS0
>        })
> +    Method (_PRS, 0) {
> +      Return CRS0
> +      })
> +    Method (_SRS, 1) { }
>      Method (_DIS) { }
>    }
>
> @@ -302,9 +306,12 @@ GeneratePciDeviceInfo (
>    PCI Firmware Specification - Revision 3.3,
>    s3.5. "Device State at Firmware/Operating System Handoff"
>
> -  The _PRS and _SRS are not supported, cf Arm Base Boot Requirements v1.0:
> +  Warning: The Arm Base Boot Requirements v1.0 states the following:
>    "The _PRS (Possible Resource Settings) and _SRS (Set Resource Settings)
>    are not supported."
> +  However, the first model to describe PCI legacy interrupts is used (cf. ACPI
> +  6.4 s6.2.13) as PCI defaults (Level Triggered, Active Low) are not compatible
> +  with GICv2 (must be Active High).
>
>    @param [in]       Irq         Interrupt controller interrupt.
>    @param [in]       IrqFlags    Interrupt flags.
> @@ -416,7 +423,41 @@ GenerateLinkDevice (
>      return Status;
>    }
>
> -  // ASL:Method (_DIS, 1) {}
> +  // ASL:
> +  // Method (_PRS, 0) {
> +  //   Return (CRS0)
> +  // }
> +  Status = AmlCodeGenMethodRetNameString (
> +             "_PRS",
> +             "CRS0",
> +             0,
> +             FALSE,
> +             0,
> +             LinkNode,
> +             NULL
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  // ASL:Method (_SRS, 1) {}
> +  // Not possible to disable interrupts.
> +  Status = AmlCodeGenMethodRetNameString (
> +             "_SRS",
> +             NULL,
> +             1,
> +             FALSE,
> +             0,
> +             LinkNode,
> +             NULL
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  // ASL:Method (_DIS, 0) {}
>    // Not possible to disable interrupts.
>    Status = AmlCodeGenMethodRetNameString (
>               "_DIS",
> --
> 2.25.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86209): https://edk2.groups.io/g/devel/message/86209
Mute This Topic: https://groups.io/mt/88746970/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Sat, 29 Jan 2022 15:52:02 +0000,
Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> (+ Marc)
> 
> On Fri, 28 Jan 2022 at 16:41, <Pierre.Gondois@arm.com> wrote:
> >
> > From: Pierre Gondois <Pierre.Gondois@arm.com>
> >
> > In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts
> > can be defined following 2 models.
> > In the first model, a _SRS object must be described to modify the PCI
> > interrupt. The _CRS and _PRS object allows to describe the PCI
> > interrupt (level/edge triggered, active high/low).
> > In the second model, the PCI interrupt cannot be described with a
> > similar granularity. PCI interrupts are by default level triggered,
> > active low.
> >
> > GicV2 SPI interrupts are level or edge triggered, active high. To
> > correctly describe PCI interrupts, the first model is used, even though
> > Arm Base Boot Requirements v1.0 requires to use the second mode.
> >
> 
> There are two different issues here:
> 
> - using separate 'interrupt link' device objects with an Interrupt()
> resource rather than a simple GSIV number
> - whether _PRS and _SRS need to be implemented on those link objects.
> 
> The latter is simply not true - _PRS and _SRS are optional, and
> pointless if there is only a single possible value, so there is really
> no need to add them here.
> 
> As for the choice between the link object or the GSIV number: I don't
> think INTx interrupts on ARM systems are actually level low, and the
> GSIV option is widely used, also in platforms that exist in
> edk2-platforms, without any reported issues.
> 
> I've cc'ed Marc, perhaps he can shed some light on this, but I'd
> prefer to stick to the GSIV approach if we can, as it is much simpler.

I don't immediately see the point either. Yes, the GIC only supports
level-high interrupts. However, all the PCIe implementations connected
to it are inverting the level. If they don't, that's even simpler (the
HW is broken).

Is this to address an apparent disconnect with the spec?

[...]

> > -  The _PRS and _SRS are not supported, cf Arm Base Boot Requirements v1.0:
> > +  Warning: The Arm Base Boot Requirements v1.0 states the following:
> >    "The _PRS (Possible Resource Settings) and _SRS (Set Resource Settings)
> >    are not supported."
> > +  However, the first model to describe PCI legacy interrupts is used (cf. ACPI
> > +  6.4 s6.2.13) as PCI defaults (Level Triggered, Active Low) are not compatible
> > +  with GICv2 (must be Active High).

nit: GICv3 has the exact same behaviour. Why is v2 singled out?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86224): https://edk2.groups.io/g/devel/message/86224
Mute This Topic: https://groups.io/mt/88746970/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi,

On 1/29/22 7:20 PM, Marc Zyngier wrote:
> On Sat, 29 Jan 2022 15:52:02 +0000,
> Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> (+ Marc)
>>
>> On Fri, 28 Jan 2022 at 16:41, <Pierre.Gondois@arm.com> wrote:
>>>
>>> From: Pierre Gondois <Pierre.Gondois@arm.com>
>>>
>>> In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts
>>> can be defined following 2 models.
>>> In the first model, a _SRS object must be described to modify the PCI
>>> interrupt. The _CRS and _PRS object allows to describe the PCI
>>> interrupt (level/edge triggered, active high/low).
>>> In the second model, the PCI interrupt cannot be described with a
>>> similar granularity. PCI interrupts are by default level triggered,
>>> active low.
>>>
>>> GicV2 SPI interrupts are level or edge triggered, active high. To
>>> correctly describe PCI interrupts, the first model is used, even though
>>> Arm Base Boot Requirements v1.0 requires to use the second mode.
>>>
>>
>> There are two different issues here:
>>
>> - using separate 'interrupt link' device objects with an Interrupt()
>> resource rather than a simple GSIV number
>> - whether _PRS and _SRS need to be implemented on those link objects.
>>
>> The latter is simply not true - _PRS and _SRS are optional, and
>> pointless if there is only a single possible value, so there is really
>> no need to add them here.
>>
>> As for the choice between the link object or the GSIV number: I don't
>> think INTx interrupts on ARM systems are actually level low, and the
>> GSIV option is widely used, also in platforms that exist in
>> edk2-platforms, without any reported issues.
>>
>> I've cc'ed Marc, perhaps he can shed some light on this, but I'd
>> prefer to stick to the GSIV approach if we can, as it is much simpler.
> 
> I don't immediately see the point either. Yes, the GIC only supports
> level-high interrupts. However, all the PCIe implementations connected
> to it are inverting the level. If they don't, that's even simpler (the
> HW is broken).
> 
> Is this to address an apparent disconnect with the spec?
> 
> [...]
> 

1. _PRS/_SRS methods
I agree they are optional and meaningless here as interrupts are not dynamically configurable.
However linux is checking that:
- the interrupt used in _CRS is one of the possible interrupts advertised in _PRS. If not, then a warning is issued and the interrupt is not used.
- the _SRS method is present. If not, setting the interrupt fails.
If _PRS and _SRS method are really optional, it seems these checks should not happen.
For now, using a link object without _PRS/_SRS doesn't work.

Note:
The first check was initially done because an invalid interrupt was advertised in _CRS when valid interrupts were available in _PRS.
https://bugzilla.kernel.org/show_bug.cgi?id=2665

2. GSIV vs link object
The fist motivation was to accurately describe the interrupts.
Even though GIC interrupts must be active high, PCI interrupts are active low by default (according to spec), and the GSIV model doesn't allow to describe the polarity/activation state.

Another point that came out is that in linux, GSIV interrupts for PCI are configured as level triggered by default.
 From "Base System Architecture 1.0", sE.4 and sE.6, PCI interrupts can be level or edge triggered. More specifically, KvmTool configures PCI interrupts as edge triggered.
So the only way to describe an edge interrupt is to use a link object.


>>> -  The _PRS and _SRS are not supported, cf Arm Base Boot Requirements v1.0:
>>> +  Warning: The Arm Base Boot Requirements v1.0 states the following:
>>>     "The _PRS (Possible Resource Settings) and _SRS (Set Resource Settings)
>>>     are not supported."
>>> +  However, the first model to describe PCI legacy interrupts is used (cf. ACPI
>>> +  6.4 s6.2.13) as PCI defaults (Level Triggered, Active Low) are not compatible
>>> +  with GICv2 (must be Active High).
> 
> nit: GICv3 has the exact same behaviour. Why is v2 singled out?

No specific reason, GICv3 should have been in the comment aswell.

> 
> Thanks,
> 
> 	M.
> 

Regards,
Pierre


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86249): https://edk2.groups.io/g/devel/message/86249
Mute This Topic: https://groups.io/mt/88746970/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, 31 Jan 2022 12:59:11 +0000,
Pierre Gondois <pierre.gondois@arm.com> wrote:
> 
> Hi,
> 
> On 1/29/22 7:20 PM, Marc Zyngier wrote:
> > On Sat, 29 Jan 2022 15:52:02 +0000,
> > Ard Biesheuvel <ardb@kernel.org> wrote:
> >> 
> >> (+ Marc)
> >> 
> >> On Fri, 28 Jan 2022 at 16:41, <Pierre.Gondois@arm.com> wrote:
> >>> 
> >>> From: Pierre Gondois <Pierre.Gondois@arm.com>
> >>> 
> >>> In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts
> >>> can be defined following 2 models.
> >>> In the first model, a _SRS object must be described to modify the PCI
> >>> interrupt. The _CRS and _PRS object allows to describe the PCI
> >>> interrupt (level/edge triggered, active high/low).
> >>> In the second model, the PCI interrupt cannot be described with a
> >>> similar granularity. PCI interrupts are by default level triggered,
> >>> active low.
> >>> 
> >>> GicV2 SPI interrupts are level or edge triggered, active high. To
> >>> correctly describe PCI interrupts, the first model is used, even though
> >>> Arm Base Boot Requirements v1.0 requires to use the second mode.
> >>> 
> >> 
> >> There are two different issues here:
> >> 
> >> - using separate 'interrupt link' device objects with an Interrupt()
> >> resource rather than a simple GSIV number
> >> - whether _PRS and _SRS need to be implemented on those link objects.
> >> 
> >> The latter is simply not true - _PRS and _SRS are optional, and
> >> pointless if there is only a single possible value, so there is really
> >> no need to add them here.
> >> 
> >> As for the choice between the link object or the GSIV number: I don't
> >> think INTx interrupts on ARM systems are actually level low, and the
> >> GSIV option is widely used, also in platforms that exist in
> >> edk2-platforms, without any reported issues.
> >> 
> >> I've cc'ed Marc, perhaps he can shed some light on this, but I'd
> >> prefer to stick to the GSIV approach if we can, as it is much simpler.
> > 
> > I don't immediately see the point either. Yes, the GIC only supports
> > level-high interrupts. However, all the PCIe implementations connected
> > to it are inverting the level. If they don't, that's even simpler (the
> > HW is broken).
> > 
> > Is this to address an apparent disconnect with the spec?
> > 
> > [...]
> > 
> 
> 1. _PRS/_SRS methods
> I agree they are optional and meaningless here as interrupts are not dynamically configurable.
> However linux is checking that:
> - the interrupt used in _CRS is one of the possible interrupts advertised in _PRS. If not, then a warning is issued and the interrupt is not used.
> - the _SRS method is present. If not, setting the interrupt fails.
> If _PRS and _SRS method are really optional, it seems these checks should not happen.
> For now, using a link object without _PRS/_SRS doesn't work.
> 
> Note:
> The first check was initially done because an invalid interrupt was advertised in _CRS when valid interrupts were available in _PRS.
> https://bugzilla.kernel.org/show_bug.cgi?id=2665
> 
> 2. GSIV vs link object
> The fist motivation was to accurately describe the interrupts.
> Even though GIC interrupts must be active high, PCI interrupts are
> active low by default (according to spec), and the GSIV model
> doesn't allow to describe the polarity/activation state.

And any operating system that groks ACPI on arm64 already knows about
this quirks.

> Another point that came out is that in linux, GSIV interrupts for
> PCI are configured as level triggered by default.

That's part of the PCI spec. INTx is level triggered, no ifs, no
buts. Otherwise, you can't implement interrupt sharing, which legacy
PCI requires with INTx. So Linux has nothing to do with this.

> From "Base System Architecture 1.0", sE.4 and sE.6, PCI interrupts
> can be level or edge triggered.

You are confusing MSIs, which *MUST* be edge, and INTx which *MUST* be
level. These are two very different thing, and you really should not
conflate the two.

> More specifically, KvmTool configures PCI interrupts as edge
> triggered.

Well, that's a gross bug in kvmtool. I guess that it doesn't really
matter for virtio devices, but this should be fixed.

> So the only way to describe an edge interrupt is to use a link object.

MSIs should never be described in ACPI, as they are entirely SW
programmable (there is no static allocation). Only INTx must be
described, and that's strictly level.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86260): https://edk2.groups.io/g/devel/message/86260
Mute This Topic: https://groups.io/mt/88746970/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, 31 Jan 2022 at 14:53, Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 31 Jan 2022 12:59:11 +0000,
> Pierre Gondois <pierre.gondois@arm.com> wrote:
> >
> > Hi,
> >
> > On 1/29/22 7:20 PM, Marc Zyngier wrote:
> > > On Sat, 29 Jan 2022 15:52:02 +0000,
> > > Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> (+ Marc)
> > >>
> > >> On Fri, 28 Jan 2022 at 16:41, <Pierre.Gondois@arm.com> wrote:
> > >>>
> > >>> From: Pierre Gondois <Pierre.Gondois@arm.com>
> > >>>
> > >>> In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts
> > >>> can be defined following 2 models.
> > >>> In the first model, a _SRS object must be described to modify the PCI
> > >>> interrupt. The _CRS and _PRS object allows to describe the PCI
> > >>> interrupt (level/edge triggered, active high/low).
> > >>> In the second model, the PCI interrupt cannot be described with a
> > >>> similar granularity. PCI interrupts are by default level triggered,
> > >>> active low.
> > >>>
> > >>> GicV2 SPI interrupts are level or edge triggered, active high. To
> > >>> correctly describe PCI interrupts, the first model is used, even though
> > >>> Arm Base Boot Requirements v1.0 requires to use the second mode.
> > >>>
> > >>
> > >> There are two different issues here:
> > >>
> > >> - using separate 'interrupt link' device objects with an Interrupt()
> > >> resource rather than a simple GSIV number
> > >> - whether _PRS and _SRS need to be implemented on those link objects.
> > >>
> > >> The latter is simply not true - _PRS and _SRS are optional, and
> > >> pointless if there is only a single possible value, so there is really
> > >> no need to add them here.
> > >>
> > >> As for the choice between the link object or the GSIV number: I don't
> > >> think INTx interrupts on ARM systems are actually level low, and the
> > >> GSIV option is widely used, also in platforms that exist in
> > >> edk2-platforms, without any reported issues.
> > >>
> > >> I've cc'ed Marc, perhaps he can shed some light on this, but I'd
> > >> prefer to stick to the GSIV approach if we can, as it is much simpler.
> > >
> > > I don't immediately see the point either. Yes, the GIC only supports
> > > level-high interrupts. However, all the PCIe implementations connected
> > > to it are inverting the level. If they don't, that's even simpler (the
> > > HW is broken).
> > >
> > > Is this to address an apparent disconnect with the spec?
> > >
> > > [...]
> > >
> >
> > 1. _PRS/_SRS methods
> > I agree they are optional and meaningless here as interrupts are not dynamically configurable.
> > However linux is checking that:
> > - the interrupt used in _CRS is one of the possible interrupts advertised in _PRS. If not, then a warning is issued and the interrupt is not used.
> > - the _SRS method is present. If not, setting the interrupt fails.
> > If _PRS and _SRS method are really optional, it seems these checks should not happen.
> > For now, using a link object without _PRS/_SRS doesn't work.
> >
> > Note:
> > The first check was initially done because an invalid interrupt was advertised in _CRS when valid interrupts were available in _PRS.
> > https://bugzilla.kernel.org/show_bug.cgi?id=2665
> >
> > 2. GSIV vs link object
> > The fist motivation was to accurately describe the interrupts.
> > Even though GIC interrupts must be active high, PCI interrupts are
> > active low by default (according to spec), and the GSIV model
> > doesn't allow to describe the polarity/activation state.
>
> And any operating system that groks ACPI on arm64 already knows about
> this quirks.
>
> > Another point that came out is that in linux, GSIV interrupts for
> > PCI are configured as level triggered by default.
>
> That's part of the PCI spec. INTx is level triggered, no ifs, no
> buts. Otherwise, you can't implement interrupt sharing, which legacy
> PCI requires with INTx. So Linux has nothing to do with this.
>
> > From "Base System Architecture 1.0", sE.4 and sE.6, PCI interrupts
> > can be level or edge triggered.
>
> You are confusing MSIs, which *MUST* be edge, and INTx which *MUST* be
> level. These are two very different thing, and you really should not
> conflate the two.
>
> > More specifically, KvmTool configures PCI interrupts as edge
> > triggered.
>
> Well, that's a gross bug in kvmtool. I guess that it doesn't really
> matter for virtio devices, but this should be fixed.
>
> > So the only way to describe an edge interrupt is to use a link object.
>
> MSIs should never be described in ACPI, as they are entirely SW
> programmable (there is no static allocation). Only INTx must be
> described, and that's strictly level.
>

Thanks for clearing that up Marc.

So we can drop the link objects after all, which means we are not
forced to add 'optional' methods just to appease Linux. This is far
better IMHO.

In the mean time, I expect that Marc or myself will get kvmtool fixed,
but this doesn't actually matter with GSIVs anyway.

Thanks,
Ard.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86254): https://edk2.groups.io/g/devel/message/86254
Mute This Topic: https://groups.io/mt/88746970/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-