Changed vm_mid_bits and VM_MID_BIT variable to 64

[insidiaGithub]

This change allows you to run more than 8 virtual machines.

[krytarowski]

Could you please put this define into core/include/config.h?

I need it for the NetBSD backend. Also HAX_MAX_VCPUS is a little bit more verbose to me rather than cryptic VM_MID_BIT.

[insidiaGithub]

Hi. I changed HAX_MAX_VCPUS to 128 and put this define to config.h

[raphaelning]

Hmm, now VM_MID_BIT is defined twice in two different files. The ideal solution is to define it once in one common header (config.h, or another one) and include that header in vm.c. Also, I agree with @krytarowski that we should take the chance to rename it to HAX_MAX_VMS (I believe HAX_MAX_VCPUS is a typo).

@insidiaGithub You could go ahead with the proper refactoring. Or, if you want this PR to focus on the original issue, that's also fine. Please keep only the first commit, and I'll approve it. @krytarowski If #137 can't handle 64 VMs, you can define a separate constant in a NetBSD header and limit it to 8.

[raphaelning]

Please also fix your commit message:

1. The format is a (concise) subject line, followed by a blank line, followed by a more detailed description.
2. Signed-off-by: (git commit -s) is required. See https://github.com/intel/haxm/blob/master/CONTRIBUTING.md

[raphaelning]

Hmm, now VM_MID_BIT is defined twice in two different files. The ideal solution is to define it once in one common header (config.h, or another one) and include that header in vm.c. Also, I agree with @krytarowski that we should take the chance to rename it to HAX_MAX_VMS (I believe HAX_MAX_VCPUS is a typo).

@insidiaGithub You could go ahead with the proper refactoring. Or, if you want this PR to focus on the original issue, that's also fine. Please keep only the first commit, and I'll approve it. @krytarowski If #137 can't handle 64 VMs, you can define a separate constant in a NetBSD header and limit it to 8.

[raphaelning]

@insidiaGithub Do you need more than 16 vCPUs per VM? Have you tested this change? I just wonder if this is a step too far for this PR. Apparently, the underlying logic for limiting the number of vCPUs to 16 is very different from that for limiting the number of VMs, and I need more time to analyze the former and understand the ramifications of this change.

[insidiaGithub]

Hi @raphaelning ! Please check my сhanges again. Thank you)

[raphaelning]

Thanks for addressing my previous comment! You've also renamed vm_mid_bits, which led me to an inspiration--sorry about that, but that should be the last change request for this PR.

[raphaelning]

Since this variable is local to vm.c, we don't really have to rename it. If you don't like the old name, how about vm_mid_seed? It's based on the pattern of vm_t::vpid_seed (core/include/vm.h), and implies that it's used for generating VM IDs. Actually, we can follow vpid_seed's example and turn this variable into an array, which should be a better way to make a connection between it and HAX_MAX_VMS:

Suggested change

	static uint64_t hax_max_vms_bits = 0;
	static uint8_t vm_mid_seed[HAX_MAX_VMS / 8];

Note that we don't really need to initialize this array, since the default behavior is to fill it with zeros.

[krytarowski]

uint8_t vm_mid_seed[HAX_MAX_VMS / 8] I don't like this.

We are accessing this variable through an uint64_t * pointer.. Using uint64_t does not make this code suspicious (if not undefined behavior). Also I believe that the original version with uint8_t was buggy.

[raphaelning]

@krytarowski Good point. I did notice that hax_test_and_set_bit() and hax_test_and_clear_bit() both take a uint64_t * base address, but I thought they would convert it to uint8_t * underneath, which I just realized is not the case for Windows and Linux. Maybe we should make those functions take uint8_t * and handle large bit offsets (e.g. >= 64). But for now, let's scratch the idea of using an uint8_t array.

[raphaelning]

@insidiaGithub Thanks for the update, but could you amend the first commit and revert the renaming of vm_mid_bits (see my previous comment at #141 (comment)) In addition, please feel free to incorporate my suggested change into your patch, instead of creating a separate commit.

[insidiaGithub]

@insidiaGithub Thanks for the update, but could you amend the first commit and revert the renaming of vm_mid_bits (see my previous comment at #141 (comment)) In addition, please feel free to incorporate my suggested change into your patch, instead of creating a separate commit.

Done. thank you

[raphaelning]

Thanks!

[krytarowski]

Maybe we should make those functions take uint8_t * and handle large bit offsets (e.g. >= 64).

I don't think that this looks sane, atomic operations behave well on registers, not arrays (for arrays we might want mutexes). I've noted this issue during my NetBSD porting effort, but rescheduled to be presented after merging my code. Thankfully it was fixed here indirectly.

[raphaelning]

atomic operations behave well on registers, not arrays (for arrays we might want mutexes)

True. I was thinking about re-implementing them with the BT/BTS/BTC instructions, which are able to work with a very large range of bit offsets when the destination operand is in memory, but I just realized that they can't be used with the LOCK prefix...