Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap

From: Matthew Rosato <mjrosato@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>, Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	kvm@vger.kernel.org, libvir-list@redhat.com,
	Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH RFC 0/1] mdevctl: further config for vfio-ap
Date: Thu, 13 Jun 2019 10:25:32 -0400	[thread overview]
Message-ID: <9e3a3955-f073-7c1c-9987-a1c7c3073a55@linux.ibm.com> (raw)
In-Reply-To: <20190613155418.7fa17ed2.cohuck@redhat.com>

On 6/13/19 9:54 AM, Cornelia Huck wrote:
> On Fri, 7 Jun 2019 14:30:48 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 6/7/19 10:56 AM, Cornelia Huck wrote:
>>> On Thu, 6 Jun 2019 12:45:29 -0400
>>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>>   
>>>> On 6/6/19 10:44 AM, Cornelia Huck wrote:  
>>>>> This patch adds a very rough implementation of additional config data
>>>>> for mdev devices. The idea is to make it possible to specify some
>>>>> type-specific key=value pairs in the config file for an mdev device.
>>>>> If a device is started automatically, the device is stopped and restarted
>>>>> after applying the config.
>>>>>
>>>>> The code has still some problems, like not doing a lot of error handling
>>>>> and being ugly in general; but most importantly, I can't really test it,
>>>>> as I don't have the needed hardware. Feedback welcome; would be good to
>>>>> know if the direction is sensible in general.  
>>>>
>>>> Hi Connie,
>>>>
>>>> This is very similar to what I was looking to do in zdev (config via
>>>> key=value pairs), so I like your general approach.
>>>>
>>>> I pulled your code and took it for a spin on an LPAR with access to
>>>> crypto cards:
>>>>
>>>> # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough
>>>> # mdevctl set-additional-config <uuid> ap_adapters=0x4,0x5
>>>> # mdevctl set-additional-config <uuid> ap_domains=0x36
>>>> # mdevctl set-additional-config <uuid> ap_control_domains=0x37
>>>>
>>>> Assuming all valid inputs, this successfully creates the appropriate
>>>> mdev and what looks to be a valid mdevctl.d entry.  A subsequent reboot
>>>> successfully brings the same vfio_ap-passthrough device up again.  
>>>
>>> Cool, thanks for checking!  
>>
>> I also confirmed this. I realize this is a very early proof of concept,
>> if you will, but error handling could be an interesting endeavor in
>> the case of vfio_ap given the layers of configuration involved; 
> 
> I agree; that's why I mostly ignored it for now :)
> 
>> for
>> example:
>>
>> * The necessity for the vfio_ap module to be installed
>> * The necessity that the /sys/bus/ap/apmask and /sys/bus/ap/aqmask must
>>    be appropriately configured
> 
> What do you think about outsourcing that configuration to some
> s390-specific tool (probably something in s390-tools)? While we can
> (and should) rely on driverctl for vfio-ccw, this does not look like
> something that can be easily served by some generic tooling.
> 

+1 mdevctl should stick to manipulation of mdevs.  I think any config
layer above that isn't directly modifying mdev parameters, like in this
case the kernel ap/aq masks, should be separate (and for vfio_ap I agree
s390-tools is the right place for this -- it's on my todo list)

>>
>>>   
>>>>
>>>> Matt
>>>>  
>>>>>
>>>>> Also available at
>>>>>
>>>>> https://github.com/cohuck/mdevctl conf-data
>>>>>
>>>>> Cornelia Huck (1):
>>>>>    allow to specify additional config data
>>>>>
>>>>>   mdevctl.libexec | 25 ++++++++++++++++++++++
>>>>>   mdevctl.sbin    | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>   2 files changed, 80 insertions(+), 1 deletion(-)
>>>>>      
>>>>  
>>>   
>>
>