[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Prevent isolation group-related guest disappearance



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

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]