From 970811a704dc36626c00777a009dd64296a6a0c6 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Fri, 24 Jun 2022 16:29:31 -0700 Subject: [PATCH] hypervisor: x86: remove apic_base and interrupt_bitmap from Sregs These are only used in KVM, not the other x86 hypervisors, and they don't really fit into the same category as the rest of Sregs. The apic_base can be set via set_msrs(), and the interrupt_bitmap is part of the irqchip state. To enable this removal, we change the KVM set_sregs() call to retrieve the current state and use the existing apic_base and interrupt_bitmap. BUG=b:237095693 TEST=Boot x86-64 kernel on KVM Change-Id: I275eec83b74f1c364b7a543882f3ac5960201143 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3728988 Tested-by: kokoro Reviewed-by: Alexandre Courbot Reviewed-by: Vaibhav Nagarnaik Commit-Queue: Daniel Verkamp Reviewed-by: Noah Gold --- hypervisor/src/haxm/vcpu.rs | 5 ---- hypervisor/src/kvm/x86_64.rs | 57 ++++++++++++++++++------------------ hypervisor/src/whpx/types.rs | 12 ++------ hypervisor/src/x86_64.rs | 7 ----- x86_64/src/lib.rs | 6 +--- 5 files changed, 32 insertions(+), 55 deletions(-) diff --git a/hypervisor/src/haxm/vcpu.rs b/hypervisor/src/haxm/vcpu.rs index 5097db8d17..9563d53bb8 100644 --- a/hypervisor/src/haxm/vcpu.rs +++ b/hypervisor/src/haxm/vcpu.rs @@ -737,11 +737,6 @@ impl VcpuState { // HAXM does not support setting cr8 cr8: 0, efer: self.state._efer as u64, - // apic_base is part of haxm_tunnel structure - apic_base: 0, - // interrupt bitmap is really part of the APIC state, which can be gotten via - // get_lapic_state. TODO: remove from Sregs? - interrupt_bitmap: [0; 4], } } diff --git a/hypervisor/src/kvm/x86_64.rs b/hypervisor/src/kvm/x86_64.rs index 1ea8f5c9c6..adad6d95f2 100644 --- a/hypervisor/src/kvm/x86_64.rs +++ b/hypervisor/src/kvm/x86_64.rs @@ -572,10 +572,36 @@ impl VcpuX86_64 for KvmVcpu { } fn set_sregs(&self, sregs: &Sregs) -> Result<()> { - let sregs = kvm_sregs::from(sregs); + // Get the current `kvm_sregs` so we can use its `apic_base` and `interrupt_bitmap`, which + // are not present in `Sregs`. + // Safe because we know that our file is a VCPU fd, we know the kernel will only write the + // correct amount of memory to our pointer, and we verify the return result. + let mut kvm_sregs: kvm_sregs = Default::default(); + let ret = unsafe { ioctl_with_mut_ref(self, KVM_GET_SREGS(), &mut kvm_sregs) }; + if ret != 0 { + return errno_result(); + } + + kvm_sregs.cs = kvm_segment::from(&sregs.cs); + kvm_sregs.ds = kvm_segment::from(&sregs.ds); + kvm_sregs.es = kvm_segment::from(&sregs.es); + kvm_sregs.fs = kvm_segment::from(&sregs.fs); + kvm_sregs.gs = kvm_segment::from(&sregs.gs); + kvm_sregs.ss = kvm_segment::from(&sregs.ss); + kvm_sregs.tr = kvm_segment::from(&sregs.tr); + kvm_sregs.ldt = kvm_segment::from(&sregs.ldt); + kvm_sregs.gdt = kvm_dtable::from(&sregs.gdt); + kvm_sregs.idt = kvm_dtable::from(&sregs.idt); + kvm_sregs.cr0 = sregs.cr0; + kvm_sregs.cr2 = sregs.cr2; + kvm_sregs.cr3 = sregs.cr3; + kvm_sregs.cr4 = sregs.cr4; + kvm_sregs.cr8 = sregs.cr8; + kvm_sregs.efer = sregs.efer; + // Safe because we know that our file is a VCPU fd, we know the kernel will only read the // correct amount of memory from our pointer, and we verify the return result. - let ret = unsafe { ioctl_with_ref(self, KVM_SET_SREGS(), &sregs) }; + let ret = unsafe { ioctl_with_ref(self, KVM_SET_SREGS(), &kvm_sregs) }; if ret == 0 { Ok(()) } else { @@ -1214,33 +1240,6 @@ impl From<&kvm_sregs> for Sregs { cr4: r.cr4, cr8: r.cr8, efer: r.efer, - apic_base: r.apic_base, - interrupt_bitmap: r.interrupt_bitmap, - } - } -} - -impl From<&Sregs> for kvm_sregs { - fn from(r: &Sregs) -> Self { - kvm_sregs { - cs: kvm_segment::from(&r.cs), - ds: kvm_segment::from(&r.ds), - es: kvm_segment::from(&r.es), - fs: kvm_segment::from(&r.fs), - gs: kvm_segment::from(&r.gs), - ss: kvm_segment::from(&r.ss), - tr: kvm_segment::from(&r.tr), - ldt: kvm_segment::from(&r.ldt), - gdt: kvm_dtable::from(&r.gdt), - idt: kvm_dtable::from(&r.idt), - cr0: r.cr0, - cr2: r.cr2, - cr3: r.cr3, - cr4: r.cr4, - cr8: r.cr8, - efer: r.efer, - apic_base: r.apic_base, - interrupt_bitmap: r.interrupt_bitmap, } } } diff --git a/hypervisor/src/whpx/types.rs b/hypervisor/src/whpx/types.rs index a80df7b195..a252c6217a 100644 --- a/hypervisor/src/whpx/types.rs +++ b/hypervisor/src/whpx/types.rs @@ -168,12 +168,12 @@ impl From<&WHV_X64_TABLE_REGISTER> for DescriptorTable { #[derive(Default)] pub(super) struct WhpxSregs { - register_values: [WHV_REGISTER_VALUE; 17], + register_values: [WHV_REGISTER_VALUE; 16], } impl WhpxSregs { - pub(super) fn get_register_names() -> &'static [WHV_REGISTER_NAME; 17] { - const REG_NAMES: [WHV_REGISTER_NAME; 17] = [ + pub(super) fn get_register_names() -> &'static [WHV_REGISTER_NAME; 16] { + const REG_NAMES: [WHV_REGISTER_NAME; 16] = [ WHV_REGISTER_NAME_WHvX64RegisterCs, WHV_REGISTER_NAME_WHvX64RegisterDs, WHV_REGISTER_NAME_WHvX64RegisterEs, @@ -190,7 +190,6 @@ impl WhpxSregs { WHV_REGISTER_NAME_WHvX64RegisterCr4, WHV_REGISTER_NAME_WHvX64RegisterCr8, WHV_REGISTER_NAME_WHvX64RegisterEfer, // this is actually an msr - WHV_REGISTER_NAME_WHvX64RegisterApicBase, // this is actually an msr ]; ®_NAMES } @@ -242,9 +241,6 @@ impl From<&Sregs> for WhpxSregs { WHV_REGISTER_VALUE { Reg64: sregs.cr4 }, WHV_REGISTER_VALUE { Reg64: sregs.cr8 }, WHV_REGISTER_VALUE { Reg64: sregs.efer }, - WHV_REGISTER_VALUE { - Reg64: sregs.apic_base, - }, ], } } @@ -270,8 +266,6 @@ impl From<&WhpxSregs> for Sregs { cr4: whpx_regs.register_values[13].Reg64, cr8: whpx_regs.register_values[14].Reg64, efer: whpx_regs.register_values[15].Reg64, - apic_base: whpx_regs.register_values[16].Reg64, - interrupt_bitmap: [0; 4], } } } diff --git a/hypervisor/src/x86_64.rs b/hypervisor/src/x86_64.rs index 46de32c98a..1cf85d427d 100644 --- a/hypervisor/src/x86_64.rs +++ b/hypervisor/src/x86_64.rs @@ -651,11 +651,6 @@ pub struct Sregs { pub cr4: u64, pub cr8: u64, pub efer: u64, - pub apic_base: u64, - - /// A bitmap of pending external interrupts. At most one bit may be set. This interrupt has - /// been acknowledged by the APIC but not yet injected into the cpu core. - pub interrupt_bitmap: [u64; 4usize], } impl Default for Sregs { @@ -749,8 +744,6 @@ impl Default for Sregs { cr4: 0, cr8: 0, efer: 0, - apic_base: 0, - interrupt_bitmap: Default::default(), } } } diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs index 3a008f6249..7ae1ee3541 100644 --- a/x86_64/src/lib.rs +++ b/x86_64/src/lib.rs @@ -784,7 +784,7 @@ impl arch::LinuxArch for X8664arch { hypervisor: &dyn HypervisorX86_64, irq_chip: &mut dyn IrqChipX86_64, vcpu: &mut dyn VcpuX86_64, - mut vcpu_init: VcpuInitX86_64, + vcpu_init: VcpuInitX86_64, vcpu_id: usize, num_cpus: usize, _has_bios: bool, @@ -812,10 +812,6 @@ impl arch::LinuxArch for X8664arch { vcpu.set_regs(&vcpu_init.regs).map_err(Error::WriteRegs)?; - // Keep the existing `apic_base` since `vcpu_init` doesn't know the right address. - // TODO(b/237095693): remove `apic_base` from struct Sregs? - let orig_sregs = vcpu.get_sregs().map_err(Error::ReadRegs)?; - vcpu_init.sregs.apic_base = orig_sregs.apic_base; vcpu.set_sregs(&vcpu_init.sregs) .map_err(Error::SetupSregs)?;