Skip to content

Conversation

zli
Copy link
Contributor

@zli zli commented Oct 30, 2018

Signed-off-by: Zheng Li zheng.li3@citrix.com

@robhoes
Copy link
Member

robhoes commented Oct 31, 2018

So this appears to just make the VM.start fail if pv-iommu is not yet ready, rather than causing the call to wait/block until it is ready. This may be the right thing to do, but the commit message is a little misleading.

@zli zli changed the title CA-290024: VM with pv-iommu must wait for initialization complete CA-290024: Reject booting pv-iommu VMs on a host where the premap is yet to complete Oct 31, 2018
@zli
Copy link
Contributor Author

zli commented Oct 31, 2018

So this appears to just make the VM.start fail if pv-iommu is not yet ready, rather than causing the call to wait/block until it is ready.

Yes, this is to follow Bob's suggest on the ticket.

This may be the right thing to do, but the commit message is a little misleading.

Commit message is updated.

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This iooks good. Just one suggestion.

try
let s = Unixext.string_of_file "/sys/kernel/pv_iommu_ready" in
let s = Xstringext.String.strip Xstringext.String.isspace s in
is_ready := int_of_string s = 1;
Copy link
Contributor

@lindig lindig Nov 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be done simpler:

let is_ready =  
  try 
    let s = Unixext.string_of_file "/sys/kernel/pv_iommu_ready" in
    Scanf.sscanf s " %d " (fun d -> d=1)  
  with _ -> true

Note that this will scan strings like "1" or " 1" successfully.

Copy link
Contributor Author

@zli zli Nov 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with sscanf function suggested.
Though the mutable variable is_ready is on purpose, to avoid re-reading/re-parsing once it already becomes true.

…yet to complete

Signed-off-by: Zheng Li <zheng.li3@citrix.com>
@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 20.921% when pulling 3120c88 on zli:CA-290024 into d59808f on xapi-project:master.

@lindig lindig merged commit 7dda143 into xapi-project:master Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants