Re: [PATCH] Skip uninitialized CPUs

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 16/02/2023 11:55, Owen Smith wrote:
> From: Owen Smith <owen.smith@xxxxxxxxxx>
>
> EvtchnFifoAcquire will loop through all CPUs to call EVTCHNOP_init_control.
> Skip any CPUs that are not initialized, which is indicated by
> SystemProcessorVcpuId failing, instead of failing the Acquire operation.
>
> This is primarily an issue when KeQueryMaximumProcessorCountEx returns
> a different value to KeQueryActiveProcessorCountEx, or the system
> processor callback has not been called with KeProcessorAddCompleteNotify
> for that CPU.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/xenbus/evtchn_fifo.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
> index a8dab8c..7a74af7 100644
> --- a/src/xenbus/evtchn_fifo.c
> +++ b/src/xenbus/evtchn_fifo.c
> @@ -501,20 +501,21 @@ EvtchnFifoAcquire(
>   
>       ProcessorCount = KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);
>   
> -    Index = 0;
> -    while (Index < ProcessorCount) {
> +    for (Index = 0; Index < ProcessorCount; ++Index) {
>           unsigned int        vcpu_id;
>           PFN_NUMBER          Pfn;
>           PHYSICAL_ADDRESS    Address;
>   
> +        status = SystemProcessorVcpuId(Index, &vcpu_id);
> +        if (status == STATUS_NOT_SUPPORTED)
> +            continue;
I think this would probably cause a bug in the loop in the 'fail1' exit 
below where SystemProcessorVcpuId() is called without checking the 
return value and Context->ControlBlockMdl[vcpu_id] is assumed to be 
non-NULL. I can fix this up.
Acked-by: Paul Durrant <paul@xxxxxxx>

> +        if (!NT_SUCCESS(status))
> +            goto fail1;
> +
>           Mdl = __AllocatePage();
>   
>           status = STATUS_NO_MEMORY;
>           if (Mdl == NULL)
> -            goto fail1;
> -
> -        status = SystemProcessorVcpuId(Index, &vcpu_id);
> -        if (!NT_SUCCESS(status))
>               goto fail2;
>   
>           Pfn = MmGetMdlPfnArray(Mdl)[0];
> @@ -532,8 +533,6 @@ EvtchnFifoAcquire(
>                     Address.LowPart);
>   
>           Context->ControlBlockMdl[vcpu_id] = Mdl;
> -
> -        Index++;
>       }
>   
>       Trace("<====\n");
> @@ -546,11 +545,11 @@ done:
>   fail3:
>       Error("fail3\n");
>   
> +    __FreePage(Mdl);
> +
>   fail2:
>       Error("fail2\n");
>   
> -    __FreePage(Mdl);
> -
>   fail1:
>       Error("fail1 (%08x)\n", status);
>