RE: [PATCH] Fix failure cleanup in EvtchnFifoAcquire

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----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.

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