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

Re: [PATCH v5 2/2] bhyve: support 'isa' controller for LPC



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:
> > > Support modeling of the 'isa' controller for bhyve. User can manually
> > > define any PCI slot for the 'isa' controller, including PCI slot 1,
> > > but other devices are not allowed to use this address.
> > > 
> > > When domain configuration requires the 'isa' controller to be present,
> > > automatically add it on domain post-parse stage.
> > > 
> > > Now, as this controller is always available when needed, it's not
> > > necessary to implicitly add it to the bhyve command line, so remove
> > > bhyveBuildLPCArgStr().
> > > 
> > > Also, make bhyveDomainDefNeedsISAController() static as it's no longer
> > > used outside of bhyve_domain.c.
> > > 
> > > As more than one ISA controller is not supported by bhyve,
> > > and multiple controllers with the same index are forbidden,
> > > so forbid ISA controllers with non-zero index for bhyve.
> > > 
> > > Signed-off-by: Roman Bogorodskiy <bogorodskiy gmail com>
> > > ---
> > >  src/bhyve/bhyve_command.c                     | 27 +++++++-------
> > >  src/bhyve/bhyve_device.c                      | 23 +++++++++---
> > >  src/bhyve/bhyve_domain.c                      | 25 +++++++++++--
> > >  src/bhyve/bhyve_domain.h                      |  2 --
> > >  ...ml2argv-addr-isa-controller-on-slot-1.args | 10 ++++++
> > >  ...2argv-addr-isa-controller-on-slot-1.ldargs |  3 ++
> > >  ...xml2argv-addr-isa-controller-on-slot-1.xml | 26 ++++++++++++++
> > >  ...l2argv-addr-isa-controller-on-slot-31.args | 10 ++++++
> > >  ...argv-addr-isa-controller-on-slot-31.ldargs |  3 ++
> > >  ...ml2argv-addr-isa-controller-on-slot-31.xml | 26 ++++++++++++++
> > >  ...argv-addr-non-isa-controller-on-slot-1.xml | 23 ++++++++++++
> > >  .../bhyvexml2argv-console.args                |  2 +-
> > >  .../bhyvexml2argv-isa-controller.args         | 10 ++++++
> > >  .../bhyvexml2argv-isa-controller.ldargs       |  3 ++
> > >  .../bhyvexml2argv-isa-controller.xml          | 24 +++++++++++++
> > >  ...bhyvexml2argv-isa-multiple-controllers.xml | 25 +++++++++++++
> > >  .../bhyvexml2argv-serial-grub-nocons.args     |  2 +-
> > >  .../bhyvexml2argv-serial-grub.args            |  2 +-
> > >  .../bhyvexml2argv-serial.args                 |  2 +-
> > >  .../bhyvexml2argvdata/bhyvexml2argv-uefi.args |  4 +--
> > >  .../bhyvexml2argv-vnc-autoport.args           |  4 +--
> > >  .../bhyvexml2argv-vnc-vgaconf-io.args         |  4 +--
> > >  .../bhyvexml2argv-vnc-vgaconf-off.args        |  4 +--
> > >  .../bhyvexml2argv-vnc-vgaconf-on.args         |  4 +--
> > >  .../bhyvexml2argvdata/bhyvexml2argv-vnc.args  |  4 +--
> > >  tests/bhyvexml2argvtest.c                     |  5 +++
> > >  ...l2xmlout-addr-isa-controller-on-slot-1.xml | 36 +++++++++++++++++++
> > >  ...2xmlout-addr-isa-controller-on-slot-31.xml | 36 +++++++++++++++++++
> > >  .../bhyvexml2xmlout-console.xml               |  3 ++
> > >  .../bhyvexml2xmlout-isa-controller.xml        | 36 +++++++++++++++++++
> > >  .../bhyvexml2xmlout-serial-grub-nocons.xml    |  3 ++
> > >  .../bhyvexml2xmlout-serial-grub.xml           |  3 ++
> > >  .../bhyvexml2xmlout-serial.xml                |  3 ++
> > >  .../bhyvexml2xmlout-vnc-autoport.xml          |  3 ++
> > >  .../bhyvexml2xmlout-vnc-vgaconf-io.xml        |  3 ++
> > >  .../bhyvexml2xmlout-vnc-vgaconf-off.xml       |  3 ++
> > >  .../bhyvexml2xmlout-vnc-vgaconf-on.xml        |  3 ++
> > >  .../bhyvexml2xmlout-vnc.xml                   |  3 ++
> > >  tests/bhyvexml2xmltest.c                      |  3 ++
> > >  39 files changed, 378 insertions(+), 37 deletions(-)
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-isa-controller-on-slot-1.args
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-isa-controller-on-slot-1.ldargs
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-isa-controller-on-slot-1.xml
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-isa-controller-on-slot-31.args
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-isa-controller-on-slot-31.ldargs
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-isa-controller-on-slot-31.xml
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-non-isa-controller-on-slot-1.xml
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-isa-controller.args
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-isa-controller.ldargs
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-isa-controller.xml
> > >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-isa-multiple-controllers.xml
> > >  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-isa-controller-on-slot-1.xml
> > >  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-isa-controller-on-slot-31.xml
> > >  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-isa-controller.xml
> > > 
> > 
> > 
> > > 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.

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


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