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

On Wed, Feb 05, 2025 at 09:34:21AM -0800, Jason Thorpe wrote:
> 
> > 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?

I had the same question.  Now I have looked at the code.  I still have
the same question. :-)

I guess that it has something to do with letting the Tx FIFO empty before
(potentially) reconfiguring it?

It looks like some of the FIFOs are 64 bytes long.  Supposing a modest
bitrate of 9600 bps, it seems like it will take about 50 milliseconds to
empty a full FIFO.  At 300 bps, more than 1.5 seconds.

David

-- 
David Young
[email protected]    Urbana, IL    (217) 721-9981