Re: [Qemu-devel] [PATCH/RFC] KVM: s390: Add S390 configuration and control kvm device

[Christian Borntraeger]

On 01/04/14 17:12, Alexander Graf wrote:
> On 04/01/2014 05:04 PM, Christian Borntraeger wrote:
>> On 01/04/14 16:58, Alexander Graf wrote:
>>> On 04/01/2014 04:47 PM, Christian Borntraeger wrote:
>>>> From: Ekaterina Tumanova <address@hidden>
>>>>
>>>> Add KVM_DEV_TYPE_S390_CONFIG kvm device that contains
>>>> configuration and control attributes of particular vm.
>>>> The device is created by KVM_CREATE_DEVICE ioctl.
>>>> The attributes may be retrieved and stored by calling
>>>> KVM_GET_DEVICE_ATTR and KVM_SET_DEVICE_ATTR ioctls.
>>>>
>>>> Signed-off-by: Ekaterina Tumanova <address@hidden>
>>>> Signed-off-by: Christian Borntraeger <address@hidden>
>>> I don't think a device is particularly the best fit. A device can usually 
>>> be instantiated multiple times. The configuration device can only be 
>>> created once. A device also gets created by user space which enables it to 
>>> receive the fd to drive it. Your device has to be created during VM 
>>> creation.
>> I remember some discussion a year or 2 ago, and IIRC a config device
>> was actually your idea ;-) (The other idea that we had, was ONE_REG for the 
>> VM)
> 
> Omg, really? :o
> 
> A device would make sense for a specific "system information" instruction 
> trap that we handle in-kernel for whatever reason (usually because it's 
> performance critical) and some mandatory say to make sure user space always 
> creates it. And some checks to make sure it can't get created twice.

Well the device created twice problem  is also true for all existing KVM 
devices. 
The only missing piece is the check in the config device, no?

> 
> Speaking of which, why don't we just forward STSI to user space with an 
> ENABLE_CAP and handle all of this there? It's not performance critical at 
> all, right?

No, performance is not critical.
The thing is, that we definitely need the kernel to handle parts of STSI, as we 
have to provide information from the upper hipervisor (LPAR or zVM). This 
information is only available in kernel space. So in essence we could only 
forward a small subset of STSI, namely stsi3_2_2. But we still have to call 
stsi_3_2_2 in the kernel,
as 3_2_2 does contain the list of hipervisors underneath us (KVM under z/VM). 

So then only thing that we could do is to forward STSI_3_2_2 to qemu when a 
capability is set and after the kernel has filled in the upper layers. 
QEMU then has to modify the page that the kernel touched and go back. Would 
work, but needs a capability and preferably an own exit. An new ioctl or
a subcode of an ioctl (attr/group whatever) seems easier.

> 
>>> I think VM configuration is common enough to just make this a separate 
>>> interface.
>> So you propose to define a new base ioctl (e.g. VM_REG) on the vm fd, 
>> instead?
>> Seems like an easy enough change. Would you reuse the kvm_attr structure for 
>> that?
> 
> Yeah, reuse whatever we can. Basically just remove the device boilerplate - I 
> don't think it's impressively useful for a non-device.

See above, name is just a simple first user.  
The thing is, that we have to have the ioctl either define a proper namespace 
(unique groups attrs) or to make it s390 specific. The device approach does 
help us here. 

I personally dont mind which way to go, as long as Paolo is fine with the 
approach, and nobody complains about the functions being non-QOM.

Christian