Re: [libvirt] [PATCH] qemu: support bootindex on vfio-ccw mdev device

On Fri, Aug 02, 2019 at 15:15:59 +0200, Boris Fiuczynski wrote: > Add support to specify a boot order on vfio-ccw passthrough devices. > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > Acked-by: Jason J. Herne <jjherne(a)linux.ibm.com&gt; > Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > --- > src/qemu/qemu_command.c | 27 +++++++++++++---- > .../hostdev-subsys-mdev-vfio-ccw-boot.args | 29 +++++++++++++++++++ > .../hostdev-subsys-mdev-vfio-ccw-boot.xml | 23 +++++++++++++++ > tests/qemuxml2argvtest.c | 4 +++ > 4 files changed, 78 insertions(+), 5 deletions(-) > create mode 100644 tests/qemuxml2argvdata/hostdev-subsys-mdev-vfio-ccw-boot.args > create mode 100644 tests/qemuxml2argvdata/hostdev-subsys-mdev-vfio-ccw-boot.xml Note that I don't currently have enough information to verify that the mdev claim is correct (e.g. why it's only with CCW devices?), but I have two formal comments:

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index fee51158a9..36138e2ccf 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -5613,6 +5613,9 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, > if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0) > goto cleanup; > > + if (dev->info->bootIndex) > + virBufferAsprintf(&buf, ",bootindex=%u", dev->info->bootIndex); > + > if (virBufferCheckError(&buf) < 0) > goto cleanup; > > @@ -5624,6 +5627,22 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def, > return ret; > } > > +static bool > +qemuHostdevSupportsBoot(virDomainHostdevDefPtr hostdev) Refactor to this function should be separate from the logic change below.

> +{ > + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { > + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; > + if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI || > + subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB || > + subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI || > + (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV && > + subsys->u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_CCW)) { This line is misaligned. Please make sure that it's clear that the above line belongs to the term in the brackets.

> + return true; > + } > + } > + return false; > +} > + > static int > qemuBuildHostdevCommandLine(virCommandPtr cmd, > const virDomainDef *def, > @@ -5638,13 +5657,11 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, > char *devstr; > > if (hostdev->info->bootIndex) { > - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || > - (subsys->type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI && > - subsys->type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB && > - subsys->type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) { > + if (!qemuHostdevSupportsBoot(hostdev)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("booting from assigned devices is only " > - "supported for PCI, USB and SCSI devices")); > + "supported for PCI, USB, SCSI and MDEV " > + "of model vfio-ccw devices")); Ideally this validation will be done in the validation callback rather than in the command line generator. (pre-existing so I don't require you to refactor this at this time). > Also this error can be probably stashed into qemuHostdevSupportsBoot. (and name it like qemuHostdevValidateBoot).

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list