Re: [XENVIF PATCH] Fix argument to ControllerSetHashAlgorithm

[Owen Smith <owen.smith@xxxxxxxxx>]

From what I can tell, xennet will only set NONE or TOEPLITZ. But xenvif defaults the Hash->Algorithm to UNSPECIFIED, so its possible that xennet may not set the algorithm and leave it as default.

UNSPECIFIED seems to only be used as a log line to detect when xennet has not set an algorithm.

Either UNSPECIFIED should be treated the same as NONE, or removed entirely - though removal would require a change to the interface header, and a version change...

    switch (Hash->Algorithm) {
    case XENVIF_PACKET_HASH_ALGORITHM_NONE:
    case XENVIF_PACKET_HASH_ALGORITHM_UNSPECIFIED:
        NetifAlgorithm = XEN_NETIF_CTRL_HASH_ALGORITHM_NONE;
        Size = 1;
        Mapping = &Zero;
        Flags = 0;
        break;

adding UNSPECIFIED to this switch in __FrontendUpdateHash is likely the simplest solution.
At this point, Size, Mapping and Flags dont need to be assigned when NetifAlgorithm is NONE, as they are not used due to the 'goto done' code path

Owen

On Wed, Jul 23, 2025 at 9:38 AM Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx> wrote:

On 23/07/2025 10:32, Owen Smith wrote:
> XENVIF_PACKET_HASH_ALGORITHM_UNSPECIFIED seems to be a valid value, used
> to determine if xennet requested or did not request a specific
> algorithm, so I think making XENVIF_PACKET_HASH_ALGORITHM_UNSPECIFIED
> report STATUS_NOT_SUPPORTED could cause issues.
>
> The mapping from XENVIF_PACKET_HASH_ALGORITHM values
> to XEN_NETIF_CTRL_HASH_ALGORITHM_* is a good fix.

I don't mind reverting the behavior, but I couldn't find any mention of
XENVIF_PACKET_HASH_ALGORITHM_UNSPECIFIED in xennet, Windows also only
supports NdisHashFunctionToeplitz. Was this used in a previous xennet
version? Also, the default case may very well break if other hash
algorithms were introduced in the future...

>
> Owen
>
> On Wed, Jul 23, 2025 at 8:53 AM Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx> wrote:
>
>     ControllerSetHashAlgorithm expects a XEN_NETIF_CTRL_HASH_ALGORITHM_*
>     whereas Hash->Algorithm is of type XENVIF_PACKET_HASH_ALGORITHM.
>
>     These two enums are not aligned so translate them manually.
>
>     Also simply return STATUS_NOT_SUPPORTED when an invalid Hash->Algorithm
>     is supplied.
>
>     Signed-off-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
>     ---
>       src/xenvif/frontend.c | 11 ++++++-----
>       1 file changed, 6 insertions(+), 5 deletions(-)
>
>     diff --git a/src/xenvif/frontend.c b/src/xenvif/frontend.c
>     index 79b04fb..86bebf8 100644
>     --- a/src/xenvif/frontend.c
>     +++ b/src/xenvif/frontend.c
>     @@ -1883,6 +1883,7 @@ __FrontendUpdateHash(
>       {
>           PXENVIF_CONTROLLER      Controller;
>           ULONG                   Zero = 0;
>     +    ULONG                   NetifAlgorithm;
>           ULONG                   Size;
>           PULONG                  Mapping;
>           ULONG                   Flags;
>     @@ -1892,12 +1893,14 @@ __FrontendUpdateHash(
>
>           switch (Hash->Algorithm) {
>           case XENVIF_PACKET_HASH_ALGORITHM_NONE:
>     +        NetifAlgorithm = XEN_NETIF_CTRL_HASH_ALGORITHM_NONE;
>               Size = 1;
>               Mapping = &Zero;
>               Flags = 0;
>               break;
>
>           case XENVIF_PACKET_HASH_ALGORITHM_TOEPLITZ:
>     +        NetifAlgorithm = XEN_NETIF_CTRL_HASH_ALGORITHM_TOEPLITZ;
>               Size = Hash->Size;
>               Mapping = Hash->Mapping;
>               Flags = Hash->Flags;
>     @@ -1905,17 +1908,15 @@ __FrontendUpdateHash(
>
>           case XENVIF_PACKET_HASH_ALGORITHM_UNSPECIFIED:
>           default:
>     -        (VOID) ControllerSetHashAlgorithm(Controller,
>     -                                         
>     XEN_NETIF_CTRL_HASH_ALGORITHM_NONE);
>     -        goto done;
>     +        return STATUS_NOT_SUPPORTED;
>           }
>
>           status = ControllerSetHashAlgorithm(Controller,
>     -                                        Hash->Algorithm);
>     +                                        NetifAlgorithm);
>           if (!NT_SUCCESS(status))
>               goto fail1;
>
>     -    if (Hash->Algorithm == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE)
>     +    if (NetifAlgorithm == XEN_NETIF_CTRL_HASH_ALGORITHM_NONE)
>               goto done;
>
>           status = ControllerSetHashMappingSize(Controller, Size);
>     --
>     2.50.1.windows.1
>
>
>
>     Ngoc Tu Dinh | Vates XCP-ng Developer
>
>     XCP-ng & Xen Orchestra - Vates solutions
>
>     web: https://vates.tech <https://vates.tech>
>



Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech