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