[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH v2] Fix failure cleanup in EvtchnFifoAcquire


  • To: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>, "win-pv-devel@xxxxxxxxxxxxxxxxxxxx" <win-pv-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: "Durrant, Paul" <pdurrant@xxxxxxxxxxxx>
  • Date: Fri, 18 Jul 2025 11:08:15 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amazon.co.uk; dmarc=pass action=none header.from=amazon.co.uk; dkim=pass header.d=amazon.co.uk; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=kpFNK/o9XK9sECRPFDQMwAob4j5MQTj/xb8m833A4MQ=; b=opreFJy79rDAvNaAZWHncxBGHwn8+kdgEbo3iJi9tie/aznZgOTNQC/imbFw08PYyyGGl6JTSIt1coVVrMtTnDqX3mnHVyqJVuyyr4I9aQCk/4QMTOa7zzgqjcpz4W8vjnyqn5C+ucVgQj95OGVYKHTDz+Ac01/h6HeITkeadxAj71kZvf2yEPPacxN0YkdZIg8bk292olLYOmFypJwowR93zxxzo/pmN3Oh0XL3h+1ob5pLp0hjm+QQhLjBJZN4Xk78RLd+r0DMo8hv8VWPVVGqnCG4vna8RHn6HUD6TFskcOxEXPJUq8BqMhbZqDeTBDfP2DRXTYIvztVpvX/Mig==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=bZXVWWGd1nnYYKPVeOZyo5iRdskQIQ2GUpcf1bEvE+m9gd4mk4FujU4z/iYz0b22s/IdTTa828AifYoMyd0uFsYEg8Z5k4TigXXXYeLaRezOUdUxyGCeV4ArL63pVnX7Xz6lbpY3sCzLy4IYuQv7GmYIaqI0FwGTux3IurVj21OvQZe3EoxwXV9N96JjUHBrLfxcn1pF1Ptqf893CG63ZVc1X/nCzZCXa1QCH+xUgdBp9Cw8BtzpLgJR5feZHrWl2xbYTVUV1GkR3BJScLewrQ1aycqkq/3PjdmVQIcOLDJcocI6wqmdbDEXFa8cxaMVmoW494nONIFbeENxzbxQgQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amazon.co.uk;
  • Cc: Owen Smith <owen.smith@xxxxxxxxx>
  • Delivery-date: Fri, 18 Jul 2025 11:08:33 +0000
  • List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>
  • Thread-index: AQHb98qO3ZMDHqfNrUamuEX8SjeBh7Q3qlrggAAKrACAAAOLQA==
  • Thread-topic: [PATCH v2] Fix failure cleanup in EvtchnFifoAcquire

> -----Original Message-----
> From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
> Sent: Friday, July 18, 2025 11:54 AM
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; win-pv-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH v2] 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.
> 
> 
> 
> On 18/07/2025 12:16, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On
> Behalf
> >> Of Tu Dinh
> >> Sent: Friday, July 18, 2025 10:59 AM
> >> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>; Owen Smith
> <owen.smith@xxxxxxxxx>;
> >> Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> >> Subject: [EXTERNAL] [PATCH v2] 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.
> >>
> >>
> >>
> >> The current index is incremented before checking for failure:
> >>
> >>      while (Index < ProcessorCount) {
> >>          Index++;
> >>          [...]
> >>          if (!NT_SUCCESS(status))
> >>              goto fail1;
> >>          Context->ControlBlockMdl[vcpu_id] = Mdl;
> >>      }
> >>
> >> Make the main loop a normal for loop to avoid calling __FreePage on
> >> invalid PMDLs.
> >>
> >> Signed-off-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
> >> ---
> >>   src/xenbus/evtchn_fifo.c | 5 +----
> >>   1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
> >> index 1137dea..fd06ef4 100644
> >> --- a/src/xenbus/evtchn_fifo.c
> >> +++ b/src/xenbus/evtchn_fifo.c
> >> @@ -502,16 +502,13 @@ EvtchnFifoAcquire(
> >>
> >>       ProcessorCount =
> >> KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);
> >>
> >> -    Index = 0;
> >> -    while (Index < ProcessorCount) {
> >> +    for (Index = 0; Index < ProcessorCount; Index++) {
> >
> > Prevailing style uses while loops I think so stick with that for
> consistency, just move the increment down.
> 
> I don't see how the increment could be moved down without breaking the
> index skip when SystemProcessorVcpuId() == STATUS_NOT_SUPPORTED.
> 

That's true, it would need a separate increment in that case.

> I also find the while loop usage in EvtchnFifoAcquire very confusing,
> and it was the cause of the cleanup bug in the first place. So I'd
> prefer to make it more idiomatic this way.
> 

More idiomatic but less consistent. Anyway...

Reviewed-by: Paul Durrant <paul@xxxxxxx>

> >
> >>           unsigned int        vcpu_id;
> >>           PFN_NUMBER          Pfn;
> >>           PHYSICAL_ADDRESS    Address;
> >>
> >>           status = SystemProcessorVcpuId(Index, &vcpu_id);
> >>
> >> -        Index++;
> >> -
> >>           if (status == STATUS_NOT_SUPPORTED)
> >>               continue;
> >>
> >> --
> >> 2.50.1.windows.1
> >>
> >>
> >>
> >> 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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.