| Message ID | 1525782303-16940-5-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 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
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
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 > >
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 >
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
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 > >
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
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 --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 {
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