Re: [kvm-unit-tests PATCH v3 3/3] lib: s390x: better smp interrupt checks

Janosch Frank
2022-07-13T13:13:45Z

On 7/13/22 15:07, Claudio Imbrenda wrote:
> On Wed, 13 Jul 2022 14:24:57 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> On 7/13/22 12:45, Claudio Imbrenda wrote:
>>> Use per-CPU flags and callbacks for Program and Extern interrupts,
>>> instead of global variables.
>>>
>>> This allows for more accurate error handling; a CPU waiting for an
>>> interrupt will not have it "stolen" by a different CPU that was not
>>> supposed to wait for one, and now two CPUs can wait for interrupts at
>>> the same time.
>>>
>>> This will significantly improve error reporting and debugging when
>>> things go wrong.
>>>
>>> Both program interrupts and external interrupts are now CPU-bound, even
>>> though some external interrupts are floating (notably, the SCLP
>>> interrupt). In those cases, the testcases should mask interrupts and/or
>>> expect them appropriately according to need.
>>>
>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> ---
>>> lib/s390x/asm/arch_def.h | 16 ++++++++++-
>>> lib/s390x/smp.h | 8 +-----
>>> lib/s390x/interrupt.c | 57 +++++++++++++++++++++++++++++-----------
>>> lib/s390x/smp.c | 11 ++++++++
>>> 4 files changed, 69 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>>> index b3282367..03578277 100644
>>> --- a/lib/s390x/asm/arch_def.h
>>> +++ b/lib/s390x/asm/arch_def.h
>>> @@ -41,6 +41,17 @@ struct psw {
>>> uint64_t addr;
>>> };
>>>
>>> +struct cpu {
>>> + struct lowcore *lowcore;
>>> + uint64_t *stack;
>>> + void (*pgm_cleanup_func)(void);
>>
>> We should change the parameter to include the stack frame for easier
>> manipulation of the pre-exception registers, especially the CRs.
>
> will do
>
>>
>>> + uint16_t addr;
>>> + uint16_t idx;
>>> + bool active;
>>> + bool pgm_int_expected;
>>> + bool ext_int_expected;
>>> +};
>>
>> And I'd opt for also integrating the io handling function and getting
>> rid of the unset function to make them all look the same.
>
> I/O is usually floating, though, I don't think it makes sense to have
> it per-cpu

Right, it just bugs me that it's handled so differently.
I'll find a solution for that if my eyes stumble over it too often.

>
>>
>> Looking at Nico's patches the external handler will follow soon anyway.
>
> should I add the external handler here?

Discuss that with Nico, I don't have a strong opinion on that

>
>>
>>
>> I'm not 100% happy with having this struct in this file, what kept you
>> from including smp.h?
>
> smp.h depends on arch_def.h, which then would depend on smp.h
>
>>
>>> +struct lowcore *smp_get_lowcore(uint16_t idx)
>>> +{
>>> + if (THIS_CPU->idx == idx)
>>> + return &lowcore;
>>> +
>>> + check_idx(idx);
>>> + return cpus[idx].lowcore;
>>> +}
>>
>> I'm waiting for the moment where we need locking in the struct cpu.
>>
>>> +
>>> int smp_sigp(uint16_t idx, uint8_t order, unsigned long parm, uint32_t *status)
>>> {
>>> check_idx(idx);
>>> @@ -253,6 +262,7 @@ static int smp_cpu_setup_nolock(uint16_t idx, struct psw psw)
>>>
>>> /* Copy all exception psws. */
>>> memcpy(lc, cpus[0].lowcore, 512);
>>> + lc->this_cpu = &cpus[idx];
>>>
>>> /* Setup stack */
>>> cpus[idx].stack = (uint64_t *)alloc_pages(2);
>>> @@ -325,6 +335,7 @@ void smp_setup(void)
>>> for (i = 0; i < num; i++) {
>>> cpus[i].addr = entry[i].address;
>>> cpus[i].active = false;
>>> + cpus[i].idx = i;
>>> /*
>>> * Fill in the boot CPU. If the boot CPU is not at index 0,
>>> * swap it with the one at index 0. This guarantees that the
>>
>