Re: [RFC V1 02/14] accel: accel preinit function

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/17/24 17:14, Steve Sistare wrote:
> Extract the first part of the AccelState init_machine function into
> a new function preinit, which can be called without knowing any
> machine properties.  For now call preinit and init_machine at the
> same place, so no functional change.

For KVM I would also include

     missing_cap = kvm_check_extension_list(s, kvm_required_capabilites);
     if (!missing_cap) {
         missing_cap =
             kvm_check_extension_list(s, kvm_arch_required_capabilities);
     }

With this change, patches 1-3 are okay as a separate submission.

Paolo

> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   accel/accel-system.c        |  6 +++++
>   accel/kvm/kvm-all.c         | 58 +++++++++++++++++++++++++++++----------------
>   accel/xen/xen-all.c         | 11 ++++++---
>   include/qemu/accel.h        |  1 +
>   target/i386/nvmm/nvmm-all.c | 10 +++++++-
>   target/i386/whpx/whpx-all.c | 14 +++++++----
>   6 files changed, 70 insertions(+), 30 deletions(-)
> 
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947d..fef6625 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -36,8 +36,14 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>       int ret;
>       ms->accelerator = accel;
>       *(acc->allowed) = true;
> +    ret = acc->preinit(accel);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
>       ret = acc->init_machine(ms);
>       if (ret < 0) {
> +fail:
>           ms->accelerator = NULL;
>           *(acc->allowed) = false;
>           object_unref(OBJECT(accel));
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 905fb84..c7c6001 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2484,6 +2484,42 @@ static int kvm_setup_dirty_ring(KVMState *s)
>       return 0;
>   }
>   
> +static int kvm_preinit(AccelState *accel)
> +{
> +    int ret;
> +    KVMState *s = KVM_STATE(accel);
> +
> +    s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> +    if (s->fd == -1) {
> +        error_report("Could not access KVM kernel module: %m");
> +        ret = -errno;
> +        goto err;
> +    }
> +
> +    ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> +    if (ret < KVM_API_VERSION) {
> +        if (ret >= 0) {
> +            ret = -EINVAL;
> +        }
> +        fprintf(stderr, "kvm version too old\n");
> +        goto err;
> +    }
> +
> +    if (ret > KVM_API_VERSION) {
> +        ret = -EINVAL;
> +        error_report("kvm version not supported");
> +        goto err;
> +    }
> +    return 0;
> +
> +err:
> +    assert(ret < 0);
> +    if (s->fd != -1) {
> +        close(s->fd);
> +    }
> +    return ret;
> +}
> +
>   static int kvm_init(MachineState *ms)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -2523,27 +2559,6 @@ static int kvm_init(MachineState *ms)
>       QTAILQ_INIT(&s->kvm_sw_breakpoints);
>   #endif
>       QLIST_INIT(&s->kvm_parked_vcpus);
> -    s->fd = qemu_open_old(s->device ?: "/dev/kvm", O_RDWR);
> -    if (s->fd == -1) {
> -        error_report("Could not access KVM kernel module: %m");
> -        ret = -errno;
> -        goto err;
> -    }
> -
> -    ret = kvm_ioctl(s, KVM_GET_API_VERSION, 0);
> -    if (ret < KVM_API_VERSION) {
> -        if (ret >= 0) {
> -            ret = -EINVAL;
> -        }
> -        error_report("kvm version too old");
> -        goto err;
> -    }
> -
> -    if (ret > KVM_API_VERSION) {
> -        ret = -EINVAL;
> -        error_report("kvm version not supported");
> -        goto err;
> -    }
>   
>       kvm_supported_memory_attributes = kvm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
>       kvm_guest_memfd_supported =
> @@ -3891,6 +3906,7 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
>   {
>       AccelClass *ac = ACCEL_CLASS(oc);
>       ac->name = "KVM";
> +    ac->preinit = kvm_preinit;
>       ac->init_machine = kvm_init;
>       ac->has_memory = kvm_accel_has_memory;
>       ac->allowed = &kvm_allowed;
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 0bdefce..dfcb90c 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -75,10 +75,8 @@ static void xen_setup_post(MachineState *ms, AccelState *accel)
>       }
>   }
>   
> -static int xen_init(MachineState *ms)
> +static int xen_preinit(AccelState *accel)
>   {
> -    MachineClass *mc = MACHINE_GET_CLASS(ms);
> -
>       xen_xc = xc_interface_open(0, 0, 0);
>       if (xen_xc == NULL) {
>           xen_pv_printf(NULL, 0, "can't open xen interface\n");
> @@ -97,6 +95,12 @@ static int xen_init(MachineState *ms)
>           xc_interface_close(xen_xc);
>           return -1;
>       }
> +    return 0;
> +}
> +
> +static int xen_init(MachineState *ms)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>   
>       /*
>        * The XenStore write would fail when running restricted so don't attempt
> @@ -125,6 +129,7 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>       };
>   
>       ac->name = "Xen";
> +    ac->preinit = xen_preinit;
>       ac->init_machine = xen_init;
>       ac->setup_post = xen_setup_post;
>       ac->allowed = &xen_allowed;
> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
> index 972a849..6b3b1cf 100644
> --- a/include/qemu/accel.h
> +++ b/include/qemu/accel.h
> @@ -37,6 +37,7 @@ typedef struct AccelClass {
>       /*< public >*/
>   
>       const char *name;
> +    int (*preinit)(AccelState *accel);
>       int (*init_machine)(MachineState *ms);
>   #ifndef CONFIG_USER_ONLY
>       void (*setup_post)(MachineState *ms, AccelState *accel);
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 65768ac..ce858a0 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -1153,7 +1153,7 @@ static struct RAMBlockNotifier nvmm_ram_notifier = {
>   /* -------------------------------------------------------------------------- */
>   
>   static int
> -nvmm_accel_init(MachineState *ms)
> +nvmm_accel_preinit(MachineState *ms)
>   {
>       int ret, err;
>   
> @@ -1178,6 +1178,13 @@ nvmm_accel_init(MachineState *ms)
>           error_report("NVMM: Wrong state size %u", qemu_mach.cap.state_size);
>           return -EPROGMISMATCH;
>       }
> +    return 0;
> +}
> +
> +static int
> +nvmm_accel_init(MachineState *ms)
> +{
> +    int ret, err;
>   
>       ret = nvmm_machine_create(&qemu_mach.mach);
>       if (ret == -1) {
> @@ -1204,6 +1211,7 @@ nvmm_accel_class_init(ObjectClass *oc, void *data)
>   {
>       AccelClass *ac = ACCEL_CLASS(oc);
>       ac->name = "NVMM";
> +    ac->preinit = nvmm_accel_preinit;
>       ac->init_machine = nvmm_accel_init;
>       ac->allowed = &nvmm_allowed;
>   }
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index a6674a8..50bfc19 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -2516,6 +2516,14 @@ static void whpx_set_kernel_irqchip(Object *obj, Visitor *v,
>    * Partition support
>    */
>   
> +static int whpx_accel_preinit(AccelState *accel)
> +{
> +    if (!init_whp_dispatch()) {
> +        return -ENOSYS;
> +    }
> +    return 0;
> +}
> +
>   static int whpx_accel_init(MachineState *ms)
>   {
>       struct whpx_state *whpx;
> @@ -2529,11 +2537,6 @@ static int whpx_accel_init(MachineState *ms)
>   
>       whpx = &whpx_global;
>   
> -    if (!init_whp_dispatch()) {
> -        ret = -ENOSYS;
> -        goto error;
> -    }
> -
>       whpx->mem_quota = ms->ram_size;
>   
>       hr = whp_dispatch.WHvGetCapability(
> @@ -2713,6 +2716,7 @@ static void whpx_accel_class_init(ObjectClass *oc, void *data)
>   {
>       AccelClass *ac = ACCEL_CLASS(oc);
>       ac->name = "WHPX";
> +    ac->preinit = whpx_accel_preinit;
>       ac->init_machine = whpx_accel_init;
>       ac->allowed = &whpx_allowed;
>