RFC: Plan for new "hwmatch" command

RFC: Plan for new "hwmatch" command

[Evan Broder]

Hi -
   Based on some off-list discussion, I'd like to try a different
angle for the Lua patches I submitted a week or two ago. For context,
Ubuntu is interested in setting gfxpayload=keep as often as we can in
the next release [1]. Since gfxpayload=keep doesn't work with all
hardware/driver combinations, we need a way to selectively turn it on,
based on a whitelist or blacklist.

For my first crack at this [2], I used Lua scripting for comparing
current hardware against the whitelist/blacklist. However, I've gotten
feedback that a solution without Lua would be better. Whatever I do,
I'd like it to be with upstream's approval, and ideally adoption, so I
want to have a discussion about interfaces before I start coding.

I'm intentionally opening myself up to bikeshedding, but I'm
interested in people's opinions on the format of the
blacklist/whitelist file. I'm also interested in if this is generic
enough to be a reasonable addition to the GRUB core. And of course I'm
interested to know if people think I'm going about this all wrong.

As a strawman, I propose adding the "hwmatch" command:

  Usage: hwmatch MATCHLIST [BASECLASS]

MATCHLIST is a file containing a list of hardware identifiers.
BASECLASS is a PCI base class code. If specified, only PCI devices
with that base class will be checked. hwmatch returns 0 if any PCI
device in the system is listed in MATCHLIST, and 1 otherwise.

MATCHLIST has one device per line with the following space-separated
fields: base class, subclass, vendor ID, device ID, subsystem vendor
ID, subsystem device ID. The first two fields are 2 hex digits; the
other fields are 4 hex digits. Any field can be replaced by a single
'*' to indicate a wildcard.

Thanks in advance,
 - Evan

[1] https://blueprints.launchpad.net/ubuntu/+spec/packageselection-foundations-n-grub2-boot-framebuffer
[2] https://code.launchpad.net/~broder/+junk/grub2-extras-lua-enum-pci

Re: RFC: Plan for new "hwmatch" command

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

On 11/18/2010 05:58 AM, Evan Broder wrote:
> Hi -
>    Based on some off-list discussion, I'd like to try a different
> angle for the Lua patches I submitted a week or two ago. For context,
> Ubuntu is interested in setting gfxpayload=keep as often as we can in
> the next release [1]. Since gfxpayload=keep doesn't work with all
> hardware/driver combinations, we need a way to selectively turn it on,
> based on a whitelist or blacklist.
>
> For my first crack at this [2], I used Lua scripting for comparing
> current hardware against the whitelist/blacklist. However, I've gotten
> feedback that a solution without Lua would be better. Whatever I do,
> I'd like it to be with upstream's approval, and ideally adoption, so I
> want to have a discussion about interfaces before I start coding.
>
> I'm intentionally opening myself up to bikeshedding, but I'm
> interested in people's opinions on the format of the
> blacklist/whitelist file. I'm also interested in if this is generic
> enough to be a reasonable addition to the GRUB core. And of course I'm
> interested to know if people think I'm going about this all wrong.
>
> As a strawman, I propose adding the "hwmatch" command:
>
>   Usage: hwmatch MATCHLIST [BASECLASS]
>
> MATCHLIST is a file containing a list of hardware identifiers.
> BASECLASS is a PCI base class code. If specified, only PCI devices
> with that base class will be checked. hwmatch returns 0 if any PCI
> device in the system is listed in MATCHLIST, and 1 otherwise.
>
> MATCHLIST has one device per line with the following space-separated
> fields: base class, subclass, vendor ID, device ID, subsystem vendor
> ID, subsystem device ID. The first two fields are 2 hex digits; the
> other fields are 4 hex digits. Any field can be replaced by a single
> '*' to indicate a wildcard.
>   
This has a problem of multiple matches in both white and black list. To
disambigute such thing you would need some logic. Also having a command
dedicated to this seems ad-hoc. I think something more along the lines
of extending scripting is better. So you'd have something like:
iterate_pci {
   classid=$1
   vendorid=$2
   deviceid=$3
   if [ x$classid != x<CLASS> ]; then
       continue
   fi
   case x$vendorid
      <VEN1>) .... ;;
      <VEN2>) .... ;;
   esac
}
I'd recommend to have this code in a separate hwconfig.cfg for convenience.
We already have the necessary infrastructure to implement iterate_pci
without touching the scripting code. "continue" though will need a bit
of extension to work in iterate_pci. "case" isn't implemented yet but we
already have pattern matching, the main issue is the arrival of new
terminal ";;" and proper handling of it. Some logic with && and || would
come in handy too.
> Thanks in advance,
>  - Evan
>
> [1] https://blueprints.launchpad.net/ubuntu/+spec/packageselection-foundations-n-grub2-boot-framebuffer
> [2] https://code.launchpad.net/~broder/+junk/grub2-extras-lua-enum-pci
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: RFC: Plan for new "hwmatch" command

[Colin Watson]

On Thu, Nov 18, 2010 at 12:32:20PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 11/18/2010 05:58 AM, Evan Broder wrote:
> > As a strawman, I propose adding the "hwmatch" command:
> >
> >   Usage: hwmatch MATCHLIST [BASECLASS]
> >
> > MATCHLIST is a file containing a list of hardware identifiers.
> > BASECLASS is a PCI base class code. If specified, only PCI devices
> > with that base class will be checked. hwmatch returns 0 if any PCI
> > device in the system is listed in MATCHLIST, and 1 otherwise.
> >
> > MATCHLIST has one device per line with the following space-separated
> > fields: base class, subclass, vendor ID, device ID, subsystem vendor
> > ID, subsystem device ID. The first two fields are 2 hex digits; the
> > other fields are 4 hex digits. Any field can be replaced by a single
> > '*' to indicate a wildcard.
> 
> This has a problem of multiple matches in both white and black list. To
> disambigute such thing you would need some logic.

I hadn't been expecting to have both a whitelist and blacklist in
existence at once.  That said, thinking about it, I can imagine that we
might want to say something like "all Intel cards, except for these
particular devices".

> Also having a command dedicated to this seems ad-hoc. I think
> something more along the lines of extending scripting is better. So
> you'd have something like:
> iterate_pci {
>    classid=$1
>    vendorid=$2
>    deviceid=$3
>    if [ x$classid != x<CLASS> ]; then
>        continue
>    fi
>    case x$vendorid
>       <VEN1>) .... ;;
>       <VEN2>) .... ;;
>    esac
> }
> I'd recommend to have this code in a separate hwconfig.cfg for convenience.
> We already have the necessary infrastructure to implement iterate_pci
> without touching the scripting code. "continue" though will need a bit
> of extension to work in iterate_pci. "case" isn't implemented yet but we
> already have pattern matching, the main issue is the arrival of new
> terminal ";;" and proper handling of it. Some logic with && and || would
> come in handy too.

I agree with you to some extent that a new command seems rather ad-hoc.
On the other hand, I suspect that the configuration might grow rather
large, and I'm concerned about performance; it doesn't seem to me that
implementing this entirely in code would be likely to be very fast.  If
I were doing this in a POSIX shell script, I wouldn't use a giant case
statement.

Maybe it would be worth having a 'grep' command?  You could probably
implement this as a fairly large regex assuming that you didn't mind the
wildcards being a little inflexible (you'd only be able to use a
wildcard for an entire field, not for substrings).  Then you'd have a
logic file plus a data file that it searches, and you wouldn't have to
put lengthy strings of data in the logic file.

It might be worth prototyping this in POSIX shell with fairly minimal
utilities, to see which utilities we would need to add to GRUB to make
this possible.

(Of course, there's still the option of carrying this as an Ubuntu
patch, and advertising it as having an unstable interface.  I'd rather
avoid that if we can figure out a good upstream design for this,
though.)

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: RFC: Plan for new "hwmatch" command

[Brendan Trotter]

Hi,

On Thu, Nov 18, 2010 at 3:28 PM, Evan Broder <evan@ebroder.net> wrote:
>   Based on some off-list discussion, I'd like to try a different
> angle for the Lua patches I submitted a week or two ago. For context,
> Ubuntu is interested in setting gfxpayload=keep as often as we can in
> the next release [1]. Since gfxpayload=keep doesn't work with all
> hardware/driver combinations, we need a way to selectively turn it on,
> based on a whitelist or blacklist.

I'm curious why setting gfxpayload=keep doesn't work with all
hardware/driver combinations; and by extension, also wondering if
Ubuntu is relying on extending an already over-complicated generic
boot loader (GRUB) to hide symptoms of problems that exist elsewhere
(rather than fixing the problem/s that exist elsewhere, and maybe not
setting gfxpayload=keep until those problems are fixed).

Mostly, I'm wondering where GRUB's responsibilities end and OS
responsibilities start, and how this can effect the GRUB developer's
ability to maintain GRUB in the upcoming months/years/decades; given
that it's easy to add features but almost impossible to remove them.


Cheers,

Brendan

Re: RFC: Plan for new "hwmatch" command

[Evan Broder]

On Thu, Nov 18, 2010 at 7:52 PM, Brendan Trotter <btrotter@gmail.com> wrote:
> Hi,
>
> On Thu, Nov 18, 2010 at 3:28 PM, Evan Broder <evan@ebroder.net> wrote:
>>   Based on some off-list discussion, I'd like to try a different
>> angle for the Lua patches I submitted a week or two ago. For context,
>> Ubuntu is interested in setting gfxpayload=keep as often as we can in
>> the next release [1]. Since gfxpayload=keep doesn't work with all
>> hardware/driver combinations, we need a way to selectively turn it on,
>> based on a whitelist or blacklist.
>
> I'm curious why setting gfxpayload=keep doesn't work with all
> hardware/driver combinations; and by extension, also wondering if
> Ubuntu is relying on extending an already over-complicated generic
> boot loader (GRUB) to hide symptoms of problems that exist elsewhere
> (rather than fixing the problem/s that exist elsewhere, and maybe not
> setting gfxpayload=keep until those problems are fixed).
>
> Mostly, I'm wondering where GRUB's responsibilities end and OS
> responsibilities start, and how this can effect the GRUB developer's
> ability to maintain GRUB in the upcoming months/years/decades; given
> that it's easy to add features but almost impossible to remove them.

I can certainly try to give a bit more context on Ubuntu's perspective:

In the last release development cycle, we experimented with setting
gfxpayoad=keep. As I recall, we found that some drivers, which are
able to initialize the hardware when it's in text mode, are unable to
initialize the hardware when it's in graphics mode. This results in
the X environment failing to initialize (not sure whether that's
"crash" or just "invisible" - or some of each). I know there were bugs
involving the radeon driver (i.e. open-source ATI) [1], the nvidia
driver (i.e. closed source NVIDIA) [2], and the VMware guest video
driver [3], and those were just the bugs I was able to track down
easily [4].

We're certainly planning to work to get these drivers improved, but it
seems unlikely that we can guarantee that gfxpayload=keep will work
with every piece of hardware running GRUB and Ubuntu - certainly not
by April, if ever. However, it's considered a comparatively
high-priority goal for this release to set gfxpayload=keep when we're
confident that the hardware we're running on supports it. Based on the
impression I've gotten, it's high enough priority that we're willing
to carry Ubuntu-specific patches to make this happen, but of course
it's always preferable to build these sorts of things by coordinating
with upstream.

[1] https://bugs.launchpad.net/bugs/605614
[2] https://bugs.launchpad.net/bugs/612626
[3] https://bugs.launchpad.net/bugs/608429
[4] Yes, the thought has occurred to me that I've just eliminated 2 of
the 3 primary graphics hardware manufacturers with that sentence,
although I'm hoping that the damage is more limited than having to
eliminate entire manufacturers at a time.

- Evan

Re: RFC: Plan for new "hwmatch" command

[Colin Watson]

On Wed, Nov 17, 2010 at 08:58:43PM -0800, Evan Broder wrote:
>    Based on some off-list discussion, I'd like to try a different
> angle for the Lua patches I submitted a week or two ago. For context,
> Ubuntu is interested in setting gfxpayload=keep as often as we can in
> the next release [1]. Since gfxpayload=keep doesn't work with all
> hardware/driver combinations, we need a way to selectively turn it on,
> based on a whitelist or blacklist.

This is an implementation of Evan's original proposal in C.  I realise
that there was feedback on the list
(http://lists.gnu.org/archive/html/grub-devel/2010-11/msg00038.html)
that I haven't incorporated; my initial priority was solely to avoid
needing to ship the Lua grub-extras module in Ubuntu 11.04, since we had
feedback from Robert Millan among others that this would be undesirable.

I rather expect that any upstreamable implementation of this would want
to be built on more than just i386_pc, too.

But in case anyone is interested in using this as a starting point:

=== modified file 'grub-core/Makefile.core.def'
--- grub-core/Makefile.core.def	2011-03-26 11:59:02 +0000
+++ grub-core/Makefile.core.def	2011-03-29 15:10:11 +0000
@@ -595,6 +595,15 @@ module = {
 };
 
 module = {
+  name = hwmatch;
+  i386_pc = commands/i386/pc/hwmatch.c;
+  enable = i386_pc;
+  ldadd = libgnulib.a;
+  cflags = '$(CFLAGS_POSIX) $(CFLAGS_GNULIB)';
+  cppflags = '$(CPPFLAGS_POSIX) $(CPPFLAGS_GNULIB)';
+};
+
+module = {
   name = keystatus;
   common = commands/keystatus.c;
 };

=== added file 'grub-core/commands/i386/pc/hwmatch.c'
--- grub-core/commands/i386/pc/hwmatch.c	1970-01-01 00:00:00 +0000
+++ grub-core/commands/i386/pc/hwmatch.c	2011-03-29 20:29:07 +0000
@@ -0,0 +1,139 @@
+/* hwmatch.c - Match hardware against a whitelist/blacklist.  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2011  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/dl.h>
+#include <grub/misc.h>
+#include <grub/command.h>
+#include <grub/pci.h>
+#include <grub/normal.h>
+#include <grub/file.h>
+#include <grub/env.h>
+#include <grub/i18n.h>
+#include <regex.h>
+
+static grub_err_t
+grub_cmd_hwmatch (grub_command_t cmd __attribute__ ((unused)),
+		  int argc, char **args)
+{
+  grub_file_t matches_file;
+  int class_match;
+  int match = 0;
+  char *match_str;
+
+  auto int NESTED_FUNC_ATTR hwmatch_iter (grub_pci_device_t dev,
+					  grub_pci_id_t pciid);
+
+  int NESTED_FUNC_ATTR hwmatch_iter (grub_pci_device_t dev,
+				     grub_pci_id_t pciid)
+  {
+    grub_pci_address_t addr;
+    grub_uint32_t class, baseclass, vendor, device;
+    grub_pci_id_t subpciid;
+    grub_uint32_t subvendor, subdevice, subclass;
+    char *id, *line;
+
+    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+    class = grub_pci_read (addr);
+    baseclass = class >> 24;
+
+    if (class_match != baseclass)
+      return 0;
+
+    vendor = pciid & 0xffff;
+    device = pciid >> 16;
+
+    addr = grub_pci_make_address (dev, GRUB_PCI_REG_SUBVENDOR);
+    subpciid = grub_pci_read (addr);
+
+    subclass = (class >> 16) & 0xff;
+    subvendor = subpciid & 0xffff;
+    subdevice = subpciid >> 16;
+
+    id = grub_xasprintf ("v%04xd%04xsv%04xsd%04xbc%02xsc%02x",
+			 vendor, device, subvendor, subdevice,
+			 baseclass, subclass);
+
+    grub_file_seek (matches_file, 0);
+    while ((line = grub_file_getline (matches_file)) != NULL)
+      {
+	char *anchored_line;
+	regex_t regex;
+	int ret;
+
+	if (! *line || *line == '#')
+	  {
+	    grub_free (line);
+	    continue;
+	  }
+
+	anchored_line = grub_xasprintf ("^%s$", line);
+	ret = regcomp (&regex, anchored_line, REG_EXTENDED | REG_NOSUB);
+	grub_free (anchored_line);
+	if (ret)
+	  {
+	    grub_free (line);
+	    continue;
+	  }
+
+	ret = regexec (&regex, id, 0, NULL, 0);
+	regfree (&regex);
+	grub_free (line);
+	if (! ret)
+	  {
+	    match = 1;
+	    return 1;
+	  }
+      }
+
+    return 0;
+  }
+
+  if (argc < 2)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "list file and class required");
+
+  matches_file = grub_file_open (args[0]);
+  if (! matches_file)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "file not found");
+
+  class_match = grub_strtol (args[1], 0, 10);
+
+  grub_pci_iterate (hwmatch_iter);
+
+  match_str = grub_xasprintf ("%d", match);
+  grub_env_set ("match", match_str);
+  grub_free (match_str);
+
+  grub_file_close (matches_file);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_command_t cmd;
+\f
+GRUB_MOD_INIT(hwmatch)
+{
+  cmd = grub_register_command ("hwmatch", grub_cmd_hwmatch,
+			       N_("MATCHES-FILE CLASS"),
+			       N_("Match PCI devices."));
+}
+
+GRUB_MOD_FINI(hwmatch)
+{
+  grub_unregister_command (cmd);
+}

-- 
Colin Watson                                       [cjwatson@ubuntu.com]