Add #VC handler #55

p4zuu · 2023-06-20T18:04:38Z

This is a draft for the implementation of #14, which is implementing a #VC handler to handle NAE events.

NAE events have to be handled early, so we also set and load the IDT in stage 2.

At this point, interrupt handling in stage 2 doesn't work. I mostly moved and debug code to try to call the VC handler from a stage 2 cpuid, which fails. The IDT setup in this PR still handles interrupts in the SVSM kernel, but not in stage 2. My guess goes that IDT entries should be different because stage2 is a different binary (with different GDT entries?), but I'm still debugging. That's where my lack of system programming background hits.

00xc · 2023-06-21T07:56:55Z

My guess goes that IDT entries should be different because stage2 is a different binary (with different GDT entries?)

I'd say you at least need to set up a GDT with a CPL0 code segment.

p4zuu · 2023-06-21T08:06:22Z

I'd say you at least need to set up a GDT with a CPL0 code segment.

GDT is set and loaded in src/boot_stage2.rs

         * Load a GDT. Despite the naming, it contains valid
         * entries for both, "legacy" 32bit and long mode each.
         */
        movl $gdt64_desc, %eax
        lgdt (%eax)

So I guess no need to set and load it again, right? Just ensure that the IDT fields match the correct GDT entries

00xc · 2023-06-21T08:18:45Z

So I guess no need to set and load it again, right? Just ensure that the IDT fields match the correct GDT entries

The only GDT entry that is referenced from the IDT is the TSS, which contains the IST stacks for #DF handling, as far as I can tell. This is not set up until bsp_percpu.load() is called (which ends up calling load_tss()), followed by idt_init() (which calls init_ist_vectors()). So, by the time early_idt_init() is called, I don't think any entries should be different in the GDT. Have you tried calling load_gdt() in stage2?

p4zuu · 2023-06-21T09:04:35Z

The only GDT entry that is referenced from the IDT is the TSS, which contains the IST stacks for #DF handling, as far as I can tell. This is not set up until bsp_percpu.load() is called (which ends up calling load_tss()), followed by idt_init() (which calls init_ist_vectors()). So, by the time early_idt_init() is called, I don't think any entries should be different in the GDT.

That was my conclusion too, that's why I was struggling finding what could possibly go wrong.

Have you tried calling load_gdt() in stage2?

I didn't, but I just tried and it worked :) VC handler gets called by cpuid in stage 2 and in the SVSM kernel as well (depending where you put the instruction of course). Thanks Carlos! I still have to clearly figure out happens

joergroedel · 2023-07-04T12:44:27Z

src/cpu/idt_generic_handler.rs

+//
+// Copyright (c) 2022-2023 SUSE LLC
+//
+// Author: Thomas Leroy <tleroy@suse.de>


When you move code to a new file, please also add the original authors to the file header.

joergroedel · 2023-07-04T12:48:29Z

In general this looks good to me. The question is how we organize the code sharing of the IDT handlers between stage2 and SVSM kernel. The SVSM needs more complicated handlers for some things (e.g. #PF) than stage2, and we want separate handlers for both.

We can divide that by introducing features, or just include different sub-modules for SVSM and stage2. Other suggestions are also welcome.

joergroedel · 2023-07-04T12:50:43Z

I didn't, but I just tried and it worked :) VC handler gets called by cpuid in stage 2 and in the SVSM kernel as well (depending where you put the instruction of course). Thanks Carlos! I still have to clearly figure out happens

Exception handling needs a GDT to be set up, because raising the exception will reference the GDT to load the handlers code segment, and the IRET also references the GDT to jump back to the original code segment.

00xc · 2023-07-04T13:00:59Z

In general this looks good to me. The question is how we organize the code sharing of the IDT handlers between stage2 and SVSM kernel. The SVSM needs more complicated handlers for some things (e.g. #PF) than stage2, and we want separate handlers for both.

We can divide that by introducing features, or just include different sub-modules for SVSM and stage2. Other suggestions are also welcome.

I think the feeature approach will only work if we separate things into different crates. As far as I know, there is no way to do conditional compilation depending on the binary being built (e.g. stage2 or SVSM kernel).

I'd say the best way is to separate both into different sub-modules and include them based on what's needed, as you suggest. Maybe something like src/cpu/idt/stage2.rs and src/cpu/idt/svsm.rs? And perhaps a third file for any common code and structures needed.

joergroedel · 2023-07-04T13:07:35Z

I think the feeature approach will only work if we separate things into different crates. As far as I know, there is no way to do conditional compilation depending on the binary being built (e.g. stage2 or SVSM kernel).

If needed we can emulate that by introducing features which are only set per-target, like idt_stage2 and idt_svsm.

I'd say the best way is to separate both into different sub-modules and include them based on what's needed, as you suggest. Maybe something like src/cpu/idt/stage2.rs and src/cpu/idt/svsm.rs? And perhaps a third file for any common code and structures needed.

Yes, that sounds worth a try. Thanks!

p4zuu · 2023-07-04T13:20:45Z

I'd say the best way is to separate both into different sub-modules and include them based on what's needed, as you suggest. Maybe something like src/cpu/idt/stage2.rs and src/cpu/idt/svsm.rs? And perhaps a third file for any common code and structures needed.

This looks good to me too

p4zuu · 2023-07-07T12:31:59Z

Does it make sense to have two different IDT handler arrays, one for stage 2 handlers and one for SVSM? Or would it be better to just refill idt_handler_array after stage 2 with the SVSM handlers?

p4zuu · 2023-09-14T12:01:38Z

PR is almost ready to be reviewed. I left some TODOs were I asked opened questions to me and reviewers, but here is a new one: do we need to already check CPL before the #VC handling (especially for IOIO) or should we wait for CPL-3 support to be working? Maybe @joergroedel

joergroedel · 2023-09-15T14:36:28Z

PR is almost ready to be reviewed. I left some TODOs were I asked opened questions to me and reviewers, but here is a new one: do we need to already check CPL before the #VC handling (especially for IOIO) or should we wait for CPL-3 support to be working? Maybe @joergroedel

I think there is no need to check CPL right now. For CPUID it doesn't matter and CPL-3 will not be able to do IOIO directly. For that an IO permission bitmap is needed which we are not planning to implement.

Adds tests that make sure a #VC exception is raised while executing various instructions and that the #VC handler properly handles them via the ghcb. Adds tests for cpuid, msrs, port IO, MMIO, wbinvd, rdtsc, dr7, and vmmcall. Relies on being able to run tests inside the guest VM, e.g. with PR coconut-svsm#120. The #VC handler is not yet implemented, so these tests are all marked with #[should_panic]. See PR coconut-svsm#55 for progress on its implementation. TODO: Remove added dependencies Move the tests to vc.rs or a new file Clean up comments, etc. Move assembly wrapper helper functions to more sensible places Implement page state change (PSC) test

Adds tests that make sure a #VC exception is raised while executing various instructions and that the #VC handler properly handles them via the ghcb. Adds tests for cpuid, msrs, port IO, MMIO, wbinvd, rdtsc, dr7, and vmmcall. Relies on being able to run tests inside the guest VM, e.g. with PR coconut-svsm#120. The #VC handler is not yet implemented, so these tests are all marked with #[should_panic]. See PR coconut-svsm#55 for progress on its implementation. TODO: Remove added dependencies Move the tests to vc.rs or a new file Clean up comments, etc. Move assembly wrapper helper functions to more sensible places Implement page state change (PSC) test Signed-off-by: Adam Dunlap <acdunlap@google.com>

AdamCDunlap · 2023-11-02T21:25:06Z

I pushed a new version with IOIO handling in stage 2. If your tests still pass, I will remove the Draft tag :)

I tried the new version and the previously-passing tests still pass (also IOIO for 32-bit operands, which was failing before due a bug in the test). I don't have them running in stage 2 yet, though.

There's also some IO tests that are still failing when the port is encoded in the instruction, e.g:

fn outl_to_testdev_echo(value: u32) {
    unsafe { asm!("outl %eax, $0xe0", in("eax") value, options(att_syntax)) }
}

it looks like this happens when the opcode is 0xe4, 0xe5, 0xe6, or 0xe7. I'm not sure if you care about supporting this case, but I thought I'd mention it.

p4zuu · 2023-11-03T08:37:35Z

I pushed a new version with IOIO handling in stage 2. If your tests still pass, I will remove the Draft tag :)

I tried the new version and the previously-passing tests still pass (also IOIO for 32-bit operands, which was failing before due a bug in the test). I don't have them running in stage 2 yet, though.

As we said, I don't expect different behavior in stage 2 and SVSM kernel, but if it's possible for you to test in stage 2, that would be safer. Let me know if there's anything I can do :)

There's also some IO tests that are still failing when the port is encoded in the instruction, e.g:
fn outl_to_testdev_echo(value: u32) {
    unsafe { asm!("outl %eax, $0xe0", in("eax") value, options(att_syntax)) }
}
it looks like this happens when the opcode is 0xe4, 0xe5, 0xe6, or 0xe7. I'm not sure if you care about supporting this case, but I thought I'd mention it.

Indeed, port specified in an imm8 is currently not supported. We'll add support later if needed.

AdamCDunlap · 2023-11-03T19:47:09Z

I added testing support to stage2 and the cpuid tests pass there as well.

p4zuu · 2023-11-13T15:05:59Z

I added testing support to stage2 and the cpuid tests pass there as well.

Great. I'll remove the Draft tag of my PR then, and make it open for review. Your testing code looks good to me :)

00xc

Added some comments below. Overall error handling could be cleaned up a bit. There are also are some safety concerns I have that I noted below.

src/cpu/vc.rs

src/cpu/insn.rs

src/cpu/vc.rs

src/cpu/insn.rs

joergroedel · 2023-11-20T15:46:26Z

Okay, so there have been some suggestions included by me on-top. Can you nuke this commit and rebase the base-PR to latest main branch, please? Please also address Carlos' comments when updating.

p4zuu · 2023-11-21T11:09:53Z

Rebased on main, and added Carlos' proposals

00xc · 2023-11-21T12:23:11Z

By the way your commit messages seem to be strangely line wrapped. I suggest wrapping lines at 70 or 72 columns.

00xc

A few more extra comments

00xc · 2023-11-21T12:33:33Z

src/cpu/insn.rs

+#[derive(Default, Debug, Copy, Clone)]
+pub struct Instruction {
+    pub prefixes: InstructionField,
+    pub insn_bytes: [u8; MAX_INSN_SIZE],
+    pub length: usize,
+    pub opcode: InstructionField,
+    pub opnd_bytes: usize,
+}
+
+#[derive(Default, Debug, Copy, Clone)]
+pub struct InstructionField {
+    pub bytes: [u8; MAX_INSN_FIELD_SIZE],
+    pub nb_bytes: usize,
+}


There are several arrays with a field to track length, so how about the following to reduce code duplication:

#[derive(Debug, Copy, Clone, Default)] pub(super) struct InsBuffer<const N: usize> where [u8; N]: Default, { buf: [u8; N], len: usize, } #[derive(Debug, Copy, Clone, Default)] pub(super) struct Instruction { pub prefixes: InsBuffer<MAX_INSN_FIELD_SIZE>, pub bytes: InsBuffer<MAX_INSN_SIZE>, pub opcode: InsBuffer<MAX_INSN_FIELD_SIZE>, }

This would also make Instruction::new() a bit more clear. This could also be done in a separate PR if it's too much work to refactor.

Good point, that's code inspired from C without stepping back enough.
I'll start working on a refactoring but that indeed could be in another PR.
The current InstructionField.nb_bytes is not correlated to InstructionField.bytes.len() and is used differently, so I'll keep the nb_bytes field name or something similar and add a comment why this field

Yes, please do that in an additional PR. It makes sense to properly abstract the instruction buffer, but it can be done on-top of this one.

src/cpu/insn.rs

joergroedel · 2023-11-21T15:57:28Z

@p4zuu Can you please work in the suggestions from @00xc and rebase to latest main branch? This should fix the CI failure too.

Stage2 and SVSM kernel have to handle interrupts in different ways. This creates 2 different IDT handler arrays: one calling stage2 handlers, and one calling SVSM handlers. cpu/idt/stage2.rs and cpu/idt/svsm.rs expose the same functions, callable from the appropriate context (stage2 or SVSM). Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

To handle #VC IOIO and CPUID exceptions, we need to decode those two instructions. Since we start by handling IOIO instructions using registers and CPUID, with both fixed size, we don't need a full x86 decoder. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

There would be 2 ways to handle CPUID #VC exception: using the CPUID table provided by the firmware (already mapped in memory in stage 2), and forwarding the CPUID instruction to the hypervisor through a VMEXIT. Since the hyervisor is not trusted, we only handle CPUID with the firmware table. With this, since CPUID handling doesn't require the GHCB, stage 2 and SVSM kernel have the same #VC handler. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

Handle the IN and OUT instructions by calling the hypervisor through the existing GHCB ioio methods. At this point, IN and OUT instructions with the port given as immediate value is not supported yet. INS and OUTS are also not supported yet. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

Just replace an if/else chain logic with a match in SVSM and stage 2 generic IDT handlers. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

In stage 2, we need to handle some #VC exceptions with GHCB. To do that, we need to init and load IDT again, after the GHCB is set, with new handlers using GHCB (eg. IOIO handler). Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

Cleaner error handling with the use of `expect()` and a more precise `VcError`. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

p4zuu · 2023-11-21T16:11:19Z

@p4zuu Can you please work in the suggestions from @00xc and rebase to latest main branch? This should fix the CI failure too.

Should be good now

p4zuu force-pushed the stage2_vc_handler branch from 98eafda to 8b72185 Compare June 21, 2023 09:00

joergroedel reviewed Jul 4, 2023

View reviewed changes

p4zuu force-pushed the stage2_vc_handler branch 2 times, most recently from c1b1589 to f38ab74 Compare July 10, 2023 14:12

p4zuu force-pushed the stage2_vc_handler branch from f38ab74 to dd7bd2b Compare August 17, 2023 13:39

p4zuu force-pushed the stage2_vc_handler branch from dd7bd2b to 196bf9b Compare August 25, 2023 12:59

p4zuu force-pushed the stage2_vc_handler branch 2 times, most recently from bad3123 to 164f547 Compare September 14, 2023 12:00

p4zuu force-pushed the stage2_vc_handler branch from 164f547 to dd306f6 Compare September 14, 2023 12:30

p4zuu force-pushed the stage2_vc_handler branch from dd306f6 to fc416a7 Compare September 29, 2023 13:29

p4zuu force-pushed the stage2_vc_handler branch 4 times, most recently from 98fbbed to 3d3018d Compare October 20, 2023 13:29

p4zuu force-pushed the stage2_vc_handler branch from 3d3018d to d1a69b0 Compare October 25, 2023 13:05

p4zuu force-pushed the stage2_vc_handler branch from 1dd1f3d to 506f4f3 Compare November 2, 2023 15:59

p4zuu marked this pull request as ready for review November 13, 2023 15:06

p4zuu changed the title ~~Draft: Add #VC handler~~ Add #VC handler Nov 13, 2023

00xc reviewed Nov 17, 2023

View reviewed changes

p4zuu force-pushed the stage2_vc_handler branch 4 times, most recently from ee760f2 to 7fde4c7 Compare November 21, 2023 10:50

00xc reviewed Nov 21, 2023

View reviewed changes

p4zuu force-pushed the stage2_vc_handler branch from 7fde4c7 to 6478545 Compare November 21, 2023 16:01

p4zuu added 6 commits November 21, 2023 17:07

cpu/idt: cleaner vector matching in IDT handlers

c5f0bd3

Just replace an if/else chain logic with a match in SVSM and stage 2 generic IDT handlers. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

cpu/vc: stage2: init GHCB-based inerrupt handlers

94d1705

In stage 2, we need to handle some #VC exceptions with GHCB. To do that, we need to init and load IDT again, after the GHCB is set, with new handlers using GHCB (eg. IOIO handler). Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

cpu/insn,vc: improved error handling

e6263e8

Cleaner error handling with the use of `expect()` and a more precise `VcError`. Signed-off-by: Thomas Leroy <thomas.leroy@suse.com>

p4zuu force-pushed the stage2_vc_handler branch from 6478545 to e6263e8 Compare November 21, 2023 16:07

joergroedel merged commit 1504201 into coconut-svsm:main Nov 21, 2023
2 checks passed

00xc mentioned this pull request Nov 30, 2023

Implement VC handling in stage2 and COCONUT kernel #14

Open

00xc mentioned this pull request Apr 23, 2024

cpu/vc: implement RDTSC and RDSTCP #VC handlers #327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #VC handler #55

Add #VC handler #55

p4zuu commented Jun 20, 2023 •

edited

00xc commented Jun 21, 2023

p4zuu commented Jun 21, 2023

00xc commented Jun 21, 2023

p4zuu commented Jun 21, 2023

joergroedel Jul 4, 2023

joergroedel commented Jul 4, 2023

joergroedel commented Jul 4, 2023

00xc commented Jul 4, 2023

joergroedel commented Jul 4, 2023

p4zuu commented Jul 4, 2023

p4zuu commented Jul 7, 2023

p4zuu commented Sep 14, 2023

joergroedel commented Sep 15, 2023

AdamCDunlap commented Nov 2, 2023

p4zuu commented Nov 3, 2023

AdamCDunlap commented Nov 3, 2023

p4zuu commented Nov 13, 2023

00xc left a comment

joergroedel commented Nov 20, 2023

p4zuu commented Nov 21, 2023

00xc commented Nov 21, 2023

00xc left a comment

00xc Nov 21, 2023 •

edited

p4zuu Nov 21, 2023

joergroedel Nov 21, 2023

joergroedel commented Nov 21, 2023

p4zuu commented Nov 21, 2023

Add #VC handler #55

Add #VC handler #55

Conversation

p4zuu commented Jun 20, 2023 • edited

00xc commented Jun 21, 2023

p4zuu commented Jun 21, 2023

00xc commented Jun 21, 2023

p4zuu commented Jun 21, 2023

joergroedel Jul 4, 2023

Choose a reason for hiding this comment

joergroedel commented Jul 4, 2023

joergroedel commented Jul 4, 2023

00xc commented Jul 4, 2023

joergroedel commented Jul 4, 2023

p4zuu commented Jul 4, 2023

p4zuu commented Jul 7, 2023

p4zuu commented Sep 14, 2023

joergroedel commented Sep 15, 2023

AdamCDunlap commented Nov 2, 2023

p4zuu commented Nov 3, 2023

AdamCDunlap commented Nov 3, 2023

p4zuu commented Nov 13, 2023

00xc left a comment

Choose a reason for hiding this comment

joergroedel commented Nov 20, 2023

p4zuu commented Nov 21, 2023

00xc commented Nov 21, 2023

00xc left a comment

Choose a reason for hiding this comment

00xc Nov 21, 2023 • edited

Choose a reason for hiding this comment

p4zuu Nov 21, 2023

Choose a reason for hiding this comment

joergroedel Nov 21, 2023

Choose a reason for hiding this comment

joergroedel commented Nov 21, 2023

p4zuu commented Nov 21, 2023

p4zuu commented Jun 20, 2023 •

edited

00xc Nov 21, 2023 •

edited