[libvirt] [PATCH 2/4] bhyve: model PCI ISA bridge
Ján Tomko
jtomko at redhat.com
Mon Feb 11 16:27:39 UTC 2019
On Sun, Feb 10, 2019 at 07:08:53PM +0400, Roman Bogorodskiy wrote:
>bhyve(8) uses PCI ISA bridge to attach serial ports and a boot ROM.
>In the libvirt driver a PCI slot 1 was always reserved for that, and
>if a domain used serial ports or a boot ROM, then it would be added
>to the command line.
>
>However, some guests require the ISA bridge to have PCI slot other than
>1. To make things more flexible, explicitly model that in XML.
>
>So now the behavior is as follows:
>
> * The 'isa-bridge' PCI controller is added to the domain if it uses
> video or serial devices, or boot ROM
> * If user didn't assign a PCI address for that controller,
> slot 1 will reserved for it
> * If user did assign a PCI address for it, this address will be
> used, and PCI slot 1 will be free (available for reservation for
> other devices)
> * If user assigned slot 1 to other device, slot is checked. If it's
> also assigned already, then the next available PCI slot is used for
> LPC PCI-ISA bridge, and a warning about that is emitted
>
I suggests splitting this patch into:
1) Splitting out the condition for auto-adding the isa bridge
(see comment below)
2) Formatting the controller for explicitly specified
<controller type='isa'/> (with a temporary condition to skip
bhyveBuildLPCArgStr in that case)
3) Auto-adding the controller when needed
>Signed-off-by: Roman Bogorodskiy <bogorodskiy at gmail.com>
>---
> src/bhyve/bhyve_command.c | 32 +++++--------
> src/bhyve/bhyve_device.c | 47 ++++++++++++-------
> src/bhyve/bhyve_domain.c | 6 +++
> .../bhyvexml2argv-acpiapic.args | 2 +-
> ...xml2argv-addr-more-than-32-sata-disks.args | 6 +--
> ...hyvexml2argv-addr-multiple-sata-disks.args | 4 +-
> ...vexml2argv-addr-multiple-virtio-disks.args | 6 +--
> ...rgv-addr-no32devs-multiple-sata-disks.args | 6 +--
> ...l2argv-addr-no32devs-single-sata-disk.args | 4 +-
> .../bhyvexml2argv-addr-single-sata-disk.args | 4 +-
> ...bhyvexml2argv-addr-single-virtio-disk.args | 2 +-
> ...vexml2argv-addr-slot-1-and-31-not-lpc.args | 10 ++++
> ...xml2argv-addr-slot-1-and-31-not-lpc.ldargs | 3 ++
> ...yvexml2argv-addr-slot-1-and-31-not-lpc.xml | 27 +++++++++++
> .../bhyvexml2argv-addr-slot-1-not-lpc.args | 10 ++++
> .../bhyvexml2argv-addr-slot-1-not-lpc.ldargs | 3 ++
> .../bhyvexml2argv-addr-slot-1-not-lpc.xml | 27 +++++++++++
> .../bhyvexml2argvdata/bhyvexml2argv-base.args | 2 +-
> .../bhyvexml2argv-bhyveload-bootorder.args | 2 +-
> .../bhyvexml2argv-bhyveload-bootorder1.args | 2 +-
> .../bhyvexml2argv-bhyveload-bootorder3.args | 2 +-
> .../bhyvexml2argv-bhyveload-explicitargs.args | 2 +-
> .../bhyvexml2argv-commandline.args | 2 +-
> .../bhyvexml2argv-console.args | 2 +-
> .../bhyvexml2argv-cputopology.args | 2 +-
> .../bhyvexml2argv-custom-loader.args | 2 +-
> .../bhyvexml2argv-disk-cdrom-grub.args | 2 +-
> .../bhyvexml2argv-disk-cdrom.args | 2 +-
> .../bhyvexml2argv-explicit-lpc.args | 10 ++++
> .../bhyvexml2argv-explicit-lpc.ldargs | 3 ++
> .../bhyvexml2argv-explicit-lpc.xml | 26 ++++++++++
> .../bhyvexml2argv-grub-bootorder.args | 2 +-
> .../bhyvexml2argv-grub-bootorder2.args | 2 +-
> .../bhyvexml2argv-grub-defaults.args | 2 +-
> .../bhyvexml2argv-input-xhci-tablet.args | 4 +-
> .../bhyvexml2argv-localtime.args | 2 +-
> .../bhyvexml2argv-macaddr.args | 2 +-
> .../bhyvexml2argv-net-e1000.args | 2 +-
> .../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 +-
> .../bhyvexml2argv-wired.args | 2 +-
> tests/bhyvexml2argvtest.c | 3 ++
> .../bhyvexml2xmlout-acpiapic.xml | 2 +-
> ...ml2xmlout-addr-more-than-32-sata-disks.xml | 6 +--
> ...yvexml2xmlout-addr-multiple-sata-disks.xml | 4 +-
> ...exml2xmlout-addr-multiple-virtio-disks.xml | 6 +--
> ...lout-addr-no32devs-multiple-sata-disks.xml | 8 ++--
> ...2xmlout-addr-no32devs-single-sata-disk.xml | 4 +-
> .../bhyvexml2xmlout-addr-single-sata-disk.xml | 4 +-
> ...hyvexml2xmlout-addr-single-virtio-disk.xml | 2 +-
> ...exml2xmlout-addr-slot-1-and-31-not-lpc.xml | 36 ++++++++++++++
> .../bhyvexml2xmlout-addr-slot-1-not-lpc.xml | 36 ++++++++++++++
> .../bhyvexml2xmlout-base.xml | 2 +-
> .../bhyvexml2xmlout-bhyveload-bootorder.xml | 2 +-
> .../bhyvexml2xmlout-bhyveload-bootorder1.xml | 2 +-
> .../bhyvexml2xmlout-bhyveload-bootorder2.xml | 2 +-
> .../bhyvexml2xmlout-bhyveload-bootorder3.xml | 2 +-
> .../bhyvexml2xmlout-bhyveload-bootorder4.xml | 2 +-
> ...bhyvexml2xmlout-bhyveload-explicitargs.xml | 2 +-
> .../bhyvexml2xmlout-commandline.xml | 2 +-
> .../bhyvexml2xmlout-console.xml | 3 ++
> .../bhyvexml2xmlout-custom-loader.xml | 2 +-
> .../bhyvexml2xmlout-disk-cdrom-grub.xml | 2 +-
> .../bhyvexml2xmlout-disk-cdrom.xml | 2 +-
> .../bhyvexml2xmlout-explicit-lpc.xml | 36 ++++++++++++++
> .../bhyvexml2xmlout-grub-bootorder.xml | 2 +-
> .../bhyvexml2xmlout-grub-bootorder2.xml | 2 +-
> .../bhyvexml2xmlout-grub-defaults.xml | 2 +-
> .../bhyvexml2xmlout-input-xhci-tablet.xml | 4 +-
> .../bhyvexml2xmlout-localtime.xml | 2 +-
> .../bhyvexml2xmlout-macaddr.xml | 2 +-
> .../bhyvexml2xmlout-metadata.xml | 4 +-
> .../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 ++
> .../bhyvexml2xmlout-wired.xml | 2 +-
> tests/bhyvexml2xmltest.c | 3 ++
> 89 files changed, 400 insertions(+), 127 deletions(-)
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-slot-1-and-31-not-lpc.args
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-slot-1-and-31-not-lpc.ldargs
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-slot-1-and-31-not-lpc.xml
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-slot-1-not-lpc.args
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-slot-1-not-lpc.ldargs
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-addr-slot-1-not-lpc.xml
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-explicit-lpc.args
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-explicit-lpc.ldargs
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-explicit-lpc.xml
> create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-slot-1-and-31-not-lpc.xml
> create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-addr-slot-1-not-lpc.xml
> create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-explicit-lpc.xml
>
>diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
>index 1f215dac08..0842484086 100644
>--- a/src/bhyve/bhyve_command.c
>+++ b/src/bhyve/bhyve_command.c
>@@ -325,14 +325,6 @@ bhyveBuildVirtIODiskArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
> return 0;
> }
>
>-static int
>-bhyveBuildLPCArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
>- virCommandPtr cmd)
>-{
>- virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
>- return 0;
>-}
>-
> static int
> bhyveBuildGraphicsArgStr(const virDomainDef *def,
> virDomainGraphicsDefPtr graphics,
>@@ -460,7 +452,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> * vm0
> */
> size_t i;
>- bool add_lpc = false;
> int nusbcontrollers = 0;
> unsigned int nvcpus = virDomainDefGetVcpus(def);
>
>@@ -549,7 +540,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM)) {
> virCommandAddArg(cmd, "-l");
> virCommandAddArgFormat(cmd, "bootrom,%s", def->os.loader->path);
>- add_lpc = true;
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Installed bhyve binary does not support "
>@@ -563,12 +553,20 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> virDomainControllerDefPtr controller = def->controllers[i];
> switch (controller->type) {
> case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
>- if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
>- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>- "%s", _("unsupported PCI controller model: only PCI root supported"));
>- goto error;
>- }
>+ switch (controller->model) {
>+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ISA_BRIDGE:
>+ virCommandAddArg(cmd, "-s");
>+ virCommandAddArgFormat(cmd, "%d:0,lpc",
>+ controller->info.addr.pci.slot);
>+ break;
>+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> break;
>+ default:
>+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>+ "%s", _("unsupported PCI controller model: only PCI root supported"));
>+ goto error;
>+ }
>+ break;
> case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> if (bhyveBuildAHCIControllerArgStr(def, controller, conn, cmd) < 0)
> goto error;
>@@ -613,7 +611,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> if (bhyveBuildGraphicsArgStr(def, def->graphics[0], def->videos[0],
> conn, cmd, dryRun) < 0)
> goto error;
>- add_lpc = true;
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Multiple graphics devices are not supported"));
>@@ -621,9 +618,6 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> }
> }
>
>- if (add_lpc || def->nserials)
>- bhyveBuildLPCArgStr(def, cmd);
>-
> if (bhyveBuildConsoleArgStr(def, cmd) < 0)
> goto error;
>
>diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
>index 201044d9e6..d9ac1911a3 100644
>--- a/src/bhyve/bhyve_device.c
>+++ b/src/bhyve/bhyve_device.c
>@@ -43,16 +43,8 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> virDomainPCIAddressSetPtr addrs = opaque;
> virPCIDeviceAddressPtr addr = &info->addr.pci;
>
>- if (addr->domain == 0 && addr->bus == 0) {
>- 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 (addr->domain == 0 && addr->bus == 0 && addr->slot == 0)
>+ return 0;
>
> if (virDomainPCIAddressReserveAddr(addrs, addr,
> VIR_PCI_CONNECT_TYPE_PCI_DEVICE, 0) < 0) {
>@@ -92,15 +84,36 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def,
> virDomainPCIAddressSetPtr addrs)
> {
> size_t i;
>- virPCIDeviceAddress lpc_addr;
>
>- /* explicitly reserve slot 1 for LPC-ISA bridge */
>- memset(&lpc_addr, 0, sizeof(lpc_addr));
>- lpc_addr.slot = 0x1;
>+ /* Look for isa-bridge first, if it has no address assigned, we want to reserve
>+ PCI slot 1 for it before it's used by some other device */
>+ for (i = 0; i < def->ncontrollers; i++) {
>+ if ((def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) &&
>+ (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ISA_BRIDGE) &&
>+ virDeviceInfoPCIAddressIsWanted(&def->controllers[i]->info)) {
>+ virPCIDeviceAddress lpc_addr;
>+ memset(&lpc_addr, 0, sizeof(lpc_addr));
>+ lpc_addr.slot = 0x1;
>+
>+ if (virDomainPCIAddressSlotInUse(addrs, &lpc_addr)) {
>+ lpc_addr.slot = 0x1f;
>+
>+ if (virDomainPCIAddressSlotInUse(addrs, &lpc_addr)) {
>+ VIR_WARN("Cannot use PCI slots 1 and 31 for LPC PCI-ISA bridge "
>+ "as they are already reserved, using the next available "
>+ "address");
>+ continue;
>+ }
>+ }
>+
>+ if (virDomainPCIAddressReserveAddr(addrs, &lpc_addr,
>+ VIR_PCI_CONNECT_TYPE_PCI_DEVICE, 0) < 0) {
>+ goto error;
>+ }
>
This does not reserve the 0x01 address in case the domain has no ISA
bridge. Even if you do not intend to keep it reserved for future use
(i.e. keep address 0x1 free unless the user explicitly assigned a device
on that slot), consider reserving it here temporarily to reduce noise in
the test suite.
>- if (virDomainPCIAddressReserveAddr(addrs, &lpc_addr,
>- VIR_PCI_CONNECT_TYPE_PCI_DEVICE, 0) < 0) {
>- goto error;
>+ def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
>+ def->controllers[i]->info.addr.pci = lpc_addr;
>+ }
> }
>
> for (i = 0; i < def->ncontrollers; i++) {
>diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
>index 79cf103d28..6e09cbc484 100644
>--- a/src/bhyve/bhyve_domain.c
>+++ b/src/bhyve/bhyve_domain.c
>@@ -73,6 +73,12 @@ bhyveDomainDefPostParse(virDomainDefPtr def,
> VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0)
> return -1;
>
>+ if ((def->os.bootloader == NULL && def->os.loader) ||
>+ (def->nconsoles || def->nserials) || (def->ngraphics && def->nvideos))
This is the condition that could be put in a separate function first and
used for bhyveBuildLPCArgStr in a preparatory patch, then reused here.
e.g. bhyveDomainDefNeedsISAController
Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190211/9e7d7162/attachment-0001.sig>
More information about the libvir-list
mailing list