Re: [PATCH] term/serial: Add support for PCI serial devices

[Glenn Washburn]

On Thu, 25 Aug 2022 21:59:01 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Aug 25, 2022 at 02:23:34PM -0500, Glenn Washburn wrote:
> 
> > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > index 5212dfab1369..c0683da2353b 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -224,12 +224,14 @@ kernel = {
> > >  
> > >    i386_efi = kern/i386/efi/init.c;
> > >    i386_efi = bus/pci.c;
> > > +  i386_efi_cppflags = '-DGRUB_HAS_PCI';
> > 
> > I imagine this should be enabled for all i386 and x86_64 targets, not
> > just the few that you have here. For instance, what about i386-pc,
> > which is the 32-bit BIOS target?
> 
> So what I did was put that _cppflags thing right next to all the
> 'bus/pci.c' instances I found, with exception of the pci module.
> 
> Where do i386-pc and friends get it from? IOW, what obvious place did I
> miss?

The main missed place is in the definition of the serial module in
Makefile.core.def. That module will not be built with -DGRUB_HAS_PCI
even on tagets, like i386-efi, in which you added -DGRUB_HAS_PCI in the
kernel. Because the CPPFLAGS for the kernel don't get used for the
modules.

Looking at this more and its worse than I thought. What we want is
for all pci enabled builds to get -DGRUB_HAS_PCI. You just added it for
C/C++ files used when building the kernel, but it should happen for all
C/C++ files on targets that GRUB supports PCI. It doesn't appear there
is a good way to do this right now.

So I've created a patch that I think is the RightWay(TM) to do it (see
patch titled "[PATCH] configure: Add -DGRUB_HAS_PCI when compiling
C/C++ files on targets that support PCI"). Using this patch you should
be able to use GRUB_HAS_PCI in the C files. The next patch version
should not include -DGRUB_HAS_PCI in the Makefile.core.def.

Also, the added lines:

  x86 = term/pci/serial.c;
  mips_loongson = term/pci/serial.c;

Should be instead be one line:

  pci = term/pci/serial.c;

Let me know if something doesn't work.

Glenn