Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

port to new grant interface provided by mirage-xen #79

Merged
merged 6 commits into from Apr 5, 2019

Conversation

Projects
None yet
3 participants
@yomimono
Copy link
Member

commented Mar 28, 2019

bonuses: disconnect is now implemented, a few other arbitrary cleanups!

@yomimono yomimono requested review from djs55 and jonludlam Mar 28, 2019

@talex5
Copy link
Contributor

left a comment

Mostly looks good, but not sure about the disconnect logic (seems different to what mini-os does).

Travis is failing with:

# File "lib/blkfront.ml", line 334, characters 2-12:
# Error: This expression has type OS.Xs.client Lwt.t
#        but an expression was expected of type [< `Error of 'a | `OK of 'b ]

(I also added some minor comments about opportunities to improve the style of the existing code while updating it; feel free to ignore them)


let lookup_mapping gref =
if not(Hashtbl.mem grant_table gref) then begin
Log.err (fun f -> f "FATAL: failed to find mapped grant reference %ld" gref);
Log.err (fun f -> f "FATAL: failed to find mapped grant reference %s" @@ OS.Xen.Gntref.to_string gref);

This comment has been minimized.

Copy link
@talex5

talex5 Mar 29, 2019

Contributor

There's a OS.Xen.Gntref.pp too...

let _ = List.fold_left (fun i gref -> Hashtbl.add grant_table (Int32.of_int gref.Gnttab.ref) (Cstruct.sub buf (4096 * i) 4096); i + 1) 0 grants in
| Ok x ->
let buf = Io_page.to_cstruct @@ OS.Xen.Import.Local_mapping.to_buf x in
let () =

This comment has been minimized.

Copy link
@talex5

talex5 Mar 29, 2019

Contributor

Minor: could just use ...; here instead of let () = ... in.

| None ->
match OS.Xen.Import.mapv ~writable:true grants with
| Error (`Msg s) ->
Log.err (fun f -> f "OS.Xen.Import.mapv failed during initialization: %s" s);
failwith "Gnttab.mapv failed"

This comment has been minimized.

Copy link
@talex5

talex5 Mar 29, 2019

Contributor

Maybe just Fmt.failwith here to return the full, formatted error? Seems overkill to log and raise. Also, the exception message doesn't match the log message and uses the old name.

let th = service_thread t stats in
on_cancel th (fun () ->
let counter = ref 0 in
Ring.Rpc.Back.ack_requests ring (fun _ -> incr counter);
if !counter <> 0 then Log.err (fun f-> f "FATAL: before unmapping, there were %d outstanding requests on the ring. Events lost?" !(counter));
let () = Gnttab.unmap_exn xg mapping in ()
let () = OS.Xen.Import.Local_mapping.unmap_exn mapping in ()

This comment has been minimized.

Copy link
@talex5

talex5 Mar 29, 2019

Contributor

Could remove the let () = here too.

let rfs = snd(List.fold_left (fun (i, acc) g ->
i + 1, ((sprintf "ring-ref%d" i, string_of_int g) :: acc)
let rfs = snd
(List.fold_left (fun (i, acc) g ->

This comment has been minimized.

Copy link
@talex5

talex5 Mar 29, 2019

Contributor

Could update to use List.mapi.

(* when we write here, it ends up in the dom0 tree, not our local one *)
(* that's seeming mirrored from the actual write location, local/domid/device/vbd/blah *)
(* do we need to write it somewhere else? 34/device/vbd/blah shows *)
(* the second set of things is in the libxl/ tree - how can we manipulate that? *)

This comment has been minimized.

Copy link
@talex5

talex5 Mar 29, 2019

Contributor

I'm not sure how this is supposed to work either. It seems a bit pointless to write the Closing state and then delete the whole tree immediately (probably before the backend sees it). It looks like mini-os does this:

  1. Set frontend state to Closing.
  2. Wait for backend state to become >= Closing.
  3. Set frontend state to Closed.
  4. Wait for backend state to become >= Closed.
  5. Set frontend state to Initialising.
  6. Wait for backend state to become >= InitWait && < Closed.

(https://github.com/mirage/mini-os/blob/master/blkfront.c#L247)

This comment has been minimized.

Copy link
@yomimono

yomimono Mar 29, 2019

Author Member

Ha, oops, that's some comment garbage I didn't intend to submit with this PR. Anyway, thanks for the pointer on the flow here - I consulted some Xen experts and got some subset of that workflow, then did a little hacking around and submitted what seems to work. What you describe does indeed seem more sensible.

@yomimono yomimono force-pushed the yomimono:new-gnt-iface branch from 0a774c8 to 27e1777 Apr 2, 2019

@yomimono

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

I've somewhat presumptuously added a CHANGES entry for the 1.6.1 release I'd like to make if this PR is merged.

@yomimono yomimono requested a review from talex5 Apr 2, 2019

Show resolved Hide resolved lib/blkback.ml Outdated
Show resolved Hide resolved lib/blkfront.ml Outdated
Show resolved Hide resolved lib/blkfront.ml Outdated
Show resolved Hide resolved lib/blkback.ml Outdated

yomimono added some commits Apr 3, 2019

deal directly with OS.Xen.Gntref.t's in a few more places
Replace some code which deals with grant references as int32's - we can
use Gntref.t there and only deal with int32s when setting values in
cstructs.
@talex5

talex5 approved these changes Apr 5, 2019

@yomimono yomimono merged commit 02e0eb8 into mirage:master Apr 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.