Re: [Qemu-devel] [PATCH v5 4/6] s390x/vfio: ap: Introduce VFIO AP device

[Tony Krowiak <akrowiak@linux.vnet.ibm.com>]

On 05/15/2018 03:55 AM, Pierre Morel wrote:
> On 14/05/2018 21:26, Tony Krowiak wrote:
>> On 05/11/2018 05:02 AM, Pierre Morel wrote:
>>> On 10/05/2018 15:10, Tony Krowiak wrote:
>>>> On 05/09/2018 10:28 AM, Halil Pasic wrote:
>>>>>
>>>>>
>>>>> On 05/08/2018 02:25 PM, Tony Krowiak wrote:
>>>>>> Introduces a VFIO based AP device. The device is defined via
>>>>>> the QEMU command line by specifying:
>>>>>>
>>>>>>      -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
>>>>>>
>>>>>> There may be only one vfio-ap device configured for a guest.
>>>>>>
>>>>>> The mediated matrix device is created by the VFIO AP device
>>>>> [..]
>>>>>> + * directory.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/vfio.h>
>>>>>> +#include <sys/ioctl.h>
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qapi/error.h"
>>>>>> +#include "hw/sysbus.h"
>>>>>> +#include "hw/vfio/vfio.h"
>>>>>> +#include "hw/vfio/vfio-common.h"
>>>>>> +#include "hw/s390x/ap-device.h"
>>>>>> +#include "qemu/error-report.h"
>>>>>> +#include "qemu/queue.h"
>>>>>> +#include "qemu/option.h"
>>>>>> +#include "qemu/config-file.h"
>>>>>> +#include "cpu.h"
>>>>>> +#include "kvm_s390x.h"
>>>>>> +#include "sysemu/sysemu.h"
>>>>>> +
>>>>>> +#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>>>>>> +
>>>>>> +typedef struct VFIOAPDevice {
>>>>>> +    APDevice apdev;
>>>>>> +    VFIODevice vdev;
>>>>>> +    QTAILQ_ENTRY(VFIOAPDevice) sibling;
>>>>>> +} VFIOAPDevice;
>>>>>> +
>>>>>> +VFIOAPDevice *vfio_apdev;
>>>>>> +
>>>>>> +static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
>>>>>> +{
>>>>>> +    vdev->needs_reset = false;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * We don't need vfio_hot_reset_multi and vfio_eoi operations for
>>>>>> + * vfio-ap-matrix device now.
>>>>>> + */
>>>>>> +struct VFIODeviceOps vfio_ap_ops = {
>>>>>> +    .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
>>>>>> +};
>>>>>> +
>>>>>
>>>>> I'm not familiar with the vfio infrastructure, but AFAIR I
>>>>> haven't seen any substantial reset handling (QEMU or kernel).
>>>>> Did I miss something?
>>>>
>>>> No, you didn't miss anything, there is no reset handling.
>>>>
>>>>>
>>>>> If I did not. I think this is a big problem. We need to at least
>>>>> zeroize the queues (e.g. on system reset)  to avoid leaking
>>>>> sensitive information. Without this, there is no sane way to use
>>>>> ap-passthrough. Or am I wrong?
>>>>
>>>> I do not have a definitive answer, I will have to look into it.
>>>> I'm thinking that since we are using ap-passthrough, the AP bus
>>>> running on the guest would be responsible for handling reset possibly
>>>> by resetting or zeroizing its queues. I'll get back to you on this.
>>>>
>>>>>
>>>>> Regards,
>>>>> Halil
>>>>>
>>>>
>>> On the host, on system reset or subsystem reset, the queues are 
>>> zeroized.
>>
>> Can you point me to where this is done?
>
> This is firmware. See documentation, it is specified that the queues 
> are zeroized on system or subsystem reset.

The sticking point for me is how does this relate to the guest running 
under SIE?

>
>
>>
>>>
>>>
>>> It means we must do the same for the guest and implement both cold 
>>> and hot reset.
>>>
>>> One of the patches in the interrupt handling serie I sent last 
>>> wednesday zeroize
>>> the queues on starting and stopping the guest so no data leak occurs 
>>> between
>>> host and guests or between guests.
>>
>> I'm sorry, I was not aware of them and consequently have not yet 
>> reviewed them.
>> I'll take a look at them ASAP.
>>
>>>
>>>
>>> However we should follow the machine specification and zeroize the 
>>> queues between
>>> two guest reset too. i.e. in the "reset" call back. and implement 
>>> the VFIO_DEVICE_RESET
>>> ioctl.
>>
>> Can you point me to the specification to which you are referring? You 
>> can send a personal
>> email if this is restricted information.
>
> OK.
>
> regards,
>
> Pierre
>
>