On 09/08/20 11:35, Laszlo Ersek wrote:
> On 09/08/20 09:39, Igor Mammedov wrote:
>> On Mon, 7 Sep 2020 17:17:52 +0200
>> Laszlo Ersek <ler...@redhat.com> wrote:
>
>>> 1 Method (CSCN, 0, Serialized)
>>> 2 {
>>> 3 Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
>>> 4 Name (CNEW, Package (0xFF){})
>>> 5 Local_Uid = Zero
>>> 6 Local_HasJob = One
>>> 7 While ((Local_HasJob == One))
>>> 8 {
>>> 9 Local_HasJob = Zero
>>> 10 Local_HasEvent = One
>>> 11 Local_NumAddedCpus = Zero
>>> 12 While (((Local_HasEvent == One) && (Local_Uid <
>>> 0x08)))
>>> 13 {
>>> 14 Local_HasEvent = Zero
>>> 15 \_SB.PCI0.PRES.CSEL = Local_Uid
>>> 16 \_SB.PCI0.PRES.CCMD = Zero
>>> 17 If ((\_SB.PCI0.PRES.CDAT < Local_Uid))
>>> 18 {
>>> 19 Break
>>> 20 }
>>> 21
>>> 22 If ((Local_NumAddedCpus == 0xFF))
>>> 23 {
>>> 24 Local_HasJob = One
>>> 25 Break
>>> 26 }
>>> 27
>>> 28 Local_Uid = \_SB.PCI0.PRES.CDAT
>>> 29 If ((\_SB.PCI0.PRES.CINS == One))
>>> 30 {
>>> 31 CNEW [Local_NumAddedCpus] = Local_Uid
>>> 32 Local_NumAddedCpus++
>>> 33 Local_HasEvent = One
>>> 34 }
>>> 35 ElseIf ((\_SB.PCI0.PRES.CRMV == One))
>>> 36 {
>>> 37 CTFY (Local_Uid, 0x03)
>>> 38 \_SB.PCI0.PRES.CRMV = One
>>> 39 Local_HasEvent = One
>>> 40 }
>>> 41
>>> 42 Local_Uid++
>>> 43 }
>>> 44
>>> 45 If ((Local_NumAddedCpus > Zero))
>>> 46 {
>>> 47 \_SB.PCI0.SMI0.SMIC = 0x04
>>> 48 }
>>> 49
>>> 50 Local_CpuIdx = Zero
>>> 51 While ((Local_CpuIdx < Local_NumAddedCpus))
>>> 52 {
>>> 53 Local_Uid = DerefOf (CNEW [Local_CpuIdx])
>>> 54 CTFY (Local_Uid, One)
>>> 55 Debug = Local_Uid
>>> 56 \_SB.PCI0.PRES.CSEL = Local_Uid
>>> 57 \_SB.PCI0.PRES.CINS = One
>>> 58 Local_CpuIdx++
>>> 59 }
>>> 60 }
>>> 61
>>> 62 Release (\_SB.PCI0.PRES.CPLK)
>>> 63 }
>>>
>>> When we take the Break on line 25, then:
>>>
>>> (a) on line 25, the following equality holds:
>>>
>>> Local_Uid == CNEW[Local_NumAddedCpus - 1] + 1
>>>
>>> (b) on line 60, the following equality holds:
>>>
>>> Local_Uid == CNEW[Local_NumAddedCpus - 1]
>>>
>>> This means that, when we write Local_Uid to CSEL on line 15 again, then:
>>>
>>> - we effectively re-investigate the last-cached CPU (with selector value
>>> CNEW[Local_NumAddedCpus - 1])
>>>
>>> - rather than resuming the scanning right after it -- that is, with
>>> selector value (CNEW[Local_NumAddedCpus - 1] + 1) --, in the spirit of
>>> line 42.
>>>
>>> My question is: is this "rewind" intentional?
>>>
>>> Now, I don't see a functionality problem with this, as on line 57, we
>>> clear the pending insert event for the last-cached CPU, so when we
>>> re-check it, the "get pending" command will simply seek past it.
>>>
>>> But I'd like to understand if this is *precisely* your intent, or if
>>> it's an oversight and it just ends up working OK.
>> it's the later (it should not have any adverse effects) so I didn't care
>> much about restarting from the last processed CPU.
>>
>> how about moving
>>
>> 22 If ((Local_NumAddedCpus == 0xFF))
>> 23 {
>> 24 Local_HasJob = One
>> 25 Break
>> 26 }
>>
>> right after
>> 40 }
>> 41
>> 42 Local_Uid++
>>
>> instead of adding extra 'if' at the end of outer loop?
>
> That only seems to save a CSEL write on line 15, during the first
> iteration of the outer loop. And we would still re-select the last
> selector from CNEW, in the second iteration of the outer loop.
>
> But, again, there's no bug; I just wanted to understand your intent.
>
> Can you please squash the following patch:
>
>> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
>> index 12839720018e..8dd4d8ebbf55 100644
>> --- a/hw/acpi/cpu.c
>> +++ b/hw/acpi/cpu.c
>> @@ -601,6 +601,15 @@ void build_cpus_aml(Aml *table, MachineState *machine,
>> CPUHotplugFeatures opts,
>> aml_append(while_ctx, aml_increment(cpu_idx));
>> }
>> aml_append(while_ctx2, while_ctx);
>> + /*
>> + * If another batch is needed, then it will resume scanning
>> + * exactly at -- and not after -- the last CPU that's
>> currently
>> + * in CPU_ADDED_LIST. In other words, the last CPU in
>> + * CPU_ADDED_LIST is going to be re-checked. That's OK:
>> we've
>> + * just cleared the insert event for *all* CPUs in
>> + * CPU_ADDED_LIST, including the last one. So the scan will
>> + * simply seek past it.
>> + */
>> }
>> aml_append(method, while_ctx2);
>> aml_append(method, aml_release(ctrl_lock));
>
> With that:
>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>
> I'll also follow up with test results for this patch (meaning a lowered
> "max_cpus_per_pass").
Tested-by: Laszlo Ersek <ler...@redhat.com>
