PCI: Using Method instead of Name for _BBN, _SEG seems to cause infinite loop

[martin-hughes]

This affects read/write from PCI_Config regions.

To see what I mean, make the following changes to test_region_in_pci_device in tests/operation_region.rs:

-        Name (_SEG, 4)
+        Method (_SEG, 0, NotSerialized) {
+           Return (4)
+        }

The test goes from passing almost instantly, to looping seemingly forever.

I assume the context is getting messed up somewhere, but I haven't figured it out yet.

[martin-hughes]

Is the issue that:

context_stack only gets pushed if it sees an InternalMethodCall pseudo-opcode, which itself is only generated during "normal" AML execution. But for PCI_Config operations, we call _SEG and friends directly via evaluate, not using the internal opcode?

Then for PCI_Config ops, when the method finishes, context_stack gets popped willy-nilly without anything being pushed beforehand - i.e. it gets corrupted.

Come to think of it, this would probably explain some oddities I've been struggling with where functions like _SEG or _BBN seem to be called in a loop, rather inexplicably.

Assuming I'm right, I need a little longer to figure out the actual fix... but I suspect it'll involve generating a fuller context and InternalMethodCall opcode for calls to _SEG/etc so everything matches up (@IsaacWoods as always your input would be most welcome)

[IsaacWoods]

Huh yeah wow...

The PCI methods are the only time I guess we're calling evaluate with anything other than an empty context_stack in the Interpreter. I think you're right in that we construct and pass in a new MethodContext to evaluate that method, but with the parent context_stack still there.

However, this shows a bigger bug with the interpreter architecture - you can evaluate with a shared reference to the interpreter, but then we're using a single context_stack...

I think we need to store the context_stack elsewhere - I think it might be sound to just have it as a local at the top-level of do_execute_method but I may be missing something there (or else I feel I'd have done that first time round?)

[martin-hughes]

I think the context-per-do_execute_method approach works.

To me, it looks as though context_stack is the only member of Interpreter that stops the same Interpreter instance being used in multiple threads at once. It seems reasonable that multiple contexts could operate simultaneously (bar any locking) on the same namespace. Did you think of why you didn't put the context in do_execute_method the first time round?

(My impression is that Interpreter contains all the elements needed to deal with AML on an ACPI system, so I suspect the context just fell in to the main struct on that basis...?)