Re: [PATCH v6 2/4] Add NVMM accelerator: x86 CPU support

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/04/21 14:32, Reinoud Zandijk wrote:
> On Thu, Apr 01, 2021 at 10:35:40AM +0200, Paolo Bonzini wrote:
>> On 31/03/21 22:07, Reinoud Zandijk wrote:
>>> +void nvmm_vcpu_kick(CPUState *cpu);
>>
>> Not defined anywhere.
> 
> Hmmm, indeed. I think its a leftover of the former patch. Good catch.
> 
>>> +{
>>> +#if NVMM_USER_VERSION == 1
>>> +    struct sigaction sigact;
>>> +    sigset_t set;
>>> +
>>> +    /* Install the IPI handler. */
>>> +    memset(&sigact, 0, sizeof(sigact));
>>> +    sigact.sa_handler = nvmm_ipi_signal;
>>> +    sigaction(SIG_IPI, &sigact, NULL);
>>> +
>>> +    /* Allow IPIs on the current thread. */
>>> +    sigprocmask(SIG_BLOCK, NULL, &set);
>>> +    sigdelset(&set, SIG_IPI);
>>> +    pthread_sigmask(SIG_SETMASK, &set, NULL);
>>> +#else
>>> +    /*
>>> +     * We use the nvmm_vcpu_stop() mechanism, and don't use signals.
>>> +     * Nothing to do.
>>> +     */
>>> +#endif
>>
>> Since nvmm_vcpu_stop is very similar to KVM's immediate_exit mechanism, I
>> think you still need to have a dummy signal handler to kick the VM out of
>> the run loop *if it is in the kernel*.  The signal handler however can just
>> do nothing.
> 
> Are you worried the in-kernel thread will somehow get stuck or halt on exit of
> Qemu and left as a zombie?

No, you need all three of these:

- the signal to interrupt a thread that is running the VM

- the exit_request field to interrupt a thread that is running QEMU code

- nvmm_vcpu_stop() to interrupt a thread that is running kernel code but 
has not yet started running the VM.

>> Also, can you just drop support for NVMM_USER_VERSION == 1?
> 
> Now thats a good suggestion. We could add support for it in the pkgsrc
> package. When 9.0 gets retired, we could then retire it there without the need
> to patch Qemu again.

If it's still in use in the wild I have no problem keeping it.  The 
difference is small; I was just asking.

>>> diff --git a/target/i386/nvmm/meson.build b/target/i386/nvmm/meson.build
>>> new file mode 100644
>>> index 0000000000..c154e78014
>>> --- /dev/null
>>> +++ b/target/i386/nvmm/meson.build
>>> @@ -0,0 +1,4 @@
>>> +i386_softmmu_ss.add(when: 'CONFIG_NVMM', if_true: files(
>>> +  'nvmm-all.c',
>>> +  'nvmm-accel-ops.c',
>>> +))
>>
>> The nvmm library should be added here.
> 
> I am not sure what you mean by that. You provided a patch for the meson.build
> file, will that not suffice?

That patch dropped nvmm from meson.build, here is where it should be 
added (so that it is conditional on CONFIG_NVMM and qemu-system-arm does 
not link to libnvmm).

Paolo