Re: [PATCH v5 15/16] KVM: Add documentation for Xen hypercall and shared_info updates

[Paolo Bonzini <pbonzini@redhat.com>]

On 28/01/21 17:49, David Woodhouse wrote:
> On Thu, 2021-01-28 at 13:18 +0100, Paolo Bonzini wrote:
>> My only qualm is really that the userspace API is really ugly.
>>
>> Can you just have both a VM and a VCPU ioctl (so no vcpu_id to pass!),
> 
> Sure, that seems like a sensible thing to do.
> 
>> add a generous padding to the struct,
> 
> I think I added *some* padding to the struct kvm_xen_hvm_attr which
> wasn't there in Joao's original. I can add more, certainly.
> 
>>   and just get everything out with a
>> single ioctl without having to pass in a type?
> 
> Honestly, I don't even care about reading it out except for long_mode
> which the kernel *does* infer for itself when the MSR is used to fill
> in the hypercall page.

What about VM migration?

> I quite like keeping them separate; they *do* get set separately, in
> response to different hypercalls from the guest. And the capabilities
> translate naturally to a given field existing or not existing; having
> another mapping of that to fields in a binary structure would be
> additional complexity.

Does it make sense to reuse the bits that you return from 
KVM_CHECK_EXTENSION as a bitset for both the get and set ioctls?  The 
struct then would be

	struct kvm_xen_attr {
		uint32_t valid;
		uint32_t lm;
		struct {
		} ...;
		uint8_t pad[nnn /* to 128 bytes */];
	};

The get ioctl would return a constant value in "valid" (different for 
the VM and VCPU ioctls of course), the set ioctl would look only at the 
fields mentioned in "valid" and error out if they're unsupported or 
invalid for VM/VCPU.  Basically the "switch" you have becomes a series 
of "if (attr->valid & ...)" statements.

Paolo