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
