On Thu, Sep 17, 2020 at 06:55:36PM +0400, Roman Bogorodskiy wrote:
> Daniel P. Berrangé wrote:
>
> > On Thu, Sep 17, 2020 at 05:48:38PM +0400, Roman Bogorodskiy wrote:
> > > Daniel P. Berrangé wrote:
> > >
> > > > On Wed, Sep 16, 2020 at 07:14:39PM +0400, Roman Bogorodskiy wrote:
> > > > > Daniel P. Berrangé wrote:
> > > > >
> > > > > > On Sat, Aug 01, 2020 at 10:16:57AM +0400, Roman Bogorodskiy wrote:
> > > > > > > diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
> > > > > > > index fc52280361..52a055f205 100644
> > > > > > > --- a/src/bhyve/bhyve_device.c
> > > > > > > +++ b/src/bhyve/bhyve_device.c
> > > > > > > @@ -46,10 +46,16 @@ bhyveCollectPCIAddress(virDomainDefPtr def G_GNUC_UNUSED,
> > > > > > > if (addr->slot == 0) {
> > > > > > > return 0;
> > > > > > > } else if (addr->slot == 1) {
> > > > > > > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > > > > > - _("PCI bus 0 slot 1 is reserved for the implicit "
> > > > > > > - "LPC PCI-ISA bridge"));
> > > > > > > - return -1;
> > > > > > > + if (!(device->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
> > > > > > > + device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)) {
> > > > > > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > > > > > + _("PCI bus 0 slot 1 is reserved for the implicit "
> > > > > > > + "LPC PCI-ISA bridge"));
> > > > > > > + return -1;
> > > > > > > + } else {
> > > > > > > + /* We reserve slot 1 for LPC in bhyveAssignDevicePCISlots(), so exit early */
> > > > > > > + return 0;
> > > > > > > + }
> > > > > >
> > > > > > I forgot to respond to your followup comments on v4
> > > > > > https://www.redhat.com/archives/libvir-list/2020-July/msg01761.html
> > > > > >
> > > > > > > >
> > > > > > > > IIUC, this series makes it possible to put the TPC in a different
> > > > > > > > slot, so does it still make sense to forbid use of slot 1 as a
> > > > > > > > hardcoded rule ?
> > > > > > >
> > > > > > > IIRC, the idea behind that is to give some time window for users to
> > > > > > > allow moving guests from the new version to the old one. If we allow to
> > > > > > > use slot 1, it won't be possible to move the guest to the old libvirt as
> > > > > > > it will complain slot 1 should be used only for LPC.
> > > > > >
> > > > > > If the user has decided to move their LPC to a slot != 1, then it is
> > > > > > already impossible to migrate the guest to an old libvirt.
> > > > > >
> > > > > > If the user wants to explicitly specify another device in slot 1, then
> > > > > > we should not prevent that.
> > > > > >
> > > > > > We just need to make sure that if no LPC is in the XML, and no other
> > > > > > device explicitly has slot 1, then make sure to auto-assign LPC in slot 1
> > > > > > not some other device.
> > > > >
> > > > > I've started playing with that and remembered some more corner cases.
> > > > >
> > > > > To elaborate on your example, i.e. "no explicit LPC in XML AND no other
> > > > > device explicitly on slot 1": these conditions are not specific enough to
> > > > > tell whether an LPC device will be added or not.
> > > > >
> > > > > In case if the LPC device was not explicitly specified by a user,
> > > > > the bhyve driver tries to add it if it's needed (it's required
> > > > > for serial console, bootloader, and video devices;
> > > > > see bhyveDomainDefNeedsISAController()). Otherwise a domain will start
> > > > > without the LPC device.
> > > > >
> > > > > This could lead to a case when a user starts a domain in configuration
> > > > > that does not require LPC device, but has e.g. a network device on
> > > > > a PCI controller that's auto-assigned to slot 1.
> > > > >
> > > > > Later user decides to change the configuration and adds a video device,
> > > > > which requires LPC. This will lead to addresses changes, as LPC will go
> > > > > to slot 1 and a network device's controller will go from slot 1 to slot
> > > > > 2, which could be troublesome in some guest OSes.
> > > >
> > > > First of all, lets me clear that we're talking about persistent guests
> > > > here, not transient guests.
> > > >
> > > > With a persistent guest, the PCI addresses are assigned at the time
> > > > the XML arrives in virDomainDefineXML. If nothing requires the LPC
> > > > at this time, then a NIC could get given slot 1. This is recorded in
> > > > the persistent XML.
> > > >
> > > > If the user later uses 'virsh edit' to modify the XML and add a video
> > > > device, libvirt will see that the NIC is already on slot 1. It will
> > > > thus have to give the LPC slot 2 (or whatever is free). The NIC will
> > > > not move from slot 1, as that slot is considered taken at this time.
> > > >
> > > > The same is true if using virDomainAttachDevice to add a video
> > > > card. Any modifications to the XML must never change addresses that
> > > > are currently recorded in the XML, only ever place devices in new
> > > > unused slots.
> > >
> > > Sorry, I should have stated that I was assuming that LPC always
> > > gets slot 1 assigned if it has no address explicitly assigned by the user
> > > in the XML.
> > >
> > > In my understanding, some guests are picky about what slot LPC
> > > is assigned to (and it seems that slot 1 and slot 31 are the most common
> > > safe options). In this case we're letting user to resolve it in a way
> > > they think fits better their specific needs, correct?
> > >
> > > In other words, in context of address allocation, we treat LPC like any
> > > other device, with the only difference that we start address allocation
> > > from it, so it gets slot 1 if it has no address specified and the slot 1
> > > is still free?
> >
> > Ok, so it sounds like if the user has explicitly used slot 1 for some
> > arbitrary device we should allow that, but if libvirt is assigning
> > slots, it should never use slot 1 except for the LPC
>
> Going back to my example (slightly adjusted), when the user creates a
> configuration that does not require LPC, and explicitly assigns some
> other device to slot 1, and then updates the configuration in a way that
> LPC becomes required, the LPC device will be assigned to the next free
> slot available.
>
> If that doesn't work for this specific configuration, it's fair
> to expect user to fix that assuming the user knows what they're doing by
> interfering into address allocations. The fix will be a trivial swap of
> devices between slot 1 and the slot allocated for LPC, without affecting
> other device addresses.
Yes, if libvirt is auto-assigning all addresses, then we need to "just
work", but if the user manually assigns some or all addresses, it is
their responsibility if things break.
> This looks like a logical and expected behavior to me, I'll update the
> series accordingly.
ok
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
