Improve struct handling

[ChriMarMe]

Introduce kvm.kvmSendReceive function.

This function wraps Ioctl-function to serialize any data given. It allows the use of slices in places we use fixed size arrays in structs. Deserialization needs to be handeled in the kvm.<OpFunc>.

I implemented the use of this function for some kvm functions as proof of concept. If you like it, I will adapt all kvm functions to it.

[ChriMarMe]

I know what you mean. Some functions can be called twice: first they give information about a array size (or slice size in go terms) and the second call provides the pointer to the mem which the kernel fills.

Qemu does it that way: qemu source

With C, that is easily possible, because everything is just a bunch of arrays. not with go :(

[ChriMarMe]

Ok, my main issue is the fact, that we have const size arrays which may break under unknown circumstances (like we have now) or we have slices and need to handle them with the extra overhead, but avoid the problems of arrays.

[bobuhiro11]

Thanks for the detailed explanation of the issue!

It is true that to properly implement cases that deal with dynamically sized ioctl (such as KVM_GET_MSR_FEATURE_INDEX_LIST), serialization is necessary, as suggested here.

A few more considerations. Go's Slice, unlike Array, has an internal reference to an array. This means that if a Slice is present in a struct, all members would not be placed in contiguous memory. So if we want to use Slice as a member of MSRList, we can't simply call unsafe.Pointer(&MSRList{}), we need our own serialization.

Serialization is complex in itself, so how about implementing a function dedicated to serialization, rather than as a wrapper for ioctl?

e.g.

func serialize(arg interface{}) ([]byte, error) {
  ...
}

type MSRList struct {
  NMSRs uint32
  Indicies []uint32 // slice
}

func foo() {
  list := MSRList{}
  list.NMSRs = 3
  list.Indicies = make([]uint32, 3)
  buf, err := serial(list)
  // here the size of (buf) should be 16Bytes.
}

[ChriMarMe]

Ok, what about implementing it as an interface?

type KVMSerializer interface{
    Serialize() ([]byte, error}
}

func (m *MSRList) Serialize() ([]byte, error) {
....
}

and use it in that way:

func kvmSendReceive(fd uintptr, kvmOp uintptr, ioctlOp uint8, arg KVMSerializer) () {}

By implementing it for every struct, we avoid messing with reflections AND get it serialized.

[ChriMarMe]

Ok, ignore the shown usecase. I simply implement serialization for every struct and we call it before calling Ioctl.

I was way to fast with writing stuff down than thinking. Sorry :D

[ChriMarMe]

Ok, what do you think about this? We don't have reflections AND we have data serialization, so we can use slices. Downside: We have to implement Serialize and Deserialize on every struct.

(edit: every struct with arrays/slices)

[ChriMarMe]

I suspect TravicCI plays with the infra again. Rerun the tests, they should pass. At least they do locally.

[ChriMarMe]

We only need to use Serialize and Deserialize on structs with arrays/slices.

[bobuhiro11]

Ok, what do you think about this? We don't have reflections AND we have data serialization, so we can use slices. Downside: We have to implement Serialize and Deserialize on every struct. (edit: every struct with arrays/slices) We only need to use Serialize and Deserialize on structs with arrays/slices.

It seems good idea. I like it because it is concise and clear.

I suspect TravicCI plays with the infra again. Rerun the tests, they should pass. At least they do locally.

Really? On my local machine it fails as well as Travis CI.

    machine_test.go:82: exit status 1
2023/03/03 06:35:10 Load elf segment @0x1000000 from file 0x200000 0xbbeeb8 bytes
2023/03/03 06:35:10 Load elf segment @0x1c00000 from file 0xe00000 0x17c000 bytes
2023/03/03 06:35:10 Load elf segment @0x1d7c000 from file 0x1000000 0x28f90 bytes
2023/03/03 06:35:10 Load elf segment @0x1da5000 from file 0x11a5000 0x9c000 bytes
panic: unexpected kvm exit reason: EXITSHUTDOWN

And this seems to be solved by making the following changes.

diff --git a/kvm/cpuid.go b/kvm/cpuid.go
index b8ce1cf9d9ef..c0d9c8a77e35 100644
--- a/kvm/cpuid.go
+++ b/kvm/cpuid.go
@@ -51,7 +51,7 @@ func (c *CPUID) Deserialize(data []byte) error {

        c.Entries = make([]CPUIDEntry2, c.Nent)

-       if err := binary.Read(&buf, binary.LittleEndian, c.Entries); err != nil && !errors.Is(err, io.EOF) {
+       if err := binary.Read(&buf, binary.LittleEndian, &c.Entries); err != nil && !errors.Is(err, io.EOF) {
                return err
        }

@@ -98,9 +98,14 @@ func GetSupportedCPUID(kvmFd uintptr, kvmCPUID *CPUID) error {
 // individual vCPUs. This seems odd, but in fact lets code tailor CPUID entries
 // as needed.
 func SetCPUID2(vcpuFd uintptr, kvmCPUID *CPUID) error {
-       _, err := Ioctl(vcpuFd,
+       data, err := kvmCPUID.Serialize()
+       if err != nil {
+               return err
+       }
+
+       _, err = Ioctl(vcpuFd,
                IIOW(kvmSetCPUID2, unsafe.Sizeof(kvmCPUID)),
-               uintptr(unsafe.Pointer(kvmCPUID)))
+               uintptr(unsafe.Pointer(&data[0])))

        return err
 }
diff --git a/kvm/kvm_test.go b/kvm/kvm_test.go
index 93d938738421..8952635ffc42 100644
--- a/kvm/kvm_test.go
+++ b/kvm/kvm_test.go
@@ -86,8 +86,10 @@ func TestCPUID(t *testing.T) {
                t.Fatal(err)
        }

-       CPUID := kvm.CPUID{}
-       CPUID.Nent = 100
+       CPUID := kvm.CPUID{
+               Nent:    100,
+               Entries: make([]kvm.CPUIDEntry2, 100),
+       }

        if err := kvm.GetSupportedCPUID(devKVM.Fd(), &CPUID); err != nil {
                t.Fatal(err)
diff --git a/machine/machine.go b/machine/machine.go
index 36fe4d1190df..5bc9806d3821 100644
--- a/machine/machine.go
+++ b/machine/machine.go
@@ -583,8 +583,10 @@ func (m *Machine) initSregs(vcpufd uintptr, amd64 bool) error {
 }

 func (m *Machine) initCPUID(cpu int) error {
-       cpuid := kvm.CPUID{}
-       cpuid.Nent = 100
+       cpuid := kvm.CPUID{
+               Nent:    100,
+               Entries: make([]kvm.CPUIDEntry2, 100),
+       }

        if err := kvm.GetSupportedCPUID(m.kvmFd, &cpuid); err != nil {
                return err

[ChriMarMe]

I prolly lost focus and forgot to adapt stuff. Sorry for the hassle.

[ChriMarMe]

And thank you for the review.

[ChriMarMe]

I reported the profile. Seems like phishing or maleware.