[PATCH v2] x86/ucode: Adjust parse_ucode() to match other list handling

Andrew Cooper posted 1 patch 1 month, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20251215153245.2675388-1-andrew.cooper3@citrix.com
docs/misc/xen-command-line.pandoc | 66 ++++++++++++++++++++-----------
xen/arch/x86/cpu/microcode/core.c | 22 +++++++----
2 files changed, 57 insertions(+), 31 deletions(-)
[PATCH v2] x86/ucode: Adjust parse_ucode() to match other list handling
Posted by Andrew Cooper 1 month, 3 weeks ago
parse_ucode() is abnormal compared to similar parsing elsewhere in Xen.

Invert the ucode_mod_forced check with respect to the "scan" and integer
handling, so we can warn the user when we've elected to ignore the parameters,
and yield -EINVAL for any unrecognised list element.

Rewrite the ucode= command line docs for clarity.

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase by a year.
 * Explain how to use scan= for EFI.
---
 docs/misc/xen-command-line.pandoc | 66 ++++++++++++++++++++-----------
 xen/arch/x86/cpu/microcode/core.c | 22 +++++++----
 2 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index e92b6d55b556..2b4f80c234e1 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2752,34 +2752,52 @@ performance.
    Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
 
 ### ucode
-> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
+> `= List of [ <integer>, scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
 
     Applicability: x86
     Default: `scan` is selectable via Kconfig, `nmi,digest-check`
 
-Controls for CPU microcode loading. For early loading, this parameter can
-specify how and where to find the microcode update blob. For late loading,
-this parameter specifies if the update happens within a NMI handler.
-
-'integer' specifies the CPU microcode update blob module index. When positive,
-this specifies the n-th module (in the GrUB entry, zero based) to be used
-for updating CPU micrcode. When negative, counting starts at the end of
-the modules in the GrUB entry (so with the blob commonly being last,
-one could specify `ucode=-1`). Note that the value of zero is not valid
-here (entry zero, i.e. the first module, is always the Dom0 kernel
-image). Note further that use of this option has an unspecified effect
-when used with xen.efi (there the concept of modules doesn't exist, and
-the blob gets specified via the `ucode=<filename>` config file/section
-entry; see [EFI configuration file description](efi.html)).
-
-'scan' instructs the hypervisor to scan the multiboot images for an cpio
-image that contains microcode. Depending on the platform the blob with the
-microcode in the cpio name space must be:
-  - on Intel: kernel/x86/microcode/GenuineIntel.bin
-  - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
-When using xen.efi, the `ucode=<filename>` config file setting takes
-precedence over `scan`. The default value for `scan` is set with
-`CONFIG_UCODE_SCAN_DEFAULT`.
+Controls for CPU microcode loading.
+
+In order to load microcode at boot, Xen needs to find a suitable update
+amongst the modules provided by the bootloader.  Two kinds of microcode update
+are supported:
+
+ 1. Raw microcode containers.  The format of the container is CPU vendor
+    specific.
+
+ 2. CPIO archive.  This is Linux's preferred mechanism, and involves having
+    the raw containers expressed as files
+    (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO
+    archive, typically prepended to the initrd.
+
+The `<integer>` and `scan=<bool>` options are mutually exclusive and select
+between these two options.  Further restrictions exist for booting xen.efi
+(see below).
+
+ *  The `<integer>` option nominates a specific multiboot module as a raw
+    container (option 1 above).  Valid options start from 1 (module 0 is
+    always the dom0 kernel).  A negative number may be used, and will
+    back-reference from the end of the module list.  i.e. `ucode=-1` will
+    nominate the final multiboot module.
+
+ *  The `scan=` option causes Xen to search all modules in order to find the
+    first CPIO archive containing the appropriate file (option 2 above).  The
+    default for this option can be chosen at build time via
+    `CONFIG_UCODE_SCAN_DEFAULT`.
+
+When booting xen.efi natively, the concept of multiboot modules doesn't exist.
+Instead:
+
+ *  In the [EFI configuration file](efi.html), `ucode=<filename>` can be used
+    to identify a file as a raw container (option 1 above).  Use of this
+    mechanism will disable both `<integer>` and `scan=`.
+
+ *  If `ucode=<filename>` in the EFI configuration file is not used, it is
+    still possible to use `scan=` to search all modules.  The order of module
+    is undefined, but there is only a single `ramdisk=<filename>`
+    configuration option available.  The use of `<integer>` for xen.efi is
+    always undefined.
 
 'nmi' determines late loading is performed in NMI handler or just in
 stop_machine context. In NMI handler, even NMIs are blocked, which is
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index fe47c3a6c18d..87ab623bf9e6 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -115,11 +115,6 @@ void __init microcode_set_module(unsigned int idx)
     ucode_mod_forced = 1;
 }
 
-/*
- * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi=<bool> is parsed.
- */
 static int __init cf_check parse_ucode(const char *s)
 {
     const char *ss;
@@ -134,13 +129,24 @@ static int __init cf_check parse_ucode(const char *s)
             ucode_in_nmi = val;
         else if ( (val = parse_boolean("digest-check", s, ss)) >= 0 )
             opt_digest_check = val;
-        else if ( !ucode_mod_forced ) /* Not forced by EFI */
+        else if ( (val = parse_boolean("scan", s, ss)) >= 0 )
         {
-            if ( (val = parse_boolean("scan", s, ss)) >= 0 )
+            if ( ucode_mod_forced )
+                printk(XENLOG_WARNING
+                       "Ignoring ucode=%.*s setting; overridden by EFI\n",
+                       (int)(ss - s), s);
+            else
             {
                 opt_scan = val;
                 opt_mod_idx = 0;
             }
+        }
+        else if ( isdigit(s[0]) || s[0] == '-' )
+        {
+            if ( ucode_mod_forced )
+                printk(XENLOG_WARNING
+                       "Ignoring ucode=%.*s setting; overridden by EFI\n",
+                       (int)(ss - s), s);
             else
             {
                 const char *q;
@@ -155,6 +161,8 @@ static int __init cf_check parse_ucode(const char *s)
                     opt_scan = false;
             }
         }
+        else
+            rc = -EINVAL;
 
         s = ss + 1;
     } while ( *ss );
-- 
2.39.5


Re: [PATCH v2] x86/ucode: Adjust parse_ucode() to match other list handling
Posted by Jan Beulich 1 month, 3 weeks ago
On 15.12.2025 16:32, Andrew Cooper wrote:
> parse_ucode() is abnormal compared to similar parsing elsewhere in Xen.
> 
> Invert the ucode_mod_forced check with respect to the "scan" and integer
> handling, so we can warn the user when we've elected to ignore the parameters,
> and yield -EINVAL for any unrecognised list element.
> 
> Rewrite the ucode= command line docs for clarity.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
albeit I'm not quite happy with ...

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2752,34 +2752,52 @@ performance.
>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
>  
>  ### ucode
> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
> +> `= List of [ <integer>, scan=<bool>, nmi=<bool>, digest-check=<bool> ]`

... this change when ...

>      Applicability: x86
>      Default: `scan` is selectable via Kconfig, `nmi,digest-check`
>  
> -Controls for CPU microcode loading. For early loading, this parameter can
> -specify how and where to find the microcode update blob. For late loading,
> -this parameter specifies if the update happens within a NMI handler.
> -
> -'integer' specifies the CPU microcode update blob module index. When positive,
> -this specifies the n-th module (in the GrUB entry, zero based) to be used
> -for updating CPU micrcode. When negative, counting starts at the end of
> -the modules in the GrUB entry (so with the blob commonly being last,
> -one could specify `ucode=-1`). Note that the value of zero is not valid
> -here (entry zero, i.e. the first module, is always the Dom0 kernel
> -image). Note further that use of this option has an unspecified effect
> -when used with xen.efi (there the concept of modules doesn't exist, and
> -the blob gets specified via the `ucode=<filename>` config file/section
> -entry; see [EFI configuration file description](efi.html)).
> -
> -'scan' instructs the hypervisor to scan the multiboot images for an cpio
> -image that contains microcode. Depending on the platform the blob with the
> -microcode in the cpio name space must be:
> -  - on Intel: kernel/x86/microcode/GenuineIntel.bin
> -  - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
> -When using xen.efi, the `ucode=<filename>` config file setting takes
> -precedence over `scan`. The default value for `scan` is set with
> -`CONFIG_UCODE_SCAN_DEFAULT`.
> +Controls for CPU microcode loading.
> +
> +In order to load microcode at boot, Xen needs to find a suitable update
> +amongst the modules provided by the bootloader.  Two kinds of microcode update
> +are supported:
> +
> + 1. Raw microcode containers.  The format of the container is CPU vendor
> +    specific.
> +
> + 2. CPIO archive.  This is Linux's preferred mechanism, and involves having
> +    the raw containers expressed as files
> +    (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO
> +    archive, typically prepended to the initrd.
> +
> +The `<integer>` and `scan=<bool>` options are mutually exclusive and select
> +between these two options.  Further restrictions exist for booting xen.efi
> +(see below).

... then you say verbally that the two are exclusive of one another. That's
what the | there was intended to indicate. IOW I would prefer that line to
be left alone, but I'm not intending to insist.

Jan
Re: [PATCH v2] x86/ucode: Adjust parse_ucode() to match other list handling
Posted by Andrew Cooper 1 month, 3 weeks ago
On 15/12/2025 5:00 pm, Jan Beulich wrote:
> On 15.12.2025 16:32, Andrew Cooper wrote:
>> parse_ucode() is abnormal compared to similar parsing elsewhere in Xen.
>>
>> Invert the ucode_mod_forced check with respect to the "scan" and integer
>> handling, so we can warn the user when we've elected to ignore the parameters,
>> and yield -EINVAL for any unrecognised list element.
>>
>> Rewrite the ucode= command line docs for clarity.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> albeit I'm not quite happy with ...
>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -2752,34 +2752,52 @@ performance.
>>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
>>  
>>  ### ucode
>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>> +> `= List of [ <integer>, scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
> ... this change when ...
>
>>      Applicability: x86
>>      Default: `scan` is selectable via Kconfig, `nmi,digest-check`
>>  
>> -Controls for CPU microcode loading. For early loading, this parameter can
>> -specify how and where to find the microcode update blob. For late loading,
>> -this parameter specifies if the update happens within a NMI handler.
>> -
>> -'integer' specifies the CPU microcode update blob module index. When positive,
>> -this specifies the n-th module (in the GrUB entry, zero based) to be used
>> -for updating CPU micrcode. When negative, counting starts at the end of
>> -the modules in the GrUB entry (so with the blob commonly being last,
>> -one could specify `ucode=-1`). Note that the value of zero is not valid
>> -here (entry zero, i.e. the first module, is always the Dom0 kernel
>> -image). Note further that use of this option has an unspecified effect
>> -when used with xen.efi (there the concept of modules doesn't exist, and
>> -the blob gets specified via the `ucode=<filename>` config file/section
>> -entry; see [EFI configuration file description](efi.html)).
>> -
>> -'scan' instructs the hypervisor to scan the multiboot images for an cpio
>> -image that contains microcode. Depending on the platform the blob with the
>> -microcode in the cpio name space must be:
>> -  - on Intel: kernel/x86/microcode/GenuineIntel.bin
>> -  - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
>> -When using xen.efi, the `ucode=<filename>` config file setting takes
>> -precedence over `scan`. The default value for `scan` is set with
>> -`CONFIG_UCODE_SCAN_DEFAULT`.
>> +Controls for CPU microcode loading.
>> +
>> +In order to load microcode at boot, Xen needs to find a suitable update
>> +amongst the modules provided by the bootloader.  Two kinds of microcode update
>> +are supported:
>> +
>> + 1. Raw microcode containers.  The format of the container is CPU vendor
>> +    specific.
>> +
>> + 2. CPIO archive.  This is Linux's preferred mechanism, and involves having
>> +    the raw containers expressed as files
>> +    (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO
>> +    archive, typically prepended to the initrd.
>> +
>> +The `<integer>` and `scan=<bool>` options are mutually exclusive and select
>> +between these two options.  Further restrictions exist for booting xen.efi
>> +(see below).
> ... then you say verbally that the two are exclusive of one another. That's
> what the | there was intended to indicate. IOW I would prefer that line to
> be left alone, but I'm not intending to insist.

You said that last time around, but it's still not how the parsing works.

ucode=1,1,1,scan,scan,scan,2 is legal.  The latest takes priority and
cancels prior selections.

The reality is that | used in this context is meaningless when there's a
comma separated loop around the whole thing.

If you don't like "mutually exclusive", what else do you suggest?

~Andrew

Re: [PATCH v2] x86/ucode: Adjust parse_ucode() to match other list handling
Posted by Jan Beulich 1 month, 3 weeks ago
On 15.12.2025 18:08, Andrew Cooper wrote:
> On 15/12/2025 5:00 pm, Jan Beulich wrote:
>> On 15.12.2025 16:32, Andrew Cooper wrote:
>>> parse_ucode() is abnormal compared to similar parsing elsewhere in Xen.
>>>
>>> Invert the ucode_mod_forced check with respect to the "scan" and integer
>>> handling, so we can warn the user when we've elected to ignore the parameters,
>>> and yield -EINVAL for any unrecognised list element.
>>>
>>> Rewrite the ucode= command line docs for clarity.
>>>
>>> No practical change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> albeit I'm not quite happy with ...
>>
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -2752,34 +2752,52 @@ performance.
>>>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
>>>  
>>>  ### ucode
>>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>>> +> `= List of [ <integer>, scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>> ... this change when ...
>>
>>>      Applicability: x86
>>>      Default: `scan` is selectable via Kconfig, `nmi,digest-check`
>>>  
>>> -Controls for CPU microcode loading. For early loading, this parameter can
>>> -specify how and where to find the microcode update blob. For late loading,
>>> -this parameter specifies if the update happens within a NMI handler.
>>> -
>>> -'integer' specifies the CPU microcode update blob module index. When positive,
>>> -this specifies the n-th module (in the GrUB entry, zero based) to be used
>>> -for updating CPU micrcode. When negative, counting starts at the end of
>>> -the modules in the GrUB entry (so with the blob commonly being last,
>>> -one could specify `ucode=-1`). Note that the value of zero is not valid
>>> -here (entry zero, i.e. the first module, is always the Dom0 kernel
>>> -image). Note further that use of this option has an unspecified effect
>>> -when used with xen.efi (there the concept of modules doesn't exist, and
>>> -the blob gets specified via the `ucode=<filename>` config file/section
>>> -entry; see [EFI configuration file description](efi.html)).
>>> -
>>> -'scan' instructs the hypervisor to scan the multiboot images for an cpio
>>> -image that contains microcode. Depending on the platform the blob with the
>>> -microcode in the cpio name space must be:
>>> -  - on Intel: kernel/x86/microcode/GenuineIntel.bin
>>> -  - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
>>> -When using xen.efi, the `ucode=<filename>` config file setting takes
>>> -precedence over `scan`. The default value for `scan` is set with
>>> -`CONFIG_UCODE_SCAN_DEFAULT`.
>>> +Controls for CPU microcode loading.
>>> +
>>> +In order to load microcode at boot, Xen needs to find a suitable update
>>> +amongst the modules provided by the bootloader.  Two kinds of microcode update
>>> +are supported:
>>> +
>>> + 1. Raw microcode containers.  The format of the container is CPU vendor
>>> +    specific.
>>> +
>>> + 2. CPIO archive.  This is Linux's preferred mechanism, and involves having
>>> +    the raw containers expressed as files
>>> +    (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO
>>> +    archive, typically prepended to the initrd.
>>> +
>>> +The `<integer>` and `scan=<bool>` options are mutually exclusive and select
>>> +between these two options.  Further restrictions exist for booting xen.efi
>>> +(see below).
>> ... then you say verbally that the two are exclusive of one another. That's
>> what the | there was intended to indicate. IOW I would prefer that line to
>> be left alone, but I'm not intending to insist.
> 
> You said that last time around, but it's still not how the parsing works.
> 
> ucode=1,1,1,scan,scan,scan,2 is legal.  The latest takes priority and
> cancels prior selections.
> 
> The reality is that | used in this context is meaningless when there's a
> comma separated loop around the whole thing.
> 
> If you don't like "mutually exclusive", what else do you suggest?

I'm happy with mutually exclusive. What I said I don't like is the dropping
of the |, expressing the same "mutually exclusive" in a non-verbal way. Imo
those short forms aren't supposed to describe how parsing works, but how the
options are intended to be used.

Jan

Re: [PATCH v2] x86/ucode: Adjust parse_ucode() to match other list handling
Posted by Andrew Cooper 1 week, 5 days ago
On 16/12/2025 7:55 am, Jan Beulich wrote:
> On 15.12.2025 18:08, Andrew Cooper wrote:
>> On 15/12/2025 5:00 pm, Jan Beulich wrote:
>>> On 15.12.2025 16:32, Andrew Cooper wrote:
>>>> parse_ucode() is abnormal compared to similar parsing elsewhere in Xen.
>>>>
>>>> Invert the ucode_mod_forced check with respect to the "scan" and integer
>>>> handling, so we can warn the user when we've elected to ignore the parameters,
>>>> and yield -EINVAL for any unrecognised list element.
>>>>
>>>> Rewrite the ucode= command line docs for clarity.
>>>>
>>>> No practical change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> albeit I'm not quite happy with ...
>>>
>>>> --- a/docs/misc/xen-command-line.pandoc
>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>> @@ -2752,34 +2752,52 @@ performance.
>>>>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
>>>>  
>>>>  ### ucode
>>>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>>>> +> `= List of [ <integer>, scan=<bool>, nmi=<bool>, digest-check=<bool> ]`
>>> ... this change when ...
>>>
>>>>      Applicability: x86
>>>>      Default: `scan` is selectable via Kconfig, `nmi,digest-check`
>>>>  
>>>> -Controls for CPU microcode loading. For early loading, this parameter can
>>>> -specify how and where to find the microcode update blob. For late loading,
>>>> -this parameter specifies if the update happens within a NMI handler.
>>>> -
>>>> -'integer' specifies the CPU microcode update blob module index. When positive,
>>>> -this specifies the n-th module (in the GrUB entry, zero based) to be used
>>>> -for updating CPU micrcode. When negative, counting starts at the end of
>>>> -the modules in the GrUB entry (so with the blob commonly being last,
>>>> -one could specify `ucode=-1`). Note that the value of zero is not valid
>>>> -here (entry zero, i.e. the first module, is always the Dom0 kernel
>>>> -image). Note further that use of this option has an unspecified effect
>>>> -when used with xen.efi (there the concept of modules doesn't exist, and
>>>> -the blob gets specified via the `ucode=<filename>` config file/section
>>>> -entry; see [EFI configuration file description](efi.html)).
>>>> -
>>>> -'scan' instructs the hypervisor to scan the multiboot images for an cpio
>>>> -image that contains microcode. Depending on the platform the blob with the
>>>> -microcode in the cpio name space must be:
>>>> -  - on Intel: kernel/x86/microcode/GenuineIntel.bin
>>>> -  - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
>>>> -When using xen.efi, the `ucode=<filename>` config file setting takes
>>>> -precedence over `scan`. The default value for `scan` is set with
>>>> -`CONFIG_UCODE_SCAN_DEFAULT`.
>>>> +Controls for CPU microcode loading.
>>>> +
>>>> +In order to load microcode at boot, Xen needs to find a suitable update
>>>> +amongst the modules provided by the bootloader.  Two kinds of microcode update
>>>> +are supported:
>>>> +
>>>> + 1. Raw microcode containers.  The format of the container is CPU vendor
>>>> +    specific.
>>>> +
>>>> + 2. CPIO archive.  This is Linux's preferred mechanism, and involves having
>>>> +    the raw containers expressed as files
>>>> +    (e.g. `kernel/x86/microcode/{GenuineIntel,AuthenticAMD}.bin`) in a CPIO
>>>> +    archive, typically prepended to the initrd.
>>>> +
>>>> +The `<integer>` and `scan=<bool>` options are mutually exclusive and select
>>>> +between these two options.  Further restrictions exist for booting xen.efi
>>>> +(see below).
>>> ... then you say verbally that the two are exclusive of one another. That's
>>> what the | there was intended to indicate. IOW I would prefer that line to
>>> be left alone, but I'm not intending to insist.
>> You said that last time around, but it's still not how the parsing works.
>>
>> ucode=1,1,1,scan,scan,scan,2 is legal.  The latest takes priority and
>> cancels prior selections.
>>
>> The reality is that | used in this context is meaningless when there's a
>> comma separated loop around the whole thing.
>>
>> If you don't like "mutually exclusive", what else do you suggest?
> I'm happy with mutually exclusive. What I said I don't like is the dropping
> of the |, expressing the same "mutually exclusive" in a non-verbal way. Imo
> those short forms aren't supposed to describe how parsing works, but how the
> options are intended to be used.

The documentation needs to describe how it is, not how one wants it to be.

Anyway, this is long overdue (and needs another rebase over Alejandro's
recent work), so I'm going to get it committed.

~Andrew