Re: patch review: avoid com delay() in VMs

> On Feb 4, 2025, at 11:40 PM, Simon Burge <[email protected]> wrote:
> 
> Emile `iMil' Heitor wrote:
> 
>> com_attach_subr() in sys/dev/ic/com.c has a delay(10000) waiting for
>> output to finish, this is very unlikely to be useful inside a virtual
>> machine and slows down boot time in such machine.
>> I propose the following approach:
> 
> Expanding on Martin's suggestion, I wonder if something like this
> is better (completely untested!)?  This way we'd always have
> inside_vm_guest() available to remove the #ifdef check from any
> consumers and reduce clutter a little bit.

I have slightly broader concerns. :-)

> diff --git a/sys/arch/x86/include/cpu.h b/sys/arch/x86/include/cpu.h
> index 6c26dc624b1..94c99946bc9 100644
> --- a/sys/arch/x86/include/cpu.h
> +++ b/sys/arch/x86/include/cpu.h
> @@ -558,6 +558,12 @@ vm_guest_is_pvh(void)
>         }
>  }
> 
> +#define __HAVE_VIRTUAL_HOST_P

First, kind of hand-waving the com(4)-specific parts here, but, the style we generally have lacks the _P for the __HAVE_* macros.  For example:

- __HAVE_ATOMIC_OPERATIONS
- __HAVE_SYSCALL_INTERN
- __HAVE_FAST_SOFTINTS

etc.

> +static __inline bool __unused
> +inside_vm_guest(void)
> +{
> +       return vm_guest != VM_GUEST_NO;
> +}
> +
>  /* cpu_topology.c */
>  void   x86_cpu_topology(struct cpu_info *);
> diff --git a/sys/dev/ic/com.c b/sys/dev/ic/com.c
> index 539b6597accd..f835e493184c 100644
> --- a/sys/dev/ic/com.c
> +++ b/sys/dev/ic/com.c
> @@ -589,8 +589,15 @@ com_attach_subr(struct com_softc *sc)
>                        break;
>                }
> 
> +               if (!inside_vm_guest()) {
> +                       /*
> +                        * Wait for output to finish.  No need for
> +                        * a delay on virtual machines.
> +                        */
> +                       delay(10000);

As for the com(4)-specific bit: why are we inserting a blanket 10ms delay *at all*?  If we’re concerned about garbling output-in-progress on real hardware, isn’t there some register we can look at, or something?

> +               }
> +
>                /* Make sure the console is always "hardwired". */
> -               delay(10000);                   /* wait for output to finish */
>                if (is_console) {
>                        SET(sc->sc_hwflags, COM_HW_CONSOLE);
>                }
> diff --git a/sys/sys/cpu.h b/sys/sys/cpu.h
> index 0393f9c0058a..3f8a0d1ea684 100644
> --- a/sys/sys/cpu.h
> +++ b/sys/sys/cpu.h
> @@ -50,6 +50,14 @@ void cpu_idle(void);
> #endif
> #endif
> 
> +#ifndef __HAVE_VIRTUAL_HOST_P
> +static __inline bool __unused
> +inside_vm_guest(void)
> +{
> +       return 0;

<pedant>Please return false here if you’re going to return a bool type.</pedant> :-)

> +}
> +#endif /* !__HAVE_VIRTUAL_HOST_P */

In general, I don’t object to having something like inside_vm_guest(), but I think we need to maybe put some definition around what returning “true” allows.

-- thorpej