On 2/18/22 5:09 AM, Janis Schoetterl-Glausch wrote:
On 2/17/22 18:36, Shuah Khan wrote:
On 2/17/22 7:53 AM, Janis Schoetterl-Glausch wrote:
Introduce macro for performing MEM_OP ioctls in a concise way.
How does this help? What is the value in re-writing existing code and turning it into a macro?
I want invocations of the ioctl to be independent of each other, so the reader does not have to keep track of the state of the struct kvm_s390_mem_op.
So you have to specify all arguments manually like so, which is rather noisy and makes it hard to see what the relevant parameter is:
ksmo.gaddr = guest_mem1; ksmo.flags = 0; ksmo.size = maxsize; ksmo.op = KVM_S390_MEMOP_LOGICAL_WRITE; ksmo.buf = (uintptr_t)mem1; ksmo.ar = 17; rv = _vcpu_ioctl(vm, VCPU_ID, KVM_S390_MEM_OP, &ksmo);
Or you introduce an abstraction. Previously I used lots of functions with repeated code which got chaotic. I decided on the macro because it's more flexible, e.g. you don't have to pass default args. For example, there is only one test that passes the access register arg, so you would want to default it to 0 for all other test. For the access key argument you need to pass both a flag and the key itself, so you'd probably get rid of this redundancy also. There also might be future extensions of the ioctl that work the same way (not 100% but not purely theoretical either).
With the macro all that is orthogonal, you just pass the argument you need or you don't. With functions you'd maybe add a memop_key() variant and a _ar() variant and a _key_ar() variant if you need it (currently not necessary), doubling the number of functions with each additional argument. Another example is GADDR_V and GADDR, the first takes care of translating the address to an physical one, but sometimes you need to pass it untranslated, and we need to combine that with passing a key or not.
A big improvement was making the target of the ioctl (vm/vcpu) and the operation arguments instead of baking it into the function. Since they're mandatory arguments this is independent of the macro vs functions question.
In the end there are multiple independent but interacting improvements and it is kinda hard to make the call on how far to go along one dimension, e.g. I was unsure if I wanted to introduce the DEFAULT_READ macro, but decided for it, since, as a reviewer, you can see that it executes the same code with different arguments, instead of trying to identify the difference between 5 copy-pasted and modified lines of code. On the other hand you have the cost of introducing an indirection.
Sounds good. I am not fan of macros, however, in this case macro helps. Please split the patches so that restructuring work is done first and then the new code - as per my suggestion on the second patch.
thanks, -- Shuah