Re: [PATCH] Fix failure cleanup in EvtchnFifoAcquire

["Tu Dinh" <ngoc-tu.dinh@xxxxxxxxxx>]

On 18/07/2025 11:14, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
>> Sent: Friday, July 18, 2025 10:07 AM
>> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; win-pv-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Owen Smith <owen.smith@xxxxxxxxx>
>> Subject: RE: [EXTERNAL] [PATCH] Fix failure cleanup in EvtchnFifoAcquire
>>
>> CAUTION: This email originated from outside of the organization. Do not
>> click links or open attachments unless you can confirm the sender and know
>> the content is safe.
>>
>>
>>
>> Hi Paul,
>>
>> On 18/07/2025 11:04, Durrant, Paul wrote:
>> [...]
>>>
>>> Hi Tu,
>>>
>>>     Are we looking at the same function? This is what I see...
>>>
>>>     The loop starts at
>> https://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xe
>> nbus/evtchn_fifo.c;hb=HEAD#l505:
>>>
>>>    505     Index = 0;
>>>    506     while (Index < ProcessorCount) {
>>>    507         unsigned int        vcpu_id;
>>>    508         PFN_NUMBER          Pfn;
>>>    509         PHYSICAL_ADDRESS    Address;
>>>    510
>>>    511         status = SystemProcessorVcpuId(Index, &vcpu_id);
>>>    512
>>>    513         Index++;
>>>    514
>>>
>>> So Index is incremented before the checks:
>>>
>>>    515         if (status == STATUS_NOT_SUPPORTED)
>>>    516             continue;
>>>    517
>>>    518         if (!NT_SUCCESS(status))
>>>    519             goto fail1;
>>>
>>> So, say we have done one loop round then we'll have set up the control
>> block for the vcpu_id corresponding to Index 0. Index will now be 1 and
>> let's say we hit the fail.
>>
>> After doing one successful loop round, Index will be 1. But as soon as
>> you enter the second loop round, Index would have become 2 before you
>> could hit any failure, due to the Index++ near the very top.
> 
> Ok, yes I see it now. Just moving the increment below line 519 appears to be 
> the correct fix then.

I think the index increment needs to stay there to skip CPU indexes in 
case SystemProcessorVcpuId returns STATUS_NOT_SUPPORTED (non-present CPUs).

I've made a facsimile of the function here for manual testing: 
https://gist.github.com/dinhngtu/2af15af5aefd33ddbb0165e9b430a441

> 
> Cheers,
> 
>    Paul
> 
>>
>>>
>>>    559 fail1:
>>>    560     Error("fail1 (%08x)\n", status);
>>>    561
>>>    562     EvtchnReset();
>>>    563
>>>    564     while (--Index >= 0) {
>>>
>>> Index will be decremented back to 0...
>>>
>>>    565         unsigned int    vcpu_id;
>>>    566
>>>    567         status = SystemProcessorVcpuId(Index, &vcpu_id);
>>>
>>> We look up the corresponding vcpu_id again...
>>>
>>>    568         if (status == STATUS_NOT_SUPPORTED)
>>>    569             continue;
>>>    570
>>>    571         BUG_ON(!NT_SUCCESS(status));
>>>    572
>>>    573         Mdl = Context->ControlBlockMdl[vcpu_id];
>>>    574         Context->ControlBlockMdl[vcpu_id] = NULL;
>>>
>>> ... and we free off the control block that was allocated before.
>>>
>>>    575
>>>    576         __FreePage(Mdl);
>>>    577     }
>>>
>>> I don't see any evidence of a double increment. The existing code looks
>> correct to me.
>>>
>>>     Paul
>>>
>> [...]
>>
>>
>>
>> Ngoc Tu Dinh | Vates XCP-ng Developer
>>
>> XCP-ng & Xen Orchestra - Vates solutions
>>
>> web: https://vates.tech
> 



Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech