On Fri, Aug 25, 2017 at 11:41:22 +0200, Martin Kletzander wrote:
On Thu, Aug 24, 2017 at 05:12:20PM +0200, Andrea Bolognani wrote:
> We can't retrieve the isolation group of a device that's
> not present in the system. However, it's very common for
> VFs to be created late in the boot, so they might not be
> present yet when libvirtd starts, which would cause the
> guests using them to disappear.
>
> If a PCI address has already been set for the host device
> in question, we can assume that it either existed at some
> point in the past or the user is assigning addresses
> manually: in both cases, we should not error out and just
> ignore the (temporary) failure.
>
> Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1484254
>
> Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
> ---
> src/qemu/qemu_domain_address.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
Not an expert on this topic, but I did my due diligence and it makes
sense to me, so
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
Disclamer: I'm not objecting to the ACK, it's clearly better than it
was.
if that's enough for you.
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 16bf0cdf9..7f12f186b 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1012,6 +1012,18 @@ qemuDomainFillDeviceIsolationGroup(virDomainDefPtr def,
> tmp = virPCIDeviceAddressGetIOMMUGroupNum(hostAddr);
>
> if (tmp < 0) {
> + /* If there's already a PCI address assigned to the device
> + * we move on instead of erroring out.
> + *
> + * This means we still don't allow non-existing host
> + * devices to be assigned to guests; however, if the host
> + * device existed at some point in the past but no longer
> + * does, which can happen very easily when dealing with VFs,
> + * we prevent the guest from disappearing and give the user
> + * an opportunity to edit its configuration */
> + if (virDeviceInfoPCIAddressPresent(info))
> + goto skip;
> +
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Can't look up isolation group for host device
"
> "%04x:%02x:%02x.%x"),
Why on earth is this in the domain address callback? That means that
it's filled on parse. That's silly for a thing like the isolation group.
The isolation group is necessary only when you are going to start the VM
so only then it should be determined. That would prevent a bug like this
altogether.
Also there's PCI hotplug...