Xen singlestep support by Wenzel · Pull Request #150 · Wenzel/libmicrovmi

Wenzel · 2020-12-20T00:25:20Z

Continuing #113

Wenzel · 2020-12-20T03:12:41Z

This PR brings singlestep support for Xen, and implements the reply_event method, which was missing.

I'm still having an issue to listen on multiple VCPU, I only receive events from VCPU 0

rageagainsthepc · 2020-12-20T11:31:22Z

src/driver/xen.rs

+        // enable singlestep monitoring
+        // it will only intercept events when explicitely requested using
+        // xc_domain_debug_control()
+        // TODO: call get_vcpu_count()


That comment is obsolete afais, right?

Actually no.
I copy pasted the get_vcpu_count() implementation here, because if i created the Xen struct below and tried to call get_vcpu_count() afterwards, it had an issue with xc being moved above when constructing the Xen struct.

As I didn't have any solution, I used this.

I suggest we should make a function get_vcpu_count(xc: &Xc) and call it from the trait and the Xen driver init function

I've just refactored it.

Hm, I don't see the problem. works fine for me. On a side note, this function has become too large imho, we should split it.

The problem was that I copy pasted a chunk of code, so there was duplication.
I we update the definition of get_vcpu_count one day, the other one becomes obselete, etc.

This works fine for me.

Github won't diplay your link: Non-Image content-type returned

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=661acd93eb80b05457b11da953d02b41

oh.
I don't know I was seeing having a mutable Xen struct as a bad idea.
it's not, since it's restricted on the init function anyway.

I refactored and implemented your changes, thank you

codecov-io · 2020-12-20T11:59:05Z

Codecov Report

Merging #150 (f3ea0ae) into master (b61e80f) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
- Coverage   13.95%   13.84%   -0.12%     
==========================================
  Files           5        5              
  Lines         480      484       +4     
  Branches       71       71              
==========================================
  Hits           67       67              
- Misses        396      400       +4     
  Partials       17       17

Flag	Coverage Δ
unittests	`13.84% <0.00%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/api.rs	`0.00% <0.00%> (ø)`
src/driver/kvm.rs	`22.00% <0.00%> (-0.08%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b61e80f...0d957f0. Read the comment docs.

Wenzel · 2020-12-20T12:05:38Z

I can confirm that it traps events for all VCPUs

Wenzel · 2020-12-20T12:24:57Z

One issue that needs a fix is that the singlestep-events example leave the domain frozen.
The cause should be that there is a singlestep event left in the ring that requires processing, but we already left.

I thought that closing the monitoring and unbinding would clear the queue, but that's not the case apparently.

Looking at Libvmi xen_events_destroy():
https://github.com/libvmi/libvmi/blob/master/libvmi/driver/xen/xen_events.c#L3253

They do a finall pass of listening for events, if any, to clear the queue.
We should do the same

rageagainsthepc · 2020-12-20T12:25:02Z

src/driver/xen.rs

+                match self.xev.xenevtchn_pending()? {
+                    -1 => {
+                        // no event channel port is pending
+                        // TODO: Err


Is there a reason why you haven't implemented this yet? Feels to me like it should be done in this PR.

Because this means creating custom error types, and I'm not feeling confident / knowleadgable enough on the topic to implement this.

Yeah, the overall error handling in this library is not very good. I once tried to refactor it, but got discouraged after realizing how much I would need to change.

At some point we should create a custom error type (or perhaps even multiple, one for each driver) and implement error wrapping instead of error boxing. This is the way. ;)

I'm totally in favor of something like this.
I'm simply lacking the time to invest in learning state of the art Rust, and to reflect the changes in libmicrovmi.

But my heart breaks every time I call unwrap() in libmicrovmi simply because I'm a noob in error handling.

rageagainsthepc · 2020-12-20T12:29:32Z

src/driver/xen.rs

+                    }
+                };
+                let back_ring_ptr = &mut self.back_ring;
+                match RING_HAS_UNCONSUMED_REQUESTS!(back_ring_ptr) != 0 {


Pattern matching is overkill for a simple boolean check. Also, didn't we come to the conclusion that all those macros could have been simple functions?

Also, didn't we come to the conclusion that all those macros could have been simple functions?

Probably, but my focus right now is to make it work.
I'm sure we can improve afterwards.

rageagainsthepc · 2020-12-20T12:40:46Z

src/driver/xen.rs

+        let mut rsp: vm_event_response_t =
+            unsafe { mem::MaybeUninit::<vm_event_response_t>::zeroed().assume_init() };
+        rsp.reason = req.reason;
+        rsp.version = VM_EVENT_INTERFACE_VERSION;
+        rsp.vcpu_id = req.vcpu_id;
+        rsp.flags = req.flags & add_flags;


Suggested change

let mut rsp: vm_event_response_t =

unsafe { mem::MaybeUninit::<vm_event_response_t>::zeroed().assume_init() };

rsp.reason = req.reason;

rsp.version = VM_EVENT_INTERFACE_VERSION;

rsp.vcpu_id = req.vcpu_id;

rsp.flags = req.flags & add_flags;

let mut rsp = vm_event_response_t {

reason: req.reason,

version: VM_EVENT_INTERFACE_VERSION,

vcpu_id: req.vcpu_id,

flags: req.flags & add_flags,

..Default::default()

};

rageagainsthepc · 2020-12-20T12:43:46Z

src/driver/xen.rs

+                let op: u32 = match enabled {
+                    false => XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF,
+                    true => XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON,
+                };


Suggested change

let op: u32 = match enabled {

false => XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF,

true => XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON,

};

let op: u32 = if enabled {

XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON

} else {

XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF

};

Wenzel · 2020-12-20T12:58:54Z

the latest commits implemented your fixes @rageagainsthepc

I also implemented a cleanup of the ring when we close the Xen driver.
And even I can clearly see that we catch and replying to a remaining singlestep event:

My domain is still frozen.
if you have ideas.

rageagainsthepc · 2020-12-20T13:47:37Z

My domain is still frozen.
if you have ideas.

The cleanup code looks good to me. 🤔

Wenzel · 2020-12-20T14:14:51Z

I add debug output in xenevtchn

Wenzel · 2020-12-29T00:57:38Z

So, latest update.

the domain survives now, but only if it has 1 VCPU.
with 2 VCPU it freezes, which means that the second VCPU is missing a singlestep shutdown API call somehow, or it misses an event notification, idk.

I added a -c argument to singlestep-events example, to only catch a limited amount of events.

Wenzel · 2020-12-29T00:59:47Z

Here is the xen-access.c source that I tweaked
https://gist.github.com/Wenzel/c9fc0ab4f6d4e878e39dcca0fa2a79f4

compile with gcc xen-access.c -o xen-access -lxenctrl -lxenevtchn
run with sudo ./xen-access <dom_id> singlestep

rageagainsthepc · 2020-12-29T08:33:19Z

Here is the xen-access.c source that I tweaked

What of it? Do you use it as a "control group" to see what we did differently? Does it work without a problem?

Wenzel · 2020-12-29T09:17:41Z

Yes, it is a small example, catching one singlestep and quit, with prints on any Xen API calls.

Yes, it works.

Wenzel added 2 commits December 19, 2020 21:33

api: add Singlestep Intercept and Event type

8e1ac45

cargo: update xenctrl

5e45404

Wenzel force-pushed the xen_singlestep_support branch from ea13cdf to d484b70 Compare December 20, 2020 03:11

rageagainsthepc reviewed Dec 20, 2020

View reviewed changes

Wenzel added 4 commits December 20, 2020 12:47

xen: toggle intercept for singlestep

26f88fc

xen: handle Singlestep event

115a333

examples: add singlestep-events

0044483

xen: refactor listen

9bbd1b4

Wenzel force-pushed the xen_singlestep_support branch from d484b70 to 0d3f229 Compare December 20, 2020 11:48

Wenzel added 5 commits December 20, 2020 13:13

xen: impl reply_event

135ea9c

examples/singlestep-events: intercept on all VCPUs

185ad0c

examples/singlestep-events: display listen error and break the loop

59045e9

xen: remove unwraps in listen

1cf4f56

kvm: handle unimplemented intercepts in match arm

8d0dcae

Wenzel force-pushed the xen_singlestep_support branch from a52c35f to 8d0dcae Compare December 20, 2020 12:13

rageagainsthepc reviewed Dec 20, 2020

View reviewed changes

Wenzel added 2 commits December 20, 2020 13:47

xen: clean the ring on exit

5125067

xen: replace match on booleans by ifs

668cc86

xen: refactor building vm_event_response_t struct

e12a4c0

Wenzel force-pushed the xen_singlestep_support branch from cd4d305 to e12a4c0 Compare December 20, 2020 12:59

cargo: update xenevtchn

26da7a5

Wenzel added 6 commits December 28, 2020 23:51

cargo: upgrade xenctrl to 0.4.5

837accf

xen: unmap ring buffer on drop

e9ae825

xen: rework pause to check if domain is already paused

e0ba9f7

xen: fix disabling singlestep on drop

dde195a

examples/singlestep-events: add counter option

f3ea0ae

c_examples/regs-dump: fix compilation

0d957f0

Conversation

Wenzel commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wenzel commented Dec 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rageagainsthepc Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Wenzel commented Dec 20, 2020

Uh oh!

Wenzel commented Dec 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wenzel commented Dec 20, 2020

Uh oh!

rageagainsthepc commented Dec 20, 2020

Uh oh!

Wenzel commented Dec 20, 2020

Uh oh!

Wenzel commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wenzel commented Dec 29, 2020

Uh oh!

rageagainsthepc commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wenzel commented Dec 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Wenzel commented Dec 20, 2020 •

edited

Loading

rageagainsthepc Dec 20, 2020 •

edited

Loading

codecov-io commented Dec 20, 2020 •

edited

Loading

Wenzel commented Dec 29, 2020 •

edited

Loading

rageagainsthepc commented Dec 29, 2020 •

edited

Loading