On Wed, Jul 06, 2022 at 11:52:56AM +0200, Pierre Gondois wrote: > On 7/5/22 19:29, Bjorn Helgaas wrote: > > On Fri, Jul 01, 2022 at 06:16:23PM +0200, Pierre Gondois wrote: > > > From: Pierre Gondois <Pierre.Gondois@xxxxxxx> > > > > > > In ACPI 6.4, s6.2.13 "_PRT (PCI Routing Table)", PCI legacy > > > interrupts can be described though a link device (first model). > > > From s6.2.16 "_SRS (Set Resource Settings)": > > > "This optional control method [...]" > > > > > > Make it optional to have a _SRS method for link devices. > > > > I think it would be helpful to outline the reason for wanting these > > changes in the commit log. Otherwise we don't know the benefit and > > it's harder to justify making the change since it's not an obvious > > cleanup. > > > > IIRC from [1] there *is* a good reason: you need to use Interrupt Link > > devices so you can specify "level triggered, active high". > > > > Without an Interrupt Link, you would get the default "level triggered, > > active low" setting, which apparently isn't compatible with GICv2. > > > > I assume this fixes a device that previously didn't work correctly, > > but I don't see the details of that in the bugzilla. I'm a little > > confused about this. Isn't GICv2 widely used already? How are things > > working now? Or are there just a lot of broken devices? > > It was unsure which of the 2 models described in ACPI 6.4, s6.2.13 > "_PRT (PCI Routing Table)" would be used for UEFI for kvmtool. > > Remainder: > The first model allows to accurately describe interrupts: level/edge > triggered and active high/low. Interrupts are also configurable with > _CRS/_PRS/_SRS/_DIS methods > The second model allows to describe hardwired interrupts, and are > by default level triggered, active low. > > The kernel is aware that GivV2 interrupts are active high, so there > was actually no need to accurately describe them. Thus the second > model was used. > While experimenting, we temporarily had a configuration using > the first model, and only had a _CRS method (no _PRS/_SRS), which > triggered some warnings. OK, thanks. So it sounds like there is some existing kernel code that special-cases GICv2 interrupts to make them level/high, and that code would not have been necessary if _PRS/_SRS had been optional from the beginning. I don't think we could ever *remove* that code because there's firmware in the field that relies on it, and that firmware will never be updated. > So these patches are not fixes for existing platforms, but merely > to make _PRS/_SRS methods optional. > > In [1] I said I would submit patches to change that. If you think > this is not necessary as the configuration is non-existing, I am > perfectly fine to drop the patches. > > Also as Rafael noted, the _DIS method should also be taken into > consideration if _PRS/_SRS are made optional. But that said, I'm not opposed to making _PRS/_SRS optional if that makes legal and reasonable _PRT descriptions work, and if all the considerations Rafael mentioned are taken care of. Bjorn
