qemu-system-x86_64 appears to have sgx-epc, but not memory-backend-epc

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

--> {"execute": "qom-list-types", "arguments": {"implements": "memory-backend"}}
<-- {"return": [{"name": "memory-backend-ram", "parent": "memory-backend"}, {"name": "memory-backend-file", "parent": "memory-backend"}, {"name": "memory-backend-memfd", "parent": "memory-backend"}, {"name": "memory-backend-shm", "parent": "memory-backend"}]}

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

Quoted 30 lines · Daniel P. Berrangé

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Quoted 3 lines

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

TYPE_MEMORY_BACKEND_MEMFD does the same. I did a quick grep for "calls to type_register not indented by exactly 4 spaces" and I think the other cases are "misindented" or "data-driven type registration" rather than trying to do things conditionally (but I didn't actually look at all of them, instead making guesses based on variable names, so maybe there's one or two more I missed).

I guess maybe this is trying to work around the fact that the memory backend API doesn't give a backend any place to say "this is never going to work" early, because instance_init isn't allowed to fail, these objects aren't TYPE_DEVICE so they don't have "realize", and so the only other option is "fail every alloc call" ?

The memory backends are all implementing the UserCreatable interface, which provides a "complete" method where they could gracefully report lack of the feature via 'errp'.

Quoted 3 lines · Daniel P. Berrangé

The memory backends are all implementing the UserCreatable interface, which provides a "complete" method where they could gracefully report lack of the feature via 'errp'.

I didn't realize UserCreatable was a thing. It seems unfortunate that we have accumulated three different kinds of object creation/destruction life cycle:

- instance_init / instance_finalize

- instance_init / realize / unrealize / instance_finalize
- instance_init / complete / prepare_delete / instance_finalize

Quoted 10 lines · Peter Maydell

Quoted 3 lines · Daniel P. Berrangé

The memory backends are all implementing the UserCreatable interface, which provides a "complete" method where they could gracefully report lack of the feature via 'errp'.

I didn't realize UserCreatable was a thing. It seems unfortunate that we have accumulated three different kinds of object creation/destruction life cycle:

- instance_init / instance_finalize

This is all stuff that is supposed to be basic self contanied initialization/cleanup with no side effects or dependencies, and impossible to fail, which is pretty clear semantics. During init, no external properties have been set yet, and during finalize it is already disconnected from the QOM tree. Essentially these should be basic memory allocs/free, mutex inits/destroys, setting default int values if non-zero, etc.

Quoted 2 lines

- instance_init / realize / unrealize / instance_finalize
- instance_init / complete / prepare_delete / instance_finalize

These two have clear overlap in that they both take place after user specified properties have been set, and deal with types of initialization/setup/cleanup that can fail and have external depedencies.

UserCreatable has another aspect in that it is the witness flag for whether a QOM type is permissible to create using -object / object_add.

I've periodically wondered if we could make them the same, but the realize/unrealize phases have significantly more complexity and dependancies than the complete/prepare_delete and most notably, 'realize' has a 'bus' parameter.

I think that an approach to unification would end up with the 'Device' class implementing UserCreatable and providing a 'complete' method which in turn called the 'realize' method. IOW, nothing much would really change in terms of impls, as all Device subclasses would carry on using realize/unrealize. It would merely make Device tree available to -object/object_add. There are probably quite a few edge case problems hiding in there though if one were to actually try to do this, so the pragmatic option is really to ignore this all and accept the different APIs for Device vs non-Device QOM.

Quoted 3 lines · Daniel P. Berrangé

The memory backends are all implementing the UserCreatable interface, which provides a "complete" method where they could gracefully report lack of the feature via 'errp'.

I didn't realize UserCreatable was a thing. It seems unfortunate that we have accumulated three different kinds of object creation/destruction life cycle:

- instance_init / instance_finalize

- instance_init / realize / unrealize / instance_finalize

- instance_init / complete / prepare_delete / instance_finalize

Quoted 43 lines · Peter Maydell

Quoted 30 lines · Daniel P. Berrangé

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Quoted 3 lines

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

TYPE_MEMORY_BACKEND_MEMFD does the same. I did a quick grep for "calls to type_register not indented by exactly 4 spaces" and I think the other cases are "misindented" or "data-driven type registration" rather than trying to do things conditionally (but I didn't actually look at all of them, instead making guesses based on variable names, so maybe there's one or two more I missed).

Quoted 6 lines

I guess maybe this is trying to work around the fact that the memory backend API doesn't give a backend any place to say "this is never going to work" early, because instance_init isn't allowed to fail, these objects aren't TYPE_DEVICE so they don't have "realize", and so the only other option is "fail every alloc call" ?

The memory backends are all implementing the UserCreatable interface, which provides a "complete" method where they could gracefully report lack of the feature via 'errp'.

This is not obvious since UserCreatable is implemented in the parent base class, but the subclasses could provide their own 'complete' impl and chain up the parent impl

Quoted 51 lines · Daniel P. Berrangé

Quoted 43 lines · Peter Maydell

Quoted 30 lines · Daniel P. Berrangé

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Quoted 3 lines

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

TYPE_MEMORY_BACKEND_MEMFD does the same. I did a quick grep for "calls to type_register not indented by exactly 4 spaces" and I think the other cases are "misindented" or "data-driven type registration" rather than trying to do things conditionally (but I didn't actually look at all of them, instead making guesses based on variable names, so maybe there's one or two more I missed).

Quoted 6 lines

I guess maybe this is trying to work around the fact that the memory backend API doesn't give a backend any place to say "this is never going to work" early, because instance_init isn't allowed to fail, these objects aren't TYPE_DEVICE so they don't have "realize", and so the only other option is "fail every alloc call" ?

Quoted 3 lines

The memory backends are all implementing the UserCreatable interface, which provides a "complete" method where they could gracefully report lack of the feature via 'errp'.

Yes.

Quoted 3 lines

This is not obvious since UserCreatable is implemented in the parent base class, but the subclasses could provide their own 'complete' impl and chain up the parent impl

David, Igor, can you clean this up? Or would you like me to try?

Quoted 30 lines · Daniel P. Berrangé

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Quoted 3 lines

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

TYPE_MEMORY_BACKEND_MEMFD does the same. I did a quick grep for "calls to type_register not indented by exactly 4 spaces" and I think the other cases are "misindented" or "data-driven type registration" rather than trying to do things conditionally (but I didn't actually look at all of them, instead making guesses based on variable names, so maybe there's one or two more I missed).

Quoted 35 lines · Peter Maydell

Quoted 30 lines · Daniel P. Berrangé

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Quoted 3 lines

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

Quoted 7 lines

TYPE_MEMORY_BACKEND_MEMFD does the same. I did a quick grep for "calls to type_register not indented by exactly 4 spaces" and I think the other cases are "misindented" or "data-driven type registration" rather than trying to do things conditionally (but I didn't actually look at all of them, instead making guesses based on variable names, so maybe there's one or two more I missed).

Alright, I looked. In addition to "misindented" or "data-driven", I found:

* register_types() in hostmem-memfd.c registers "memory-backend-memfd" only when a test of memfd_create() succeeds. This is what Peter reported.

* virtio_ccw_gpu_register() in hw/s390x/virtio-ccw-gpu.c registers "virtio-gpu-ccw" only when have_virtio_ccw, which is true when hw/s390x/virtio-ccw.c gets compiled into the binary. So it's effectively compile-time conditional.

I wish it was literally compile-time conditional, either in meson or using #if.

* qemu_console_early_init() in ui/console-vc.c registers "chardev-vc" if it's not already registered. early_dbus_init() in ui/dbus.c, early_gtk_display_init() in ui/gtk.c, and spice_app_display_early_init() in ui/spice-app.c all register it.

These are all QemuDisplay .early_init() callbacks. qemu_display_early_init() calls the selected display's .early_init(), if non-null.

Because qemu_console_early_init() runs after qemu_display_early_init(), this always registers a "chardev-vc": the active display may register its own, and when it doesn't, we get ui/console-vc.c's.

More moving parts than I'd like.

I'm going to post patches for backends/hostmem-{epc,memfd}.c. I don't plan to clean up the other two at this time.

[...]

Quoted 52 lines · Markus Armbruster

Quoted 35 lines · Peter Maydell

Quoted 30 lines · Daniel P. Berrangé

Quoted 5 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

Quoted 8 lines

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Quoted 3 lines

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?

Quoted 7 lines

TYPE_MEMORY_BACKEND_MEMFD does the same. I did a quick grep for "calls to type_register not indented by exactly 4 spaces" and I think the other cases are "misindented" or "data-driven type registration" rather than trying to do things conditionally (but I didn't actually look at all of them, instead making guesses based on variable names, so maybe there's one or two more I missed).

Alright, I looked. In addition to "misindented" or "data-driven", I found:

* register_types() in hostmem-memfd.c registers "memory-backend-memfd" only when a test of memfd_create() succeeds. This is what Peter reported.

Quoted 7 lines

* virtio_ccw_gpu_register() in hw/s390x/virtio-ccw-gpu.c registers "virtio-gpu-ccw" only when have_virtio_ccw, which is true when hw/s390x/virtio-ccw.c gets compiled into the binary. So it's effectively compile-time conditional.

I wish it was literally compile-time conditional, either in meson or using #if.

That looks like it is dead code to me. hw/s390x/meson.build does

hw_s390x_modules = {}
virtio_gpu_ccw_ss = ss.source_set() virtio_gpu_ccw_ss.add(when: ['CONFIG_VIRTIO_GPU', 'CONFIG_VIRTIO_CCW'],

                        if_true: [files('virtio-ccw-gpu.c'), pixman])

hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}

it doesn't look like 'have_virtio_ccw' can ever be false, due to the CONFIG_VIRTIO_CCW Kconfig check ?

Quoted 9 lines · Markus Armbruster

My build of qemu-system-x86_64 has device sgx-epc:

--> {"execute": "qom-list-types", "arguments": {"implements": "sgx-epc"}} <-- {"return": [{"name": "sgx-epc", "parent": "device"}]}

The device as a link property to a memory-backend-epc:

--> {"execute": "qom-list-properties", "arguments": {"typename": "sgx-epc"}} <-- {"return": [...

Quoted 7 lines

                 {"name": "memdev", "type": "link<memory-backend-epc>"},
                 ...]}

However, type memory-backend-epc does not exist:

--> {"execute": "qom-list-types", "arguments": {"implements": "memory-backend"}}
<-- {"return": [{"name": "memory-backend-ram", "parent": "memory-backend"}, {"name": "memory-backend-file", "parent": "memory-backend"}, {"name": "memory-backend-memfd", "parent": "memory-backend"}, {"name": "memory-backend-shm", "parent": "memory-backend"}]}

There is some rather dubious code that conditionally registers the type

static void register_types(void)

{
    int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
    if (fd >= 0) {
        close(fd);

        type_register_static(&sgx_epc_backed_info);
    }
}

Normal practice is for types to be unconditionally registered. Do we have any examples outside of this SGX case where we conditionally
register types ? Should we change this to follow normal practice ?