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@xxxxxxxxx>
---
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
Attachment:
signature.asc
Description: PGP signature
