Libguestfs - Mar 2023 - [V2V PATCH 1/1] convert_windows: add firstboot script to install drivers with pnputil

On 3/9/23 14:48, Richard W.M. Jones wrote:
> On Thu, Mar 09, 2023 at 03:47:10PM +0200, Andrey Drobyshev wrote:
>> On 3/8/23 22:52, Richard W.M. Jones wrote:
>>> On Wed, Mar 08, 2023 at 08:05:35PM +0200, Andrey Drobyshev wrote:
>>>> During conversion we copy the necessary drivers to the
directory
>>>> "%systemroot%\Drivers\Virtio", adding it to the
DevicePath registry
>>>> value.  As documented in [1], this should be enough for Windows
to find
>>>> device drivers and successfully install them.
>>>>
>>>> However, it doesn't always happen.  Commit 73e009c04
("v2v: windows:
>>>> Document use of pnputil to install drivers.") describes
such issues with
>>>> Win2k12R2.  I'm seeing the same problem with Win2k16 and
netkvm.sys
>>>> driver not being installed.
>>>>
>>>> That same commit 73e009c04 suggests adding a firstboot script
invoking
>>>> pnputil at an early stage to install all the drivers we put
into the
>>>> drivers store.  So let's add such a script to make sure all
the
>>>> necessary drivers are installed.
>>>>
>>>> [1]
https://learn.microsoft.com/en-us/windows-hardware/drivers/install/how-windows-selects-a-driver-for-a-device
>>>>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
>>>> ---
>>>>  convert/convert_windows.ml | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/convert/convert_windows.ml
b/convert/convert_windows.ml
>>>> index 6bc2343b..e15a5e62 100644
>>>> --- a/convert/convert_windows.ml
>>>> +++ b/convert/convert_windows.ml
>>>> @@ -295,9 +295,11 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>      | Virt -> Virt
>>>>  
>>>>    and configure_firstboot () >>>> -    (* Note
that pnp_wait.exe must be the first firstboot script as it
>>>> -     * suppresses PnP for all following scripts.
>>>> +    (* Run the firstboot script with pnputil.exe before the
one with
>>>> +     * pnp_wait.exe as the latter suppresses PnP for all
following scripts.
>>>>       *)
>>>> +    configure_pnputil_install ();
>>>> +
>>>>      let tool_path = virt_tools_data_dir () //
"pnp_wait.exe" in
>>>>      if Sys.file_exists tool_path then
>>>>        configure_wait_pnp tool_path
>>>> @@ -345,6 +347,16 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>                       
strkey name value
>>>>      | None -> sprintf "reg delete \"%s\" /v
%s /f" strkey name
>>>>  
>>>> +  and configure_pnputil_install () >>>> +    let
fb_script = "@echo off\n\
>>>> +                     \n\
>>>> +                     echo Wait for VirtIO drivers to be
installed\n\
>>>> +                     %systemroot%\\Sysnative\\PnPutil -i -a \
>>>> +                     %systemroot%\\Drivers\\Virtio\\*.inf
>\"%~dpn0.log\" 2>&1\
>>>> +                    " in
>>>> +    Firstboot.add_firstboot_script g inspect.i_root
>>>> +      "pnputil install drivers" fb_script;
>>>> +
>>>
>>> I'm not sure I'm really qualified to comment on this, since
Windows is
>>> crazy.  I guess the worst this can do is fail to run, but that
won't
>>> stop anything else from happening.
>>>
>>> Note that firstboot scripts already do logging, so I don't
believe
>>> writing to a separate log file is needed here.  All the output from
>>> all firstboot scripts should go to
>>> C:\Program Files\Guestfs\Firstboot\log.txt:
>>>
>>>
https://github.com/libguestfs/libguestfs-common/blob/7acf991a25b3fd625eb1ff1fbd8dc9fedf245942/mlcustomize/firstboot.ml#L276
>>
>> Right, but apart from having the log common for all firstboot scripts,
>> some of them also utilise separate log files in scripts-done directory,
>> e.g.:
>>
>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L381
>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L168
>>
>> So I did the same considering that pnputil's output is relatively
long.
> 
> I think I'd probably get rid of those other scripts too.  It's
helpful
> to have everything go to one place.  I'm not actually sure where those
> extra files end up.
> 
>> All in all, if there're no other concerns, can we give this script
a go?
> 
> I think the patch is generally fine, so I'm just quibbling about
> the logging.
I'm surprised by this patch (which doesn't say much of course, because I
know precious little about Windows). What I'm noticing is "run
pnputil.exe before pnp_wait.exe before everything else".

We do queue a script earlier than those, at priority 2500, called
"v2vnetcf.ps1". See configure_network_interfaces ->
add_firstboot_powershell, and the reference to RHBZ 1788823.

And that script is exactly what tends *not* to work, and we've been
unable to get to the bottom of that issue. (See also RH 2068361 and
1963032.)

So... This patch could be orthogonal to that pre-existent symptom... but
could the patch perhaps *fix* the prior symptom? Is it possible that, if
we "forcibly install" the virtio drivers first, and *then* try to
configure IP addresses for the virtio-net NICs, then the latter will
start working reliably?

I have no idea really.

Anyway, once Rich is happy with the logging here, I have nothing against
the patch.

Acked-by: Laszlo Ersek <lersek at redhat.com>

On 3/13/23 11:28, Laszlo Ersek wrote:
> On 3/9/23 14:48, Richard W.M. Jones wrote:
>> On Thu, Mar 09, 2023 at 03:47:10PM +0200, Andrey Drobyshev wrote:
>>> On 3/8/23 22:52, Richard W.M. Jones wrote:
>>>> On Wed, Mar 08, 2023 at 08:05:35PM +0200, Andrey Drobyshev
wrote:
>>>>> During conversion we copy the necessary drivers to the
directory
>>>>> "%systemroot%\Drivers\Virtio", adding it to the
DevicePath registry
>>>>> value.  As documented in [1], this should be enough for
Windows to find
>>>>> device drivers and successfully install them.
>>>>>
>>>>> However, it doesn't always happen.  Commit 73e009c04
("v2v: windows:
>>>>> Document use of pnputil to install drivers.")
describes such issues with
>>>>> Win2k12R2.  I'm seeing the same problem with Win2k16
and netkvm.sys
>>>>> driver not being installed.
>>>>>
>>>>> That same commit 73e009c04 suggests adding a firstboot
script invoking
>>>>> pnputil at an early stage to install all the drivers we put
into the
>>>>> drivers store.  So let's add such a script to make sure
all the
>>>>> necessary drivers are installed.
>>>>>
>>>>> [1]
https://learn.microsoft.com/en-us/windows-hardware/drivers/install/how-windows-selects-a-driver-for-a-device
>>>>>
>>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at
virtuozzo.com>
>>>>> ---
>>>>>  convert/convert_windows.ml | 16 ++++++++++++++--
>>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/convert/convert_windows.ml
b/convert/convert_windows.ml
>>>>> index 6bc2343b..e15a5e62 100644
>>>>> --- a/convert/convert_windows.ml
>>>>> +++ b/convert/convert_windows.ml
>>>>> @@ -295,9 +295,11 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>>      | Virt -> Virt
>>>>>  
>>>>>    and configure_firstboot () >>>>> -    (*
Note that pnp_wait.exe must be the first firstboot script as it
>>>>> -     * suppresses PnP for all following scripts.
>>>>> +    (* Run the firstboot script with pnputil.exe before
the one with
>>>>> +     * pnp_wait.exe as the latter suppresses PnP for all
following scripts.
>>>>>       *)
>>>>> +    configure_pnputil_install ();
>>>>> +
>>>>>      let tool_path = virt_tools_data_dir () //
"pnp_wait.exe" in
>>>>>      if Sys.file_exists tool_path then
>>>>>        configure_wait_pnp tool_path
>>>>> @@ -345,6 +347,16 @@ let convert (g : G.guestfs) _ inspect
i_firmware block_driver _ static_ips >>>>>                       
strkey name value
>>>>>      | None -> sprintf "reg delete \"%s\"
/v %s /f" strkey name
>>>>>  
>>>>> +  and configure_pnputil_install () >>>>> + 
let fb_script = "@echo off\n\
>>>>> +                     \n\
>>>>> +                     echo Wait for VirtIO drivers to be
installed\n\
>>>>> +                     %systemroot%\\Sysnative\\PnPutil -i
-a \
>>>>> +                     %systemroot%\\Drivers\\Virtio\\*.inf
>\"%~dpn0.log\" 2>&1\
>>>>> +                    " in
>>>>> +    Firstboot.add_firstboot_script g inspect.i_root
>>>>> +      "pnputil install drivers" fb_script;
>>>>> +
>>>>
>>>> I'm not sure I'm really qualified to comment on this,
since Windows is
>>>> crazy.  I guess the worst this can do is fail to run, but that
won't
>>>> stop anything else from happening.
>>>>
>>>> Note that firstboot scripts already do logging, so I don't
believe
>>>> writing to a separate log file is needed here.  All the output
from
>>>> all firstboot scripts should go to
>>>> C:\Program Files\Guestfs\Firstboot\log.txt:
>>>>
>>>>
https://github.com/libguestfs/libguestfs-common/blob/7acf991a25b3fd625eb1ff1fbd8dc9fedf245942/mlcustomize/firstboot.ml#L276
>>>
>>> Right, but apart from having the log common for all firstboot
scripts,
>>> some of them also utilise separate log files in scripts-done
directory,
>>> e.g.:
>>>
>>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L381
>>>
https://github.com/libguestfs/virt-v2v/blob/cb792fef27f/convert/convert_windows.ml#L168
>>>
>>> So I did the same considering that pnputil's output is
relatively long.
>>
>> I think I'd probably get rid of those other scripts too.  It's
helpful
>> to have everything go to one place.  I'm not actually sure where
those
>> extra files end up.
>>
>>> All in all, if there're no other concerns, can we give this
script a go?
>>
>> I think the patch is generally fine, so I'm just quibbling about
>> the logging.
> 
> I'm surprised by this patch (which doesn't say much of course,
because I
> know precious little about Windows). What I'm noticing is "run
> pnputil.exe before pnp_wait.exe before everything else".
> 
> We do queue a script earlier than those, at priority 2500, called
> "v2vnetcf.ps1". See configure_network_interfaces ->
> add_firstboot_powershell, and the reference to RHBZ 1788823.
> 
> And that script is exactly what tends *not* to work, and we've been
> unable to get to the bottom of that issue. (See also RH 2068361 and
> 1963032.)
> 
> So... This patch could be orthogonal to that pre-existent symptom... but
> could the patch perhaps *fix* the prior symptom? Is it possible that, if
> we "forcibly install" the virtio drivers first, and *then* try to
> configure IP addresses for the virtio-net NICs, then the latter will
> start working reliably?
Could it be that what you've seen with non-working v2vnetcf.ps1 solution
is directly connected with the issue we're facing here, i.e. drivers
from the driver store not being automatically installed?

Is so, we may try prioritizing pnputil even further to make sure that
*ALL* the drivers (including netkvm.sys) are installed before anything
else happens.  For instance, assign it "~prio:2000".

What do you think?
> 
> I have no idea really.
> 
> Anyway, once Rich is happy with the logging here, I have nothing against
> the patch.
> 
> Acked-by: Laszlo Ersek <lersek at redhat.com>