2015-05-27 19:05+0200, Paolo Bonzini:
> The big ugly one. This patch adds support for switching in and out of
> system management mode, respectively upon receiving KVM_REQ_SMI and upon
> executing a RSM instruction. Both 32- and 64-bit formats are supported
> for the SMM state save area.
>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> RFC->v1: shift access rights left by 8 for 32-bit format
> move tracepoint to kvm_set_hflags
> fix NMI handling
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> static void process_smi(struct kvm_vcpu *vcpu)
> {
> + struct kvm_segment cs, ds;
> + char buf[512];
> + u32 cr0;
> + int r;
> +
> if (is_smm(vcpu)) {
> vcpu->arch.smi_pending = true;
> return;
> }
>
> - printk_once(KERN_DEBUG "Ignoring guest SMI\n");
> + trace_kvm_enter_smm(vcpu->vcpu_id, vcpu->arch.smbase, true);
> + vcpu->arch.hflags |= HF_SMM_MASK;
> + memset(buf, 0, 512);
> + if (guest_cpuid_has_longmode(vcpu))
> + process_smi_save_state_64(vcpu, buf);
> + else
> + process_smi_save_state_32(vcpu, buf);
> +
> + r = kvm_write_guest(vcpu->kvm, vcpu->arch.smbase + 0xfe00, buf, sizeof(buf));
The state is saved in SMRAM, but we are accessing it using the non-SMM
address space ... how did it pass testing?
(Restore is using SMM address space, so I'm guessing that the mapping
from QEMU wasn't really utilizing two separate address spaces.)
> + if (r < 0)
> + return;
And if we fail to write it, is there other option than throwing an error
to userspace? (Unset HF_SMM_MASK and pretend that nothing happened
doesn't find much support in docs.)
Thanks.
On 03/06/2015 21:02, Radim Krčmář wrote:
>> + r = kvm_write_guest(vcpu->kvm, vcpu->arch.smbase + 0xfe00, buf, sizeof(buf));
>
> The state is saved in SMRAM, but we are accessing it using the non-SMM
> address space ... how did it pass testing?
> (Restore is using SMM address space, so I'm guessing that the mapping
> from QEMU wasn't really utilizing two separate address spaces.)
At this point of the series there are no separate address spaces yet.
Patch 10 then changes it everywhere:
@@ -6558,7 +6558,7 @@ static void process_smi(struct kvm_vcpu *vcpu)
else
process_smi_save_state_32(vcpu, buf);
- r = kvm_write_guest(vcpu->kvm, vcpu->arch.smbase + 0xfe00, buf, sizeof(buf));
+ r = kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00, buf, sizeof(buf));
if (r < 0)
return;
Why did I order it this way? Because it is already possible to test
this code with the default SMBASE of 0x30000, and it is already
possible to run the full firmware if you hack it not to close SMRAM
(for this I used q35's high SMRAM). It is not possible to test the
code partially if you first add the two address spaces, and only
implement the world switch second.
Thanks,
Paolo
>> + if (r < 0)
>> + return;
>
> And if we fail to write it, is there other option than throwing an error
> to userspace? (Unset HF_SMM_MASK and pretend that nothing happened
> doesn't find much support in docs.)
>
> Thanks.
>
2015-06-04 08:14+0200, Paolo Bonzini:
> On 03/06/2015 21:02, Radim Krčmář wrote:
>>> + r = kvm_write_guest(vcpu->kvm, vcpu->arch.smbase + 0xfe00, buf, sizeof(buf));
>>
>> The state is saved in SMRAM, but we are accessing it using the non-SMM
>> address space ... how did it pass testing?
>> (Restore is using SMM address space, so I'm guessing that the mapping
>> from QEMU wasn't really utilizing two separate address spaces.)
>
> At this point of the series there are no separate address spaces yet.
> Patch 10 then changes it everywhere:
>
> @@ -6558,7 +6558,7 @@ static void process_smi(struct kvm_vcpu *vcpu)
My bad, people using jackhammers at 7am are getting the better of me.
> Why did I order it this way? Because it is already possible to test
> this code with the default SMBASE of 0x30000, and it is already
> possible to run the full firmware if you hack it not to close SMRAM
> (for this I used q35's high SMRAM). It is not possible to test the
> code partially if you first add the two address spaces, and only
> implement the world switch second.
The ordering makes sense; I wanted to point out the early return,
noticed this as well and missed that it was fixed later, sorry.
On 03/06/2015 21:02, Radim Krčmář wrote:
> > + if (r < 0)
> > + return;
>
> And if we fail to write it, is there other option than throwing an error
> to userspace? (Unset HF_SMM_MASK and pretend that nothing happened
> doesn't find much support in docs.)
Just do nothing, I guess. If this is ROM, things will work (for some
definition of work) the same as in real hardware. If it's MMIO, they
will break as soon as the next instruction is executed.
Paolo