diff mbox

[v5,4/6] s390x/vfio: ap: Introduce VFIO AP device

Message ID 1525782303-16940-5-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tony Krowiak May 8, 2018, 12:25 p.m. UTC
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
driver by writing a UUID to a sysfs attribute file (see
docs/vfio-ap.txt). The mediated matrix device will be named
after the UUID. Symbolic links to the $uuid are created in
many places, so the path to the mediated matrix device $uuid
can be specified in any of the following ways:

/sys/devices/vfio_ap/matrix/$uuid
/sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
/sys/bus/mdev/devices/$uuid
/sys/bus/mdev/drivers/vfio_mdev/$uuid

When the vfio-ap device is realized, it acquires and opens the
VFIO iommu group to which the mediated matrix device is
bound. This causes a VFIO group notification event to be
signaled. The vfio_ap device driver's group notification
handler will get called at which time the device driver
will configure the the AP devices to which the guest will
be granted access.

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 default-configs/s390x-softmmu.mak |    1 +
 hw/vfio/Makefile.objs             |    1 +
 hw/vfio/ap.c                      |  182 +++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-common.h     |    1 +
 4 files changed, 185 insertions(+), 0 deletions(-)
 create mode 100644 hw/vfio/ap.c

Comments

Halil Pasic May 9, 2018, 2:28 p.m. UTC | #1
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?

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?

Regards,
Halil
Tony Krowiak May 10, 2018, 1:10 p.m. UTC | #2
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
>
Pierre Morel May 11, 2018, 9:02 a.m. UTC | #3
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.

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.

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.


Regards,

Pierre
Halil Pasic May 11, 2018, 10:29 a.m. UTC | #4
On 05/10/2018 03:10 PM, Tony Krowiak wrote:
>> 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.


You are on the wrong track. What I'm talking about is the responsibility
of the hypervisor and not the responsibility of the guest.

Regards,
Halil
Tony Krowiak May 14, 2018, 7:18 p.m. UTC | #5
On 05/11/2018 06:29 AM, Halil Pasic wrote:
>
>
> On 05/10/2018 03:10 PM, Tony Krowiak wrote:
>>> 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.
>
> You are on the wrong track. What I'm talking about is the responsibility
> of the hypervisor and not the responsibility of the guest.

I think Pierre covered it in his reply.

>
>
> Regards,
> Halil
>
>
Tony Krowiak May 14, 2018, 7:26 p.m. UTC | #6
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?

>
>
> 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.

>
>
>
> Regards,
>
> Pierre
>
Pierre Morel May 15, 2018, 7:55 a.m. UTC | #7
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.

>
>>
>>
>> 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
Tony Krowiak May 15, 2018, 3:09 p.m. UTC | #8
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
>
>
Pierre Morel May 16, 2018, 9:09 a.m. UTC | #9
On 15/05/2018 17:09, Tony Krowiak wrote:
> 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?

No it has nothing to do with the SIE, therefor we must do it explicitly.

Regards,

Pierre
Tony Krowiak May 16, 2018, 10:43 a.m. UTC | #10
On 05/16/2018 05:09 AM, Pierre Morel wrote:
> On 15/05/2018 17:09, Tony Krowiak wrote:
>> 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?
>
> No it has nothing to do with the SIE, therefor we must do it explicitly.

It shall be done.

>
> Regards,
>
> Pierre
>
>
diff mbox

Patch

diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index 2f4bfe7..0b784b6 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -9,3 +9,4 @@  CONFIG_S390_FLIC=y
 CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
 CONFIG_VFIO_CCW=$(CONFIG_LINUX)
 CONFIG_WDT_DIAG288=y
+CONFIG_VFIO_AP=$(CONFIG_LINUX)
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index a2e7a0a..8b3f664 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -6,4 +6,5 @@  obj-$(CONFIG_SOFTMMU) += platform.o
 obj-$(CONFIG_VFIO_XGMAC) += calxeda-xgmac.o
 obj-$(CONFIG_VFIO_AMD_XGBE) += amd-xgbe.o
 obj-$(CONFIG_SOFTMMU) += spapr.o
+obj-$(CONFIG_VFIO_AP) += ap.o
 endif
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
new file mode 100644
index 0000000..54a51aa
--- /dev/null
+++ b/hw/vfio/ap.c
@@ -0,0 +1,182 @@ 
+/*
+ * VFIO based AP matrix device assignment
+ *
+ * Copyright 2018 IBM Corp.
+ * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * 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,
+};
+
+static void vfio_ap_put_device(VFIOAPDevice *vapdev)
+{
+    g_free(vapdev->vdev.name);
+    vfio_put_base_device(&vapdev->vdev);
+}
+
+static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
+{
+    char *tmp, group_path[PATH_MAX];
+    ssize_t len;
+    int groupid;
+
+    tmp = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
+    len = readlink(tmp, group_path, sizeof(group_path));
+    g_free(tmp);
+
+    if (len <= 0 || len >= sizeof(group_path)) {
+        error_setg(errp, "%s: no iommu_group found for %s",
+                   VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev);
+        return NULL;
+    }
+
+    group_path[len] = 0;
+
+    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
+        error_setg(errp, "vfio: failed to read %s", group_path);
+        return NULL;
+    }
+
+    return vfio_get_group(groupid, &address_space_memory, errp);
+}
+
+static void vfio_ap_realize(DeviceState *dev, Error **errp)
+{
+    VFIOGroup *vfio_group;
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    char *mdevid;
+    Error *local_err = NULL;
+    int ret;
+
+    /*
+     * Since a guest's matrix is configured in its entirety by the mediated
+     * matrix device and hot plug is not currently supported, there is no
+     * need to have more than one vfio-ap device. Check if a vfio-ap device
+     * has already been defined.
+     */
+    if (vfio_apdev) {
+        error_setg(&local_err, "Only one %s device is allowed",
+                   VFIO_AP_DEVICE_TYPE);
+        goto out_err;
+    }
+
+    if (!s390_has_feat(S390_FEAT_AP)) {
+        error_setg(&local_err, "AP support not enabled");
+        goto out_err;
+    }
+
+    vfio_apdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
+
+    vfio_group = vfio_ap_get_group(vfio_apdev, &local_err);
+    if (!vfio_group) {
+        goto out_err;
+    }
+
+    vfio_apdev->vdev.ops = &vfio_ap_ops;
+    vfio_apdev->vdev.type = VFIO_DEVICE_TYPE_AP;
+    mdevid = basename(vfio_apdev->vdev.sysfsdev);
+    vfio_apdev->vdev.name = g_strdup_printf("%s", mdevid);
+    vfio_apdev->vdev.dev = dev;
+
+    ret = vfio_get_device(vfio_group, mdevid, &vfio_apdev->vdev, &local_err);
+    if (ret) {
+        goto out_get_dev_err;
+    }
+
+    return;
+
+out_get_dev_err:
+    vfio_ap_put_device(vfio_apdev);
+    vfio_put_group(vfio_group);
+out_err:
+    vfio_apdev = NULL;
+    error_propagate(errp, local_err);
+}
+
+static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
+{
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
+    VFIOGroup *group = vapdev->vdev.group;
+
+    vfio_ap_put_device(vapdev);
+    vfio_put_group(group);
+    vfio_apdev = NULL;
+}
+
+static Property vfio_ap_properties[] = {
+    DEFINE_PROP_STRING("sysfsdev", VFIOAPDevice, vdev.sysfsdev),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vfio_ap_vmstate = {
+    .name = VFIO_AP_DEVICE_TYPE,
+    .unmigratable = 1,
+};
+
+static void vfio_ap_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = vfio_ap_properties;
+    dc->vmsd = &vfio_ap_vmstate;
+    dc->desc = "VFIO-based AP device assignment";
+    dc->realize = vfio_ap_realize;
+    dc->unrealize = vfio_ap_unrealize;
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo vfio_ap_info = {
+    .name = VFIO_AP_DEVICE_TYPE,
+    .parent = AP_DEVICE_TYPE,
+    .instance_size = sizeof(VFIOAPDevice),
+    .class_init = vfio_ap_class_init,
+};
+
+static void vfio_ap_type_init(void)
+{
+    type_register_static(&vfio_ap_info);
+    vfio_apdev = NULL;
+}
+
+type_init(vfio_ap_type_init)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index d936014..f29df6e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -47,6 +47,7 @@  enum {
     VFIO_DEVICE_TYPE_PCI = 0,
     VFIO_DEVICE_TYPE_PLATFORM = 1,
     VFIO_DEVICE_TYPE_CCW = 2,
+    VFIO_DEVICE_TYPE_AP = 3,
 };
 
 typedef struct VFIOMmap {