Re: [XENVIF PATCH] Fix argument to ControllerSetHashAlgorithm

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

On 23/07/2025 10:53, Owen Smith wrote:
>  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

That sounds good to me, it also tracks with the other places that handle
XENVIF_PACKET_HASH_ALGORITHM. I'll prepare a v2.

>
> 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> <https://
>     vates.tech <https://vates.tech>>
>      >
>
>
>
>     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