On Mon, Jan 31, 2011 at 04:15:21PM -0800, Jeremy Fitzhardinge wrote:
> My understanding of what you're proposing is:
>
> 1. the normal CPU-specific microcode driver starts up
> 2. an if (xen) test overrides the apply_microcode to call a
> Xen-specific function
> 3. the driver requests the firmware, loads and verifies it as normal
> 4. the microcode is applied via the Xen apply_microcode function
>
> With (2) needing a Xen-specific change to both the Intel and AMD drivers.
>
> If not, what are you proposing in detail?
>
> Are you instead thinking:
>
> * Xen-specific microcode driver starts up
> * It calls either the intel or amd request_microcode_fw as needed
> * apply the the loaded data via hypercall
>
> ?
I am thinking something in the sense of the above. For example, in the
AMD case you take
static struct microcode_ops microcode_amd_ops = {
.request_microcode_user = request_microcode_user,
.request_microcode_fw = request_microcode_fw,
.collect_cpu_info = collect_cpu_info_amd,
.apply_microcode = apply_microcode_amd,
.microcode_fini_cpu = microcode_fini_cpu_amd,
};
and reuse the ->request_microcode_fw, ->collect_cpu_info and
->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
to the point where you need to apply the microcode. Then, you use
your supplied ->apply_microcode hypercall wrapper to call into the
hypervisor.
> But all this is flawed because the microcode_intel/amd.c drivers assume
> they can see all the CPUs present in the system, and load suitable
> microcode for each specific one. But a kernel in a Xen domain only has
> virtual CPUs - not physical ones - and has no idea how to get
> appropriate microcode data for all the physical CPUs in the system.
Well, let me quote you:
On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
> Xen update mechanism is uniform independent of the CPU type, but the
> driver must know where to find the data file, which depends on the CPU
> type. And since the update hypercall updates all CPUs, we only need to
> execute it once on any CPU - but for simplicity it just runs it only
> on (V)CPU 0.
so you only do it once and exit early in the rest of the cases. I
wouldn't worry about performance since ucode is applied only once upon
boot.
[..]
> Right. There's nothing really for the driver to do (kernel or Xen).
> The Intel driver does a bunch of checksum checking, which is presumably
> useful for detecting a corrupted firmware update early, and splits out
> the chunks specific to the current cpus. The AMD driver does no
> checking per-se, but also looks for processor-specific microcode update
> chunks.
>
> In the Xen case, since a domain is not necessarily seeing all the
> details of the underlying physical cpus, it makes most sense for
> usermode to just pass in the whole microcode file and let Xen do all the
> checking/filtering based on complete knowledge of the system.
This is exactly what I'm talking about - why copy all that
checking/filtering code from baremetal to Xen instead of simply reusing
it? Especially if you'd need to update the copy from time to time when
baremetal changes.
> > All I'm saying is, you should try as best as possible to avoid code
> > duplication and the need for replicating functionality to Xen, thus
> > doubling - even multiplying - the effort for coding/testing baremetal
> > and then Xen. Microcode is a perfect example since the vendors do all
> > their testing/verification on baremetal anyway and the rest should
> > benefit from that work.
>
> CPU vendors test Xen, and Intel is particularly interested in getting
> this microcode driver upstream. The amount of duplicated code is
> trivial, and the basic structure of the microcode updates doesn't seem
> set to change.
Uuh, I wouldn't bet on that though :).
> Since Xen has to have all sorts of other CPU-specific code which at
> least somewhat overlaps with what's in the kernel a bit more doesn't
> matter.
Well, I'll let x86 people decide on that but last time I checked they
opposed "if (xen)" sprinkling all over the place.
Btw, hpa has a point, if you can load microcode using multiboot, all
that discussion will become moot since you'll be better at loading
microcode even than baremetal. We need a similar mechanism in x86 too
since the current one loads the microcode definitely too late.
The optimal case for baremetal would be to load it as early as possible
on each CPU's bootstrapping path and if you can do that in the
hypervisor, before even dom0 starts, you're very much fine.
> (It's interesting that nobody seems to have been interested enough in
> Via to have ever implemented a microcode driver for it - perhaps they
> only ever do it from BIOS.)
Yeah, that's one possibility. I bet they'll do it though, when BIOS
cannot be updated/is out of life on some of their platforms and they
want to get a ucode patch applied.
--
Regards/Gruss,
Boris.
On 02/01/2011 03:00 AM, Borislav Petkov wrote:
> I am thinking something in the sense of the above. For example, in the
> AMD case you take
>
> static struct microcode_ops microcode_amd_ops = {
> .request_microcode_user = request_microcode_user,
> .request_microcode_fw = request_microcode_fw,
> .collect_cpu_info = collect_cpu_info_amd,
> .apply_microcode = apply_microcode_amd,
> .microcode_fini_cpu = microcode_fini_cpu_amd,
> };
>
> and reuse the ->request_microcode_fw, ->collect_cpu_info and
> ->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
> to the point where you need to apply the microcode. Then, you use
> your supplied ->apply_microcode hypercall wrapper to call into the
> hypervisor.
collect_cpu_info can't work, because the domain doesn't have access to
all the host's physical CPUs.
However, even aside from that, it means exporting a pile of internal
details from microcode_amd and reusing them within microcode_xen. And
it requires that it be done again for each vendor.
But all that's really needed is a dead simple "request" that loads the
entire file (with a vendor-specific name) and shoves it into Xen.
There's no need for any vendor-specific code beyond the filename.
>> But all this is flawed because the microcode_intel/amd.c drivers assume
>> they can see all the CPUs present in the system, and load suitable
>> microcode for each specific one. But a kernel in a Xen domain only has
>> virtual CPUs - not physical ones - and has no idea how to get
>> appropriate microcode data for all the physical CPUs in the system.
> Well, let me quote you:
>
> On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
>> Xen update mechanism is uniform independent of the CPU type, but the
>> driver must know where to find the data file, which depends on the CPU
>> type. And since the update hypercall updates all CPUs, we only need to
>> execute it once on any CPU - but for simplicity it just runs it only
>> on (V)CPU 0.
> so you only do it once and exit early in the rest of the cases. I
> wouldn't worry about performance since ucode is applied only once upon
> boot.
Its not a performance question. The Intel and AMD microcode drivers
parse the full blob loaded from userspace, and just extract the chunk
needed for each CPU. It does this for each separate CPU, so in
principle you could have a mixture of models within one machine or
something (the driver certainly assumes that could happen; perhaps it
could on a larger multinode machine).
The point is that if it does this on (what the domain sees as ) "cpu 0",
then it may throw away microcode chunks needed for other CPUs. That's
why we need to hand Xen the entire microcode file, and let the
hypervisor do the work of splitting it up and installing it on the CPUs.
> This is exactly what I'm talking about - why copy all that
> checking/filtering code from baremetal to Xen instead of simply reusing
> it? Especially if you'd need to update the copy from time to time when
> baremetal changes.
The code in the kernel is in the wrong place. It has to be done in
Xen. When Xen is present, the code in the kernel is redundant, not the
other way around.
>> CPU vendors test Xen, and Intel is particularly interested in getting
>> this microcode driver upstream. The amount of duplicated code is
>> trivial, and the basic structure of the microcode updates doesn't seem
>> set to change.
> Uuh, I wouldn't bet on that though :).
Shrug. AFAICT the mechanism hasn't changed since it was first
introduced. If there's a change, then both Linux and Xen will have to
change, and most likely the same CPU vendor engineer will provide a
patch for both. Xen has a good record for tracking new CPU features.
>> Since Xen has to have all sorts of other CPU-specific code which at
>> least somewhat overlaps with what's in the kernel a bit more doesn't
>> matter.
> Well, I'll let x86 people decide on that but last time I checked they
> opposed "if (xen)" sprinkling all over the place.
Eh? I'm talking about code within Xen; it doesn't involve any if (xen)s
within the kernel.
> Btw, hpa has a point, if you can load microcode using multiboot, all
> that discussion will become moot since you'll be better at loading
> microcode even than baremetal. We need a similar mechanism in x86 too
> since the current one loads the microcode definitely too late.
>
> The optimal case for baremetal would be to load it as early as possible
> on each CPU's bootstrapping path and if you can do that in the
> hypervisor, before even dom0 starts, you're very much fine.
It is possible, but it requires that vendors install the microcode
updates in /boot and update the grub entries accordingly. I'd prefer a
solution which works with current distros as-is.
J
Ok, I'm reading your answers but I keep getting the impression that this
discussion is not moving anywhere. You keep finding reasons why it can't
be done and trying to persuade me that your way is the only way. Why is
that?
And I'm telling you microcode_xen has nothing to do among
vendor-specific code. Since when is xen a hw vendor and deserves special
attention? And don't tell me because people use it. It is absolutely
inacceptable to add a bunch of code to arch/x86 just because you're
telling me it can't be done differently, not intermixed with hw vendor
code and just because a hypervisor vendor needs it. Does that mean that
if every other virtualization booth wants to add their special code to
arch/x86, we just go and slap it in without questioning its design?
I don't think so.
On Tue, Feb 01, 2011 at 03:12:08PM -0800, Jeremy Fitzhardinge wrote:
> On 02/01/2011 03:00 AM, Borislav Petkov wrote:
> > I am thinking something in the sense of the above. For example, in the
> > AMD case you take
> >
> > static struct microcode_ops microcode_amd_ops = {
> > .request_microcode_user = request_microcode_user,
> > .request_microcode_fw = request_microcode_fw,
> > .collect_cpu_info = collect_cpu_info_amd,
> > .apply_microcode = apply_microcode_amd,
> > .microcode_fini_cpu = microcode_fini_cpu_amd,
> > };
> >
> > and reuse the ->request_microcode_fw, ->collect_cpu_info and
> > ->microcode_fini_cpu on dom0 as if you're running on baremetal. Up
> > to the point where you need to apply the microcode. Then, you use
> > your supplied ->apply_microcode hypercall wrapper to call into the
> > hypervisor.
>
> collect_cpu_info can't work, because the domain doesn't have access to
> all the host's physical CPUs.
Why would you need that? You can safely assume that the ucode patch
level on all cores across the system are identical - I've yet to see a
machine running with different patch levels for the same reason that
mixed silicon systems is a large pain in the ass and you're better off
buying yourself a completely new system.
> However, even aside from that, it means exporting a pile of internal
> details from microcode_amd and reusing them within microcode_xen. And
> it requires that it be done again for each vendor.
Why can't you load the appropriate, unmodified microcode_<vendor> module
in dom0 and let it call the hypercall?
> But all that's really needed is a dead simple "request" that loads the
> entire file (with a vendor-specific name) and shoves it into Xen.
> There's no need for any vendor-specific code beyond the filename.
But that adds this funny chunk
- if (c->x86_vendor == X86_VENDOR_INTEL)
+ if (xen_pv_domain())
+ microcode_ops = init_xen_microcode();
+ else if (c->x86_vendor == X86_VENDOR_INTEL)
microcode_ops = init_intel_microcode();
else if (c->x86_vendor == X86_VENDOR_AMD)
microcode_ops = init_amd_microcode();
which can clearly be avoided. Now imagine the if-else thing contained a
dozen or more virt solutions in there, bloat! Now imagine this not only
in the microcode driver but in a bunch of other places across arch/x86.
> >> But all this is flawed because the microcode_intel/amd.c drivers assume
> >> they can see all the CPUs present in the system, and load suitable
> >> microcode for each specific one. But a kernel in a Xen domain only has
> >> virtual CPUs - not physical ones - and has no idea how to get
> >> appropriate microcode data for all the physical CPUs in the system.
> > Well, let me quote you:
> >
> > On Fri, Jan 28, 2011 at 04:26:52PM -0800, Jeremy Fitzhardinge wrote:
> >> Xen update mechanism is uniform independent of the CPU type, but the
> >> driver must know where to find the data file, which depends on the CPU
> >> type. And since the update hypercall updates all CPUs, we only need to
> >> execute it once on any CPU - but for simplicity it just runs it only
> >> on (V)CPU 0.
> > so you only do it once and exit early in the rest of the cases. I
> > wouldn't worry about performance since ucode is applied only once upon
> > boot.
>
> Its not a performance question. The Intel and AMD microcode drivers
> parse the full blob loaded from userspace, and just extract the chunk
> needed for each CPU. It does this for each separate CPU, so in
> principle you could have a mixture of models within one machine or
> something (the driver certainly assumes that could happen; perhaps it
> could on a larger multinode machine).
>
> The point is that if it does this on (what the domain sees as ) "cpu 0",
> then it may throw away microcode chunks needed for other CPUs. That's
> why we need to hand Xen the entire microcode file, and let the
> hypervisor do the work of splitting it up and installing it on the CPUs.
see comment about mixed silicon above.
> > This is exactly what I'm talking about - why copy all that
> > checking/filtering code from baremetal to Xen instead of simply reusing
> > it? Especially if you'd need to update the copy from time to time when
> > baremetal changes.
>
> The code in the kernel is in the wrong place. It has to be done in
> Xen. When Xen is present, the code in the kernel is redundant, not the
> other way around.
Ok, this says it all. Let's remove the arch code and replace it with
xen-friendly version - we won't need the baremetal one anyway. Jeez!
> >> CPU vendors test Xen, and Intel is particularly interested in getting
> >> this microcode driver upstream. The amount of duplicated code is
> >> trivial, and the basic structure of the microcode updates doesn't seem
> >> set to change.
> > Uuh, I wouldn't bet on that though :).
>
> Shrug. AFAICT the mechanism hasn't changed since it was first
> introduced. If there's a change, then both Linux and Xen will have to
> change, and most likely the same CPU vendor engineer will provide a
> patch for both. Xen has a good record for tracking new CPU features.
I don't think that the same CPU vendor engineer will do that, believe me!
> >> Since Xen has to have all sorts of other CPU-specific code which at
> >> least somewhat overlaps with what's in the kernel a bit more doesn't
> >> matter.
> > Well, I'll let x86 people decide on that but last time I checked they
> > opposed "if (xen)" sprinkling all over the place.
>
> Eh? I'm talking about code within Xen; it doesn't involve any if (xen)s
> within the kernel.
Ok, I'm getting tired of this. I'll gladly read all your answers if you
have constructive suggestions and better proposals - if the only thing
you have to come back are reasons why xen's is the only way to do it,
then don't even bother to answer - at least I won't. If x86 people want
to take your code, they'll do it since it is their call anyway.
Thanks.
--
Regards/Gruss,
Boris.
On Wed, 02 Feb 2011, Borislav Petkov wrote:
> Why would you need that? You can safely assume that the ucode patch
> level on all cores across the system are identical - I've yet to see a
No. You can safely assume that the ucode patch level on all cores on a
package are identical, and that's it. Mixing processors with different
processor flags (and thus potentially different microcode) is not uncommon.
If it boots, it *will* be done, especially later in the product cycle, when
specific spare parts are harder to come by. Sometimes you can even mix
processors of different steppings.
> mixed silicon systems is a large pain in the ass and you're better off
> buying yourself a completely new system.
Well, I have some of them at work.
Mixed CPU steppings or processor flags happen when you expand the system
later to fill in processor packages (reasonably common), or when you replace
a CPU that is flaky (rare). This sort of mixing of different processors is
really common on older Xeon systems, and most of them need OS-assisted
microcode updates to get the latest microcode available.
I am not arguing anything about the way Xen decided to go about implementing
the feature, but they got the requirement "must pass through all the
microcodes without removing any" right. It exists.
> > However, even aside from that, it means exporting a pile of internal
> > details from microcode_amd and reusing them within microcode_xen. And
> > it requires that it be done again for each vendor.
>
> Why can't you load the appropriate, unmodified microcode_<vendor> module
> in dom0 and let it call the hypercall?
Or add a microcode_virtual passthrough device, for that matter. Wouldn't
that be much cleaner and more palatable?
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On 02/02/2011 01:54 AM, Borislav Petkov wrote:
> Ok, I'm reading your answers but I keep getting the impression that this
> discussion is not moving anywhere. You keep finding reasons why it can't
> be done and trying to persuade me that your way is the only way. Why is
> that?
I agree that this conversation has got bogged down. I'm getting the
impression that you're not really understanding my answers within the
context of how Xen works, and so things are going in circles. I'm happy
to go into more detail if you're interested.
I'm not trying to be obstructionist here. We've often changed the way
things work on the Xen side to smooth the path into the kernel, and I'm
perfectly willing to do it again for the microcode driver if it makes
sense to do so.
> And I'm telling you microcode_xen has nothing to do among
> vendor-specific code. Since when is xen a hw vendor and deserves special
> attention? And don't tell me because people use it.
Who's asking for special attention? I'm just trying to use the existing
microcode infrastructure for handling different methods of microcode
update to add one more. Why is "because people use it" not a useful
thing to say? If I said "but nobody uses it", then that would be a
strong argument for not including it.
> It is absolutely
> inacceptable to add a bunch of code to arch/x86 just because you're
> telling me it can't be done differently, not intermixed with hw vendor
> code and just because a hypervisor vendor needs it.
You seem to have staked out a very... specific... position here, but I
don't agree with your premise that microcode_foo is specifically
microcode_<cpu-vendor>. If you view it as microcode_<method of loading
microcode> then adding microcode_xen makes perfect sense. Since it is a
small, self-contained piece of code that doesn't have any effects on any
other code, nor any dependencies aside from the microcode driver's
internal interface, I think it is a clean way to approach the problem.
Or to turn it around, what specific problems do you see arising from
implementing the Xen microcode loader in this way? Why is adding a
third microcode_foo.c a problem?
> Does that mean that
> if every other virtualization booth wants to add their special code to
> arch/x86, we just go and slap it in without questioning its design?
Of course not; the whole point of posting code for review is to get
feedback on both general and specific issues, and I appreciate the time
you've spent on this. But I have to say I don't really understand what
your objections are. Are you objecting to the very principle of adding
a new microcode driver, or is there something specific about the code I
posted that you have issues with?
Thanks,
J
Sorry, missed your specific comments below.
On 02/02/2011 01:54 AM, Borislav Petkov wrote:
>> collect_cpu_info can't work, because the domain doesn't have access to
>> all the host's physical CPUs.
> Why would you need that? You can safely assume that the ucode patch
> level on all cores across the system are identical - I've yet to see a
> machine running with different patch levels for the same reason that
> mixed silicon systems is a large pain in the ass and you're better off
> buying yourself a completely new system.
I was thinking specifically about a large multi-chassis system with lots
of nodes. In that case you could well imagine different nodes/chassis
having variations of CPU steppings.
But even on a single board, I don't know what limitations there are on
mixing steppings between sockets, but I suspect it is possible.
Obviously I wouldn't expect there to be large variations between CPUs
(different models or features) - that would be a problem.
>> However, even aside from that, it means exporting a pile of internal
>> details from microcode_amd and reusing them within microcode_xen. And
>> it requires that it be done again for each vendor.
> Why can't you load the appropriate, unmodified microcode_<vendor> module
> in dom0 and let it call the hypercall?
Well, if it can call hypercalls, then to some extent it is already
"modified". Either I change the source to change the wrmsrs into
hypercalls, or overwrite its operations structure to change the
apply_microcode function pointer (which would require - at the very
least - making it non-static) to a hyper-calling function.
Either way it implies that we're delving into the Intel/AMD drivers in
order to implement Xen-specific functionality, which is clearly (to me,
at least) much worse than just making the Xen-specific code completely
self-contained.
>> But all that's really needed is a dead simple "request" that loads the
>> entire file (with a vendor-specific name) and shoves it into Xen.
>> There's no need for any vendor-specific code beyond the filename.
> But that adds this funny chunk
>
> - if (c->x86_vendor == X86_VENDOR_INTEL)
> + if (xen_pv_domain())
> + microcode_ops = init_xen_microcode();
> + else if (c->x86_vendor == X86_VENDOR_INTEL)
> microcode_ops = init_intel_microcode();
> else if (c->x86_vendor == X86_VENDOR_AMD)
> microcode_ops = init_amd_microcode();
>
> which can clearly be avoided. Now imagine the if-else thing contained a
> dozen or more virt solutions in there, bloat!
This is just the way microcode_core.c is structured. It doesn't have a
dozen or more virt solutions, but if it did, we could change the way
that microcode loaders are probed for to make it cleaner. Exactly the
same issue arises if you say "there might be a dozen or more cpu vendors
- bloat!". True, there might be, and we should fix it in that case. Or
conversely, there might be a new CPU vendor who decides to implement the
Intel microcode loading interface, in which case the tests on a specific
vendor become wrong.
> Now imagine this not only
> in the microcode driver but in a bunch of other places across arch/x86.
Why imagine that? We're not talking about the rest of arch/x86.
>>> This is exactly what I'm talking about - why copy all that
>>> checking/filtering code from baremetal to Xen instead of simply reusing
>>> it? Especially if you'd need to update the copy from time to time when
>>> baremetal changes.
>> The code in the kernel is in the wrong place. It has to be done in
>> Xen. When Xen is present, the code in the kernel is redundant, not the
>> other way around.
> Ok, this says it all. Let's remove the arch code and replace it with
> xen-friendly version - we won't need the baremetal one anyway. Jeez!
I didn't say anything about removing the code from Linux. But the
simple fact is that a kernel running under a hypervisor isn't the
ultimate owner of the machine's hardware - the hypervisor is, and it
controls all access to the hardware. That's deeply fundamental to the
way any hypervisor works, and Xen is just one example. And the
consequence of that is that Linux has to use hypervisor facilities to
perform certain operations rather than using its own code.
>> Shrug. AFAICT the mechanism hasn't changed since it was first
>> introduced. If there's a change, then both Linux and Xen will have to
>> change, and most likely the same CPU vendor engineer will provide a
>> patch for both. Xen has a good record for tracking new CPU features.
> I don't think that the same CPU vendor engineer will do that, believe me!
Many individual Intel and AMD engineers contribute code to both Xen and
Linux. It's not a question of belief.
Thanks,
J