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

[Steven Sistare <steven.sistare@oracle.com>]

On 10/21/2024 10:54 AM, Peter Xu wrote:
> On Thu, Oct 17, 2024 at 08:14:03AM -0700, 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.
>>
>> 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:
> 
> Jump into another failure path's if clause might be error prone in the
> future.

I agree this is ugly, but the diff is small and easy to understand, and patch 3
makes it pretty again:

@@ -36,14 +36,8 @@ 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;

- Steve

> Maybe move the error handling out while at it, e.g.:
> 
>      ret = acc->init_machine();
>      if (ret < 0) {
>         goto fail;
>      }
>      object_set_accelerator_compat_props(acc->compat_props);
>      return 0;
> 
> fail:
>      ms->accelerator = NULL;
>      *(acc->allowed) = false;
>      object_unref(OBJECT(accel));
>      return ret;
> 
>>           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);
> 
> Might be nice to add some comment on what should be part of preinit() and
> what should be part of init_machine().
> 
> AFAIU the preinit() was about probing whether an accel is supported. Maybe
> rename it to probe()?  Then it's also clear why some accel (e.g. tcg)
> doesn't need that, because it is always supported and no probe is needed.
> 
>>   #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;
>>   
>> -- 
>> 1.8.3.1
>>
>