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

[Tony Krowiak]

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