Re: [libvirt] [PATCH] Changed LPC slot from 1 to 31

Ivan Mishonov wrote: Hi Ivan, Thanks for your contribution! Some general comments: - For quite some time already commits should have the Signed-off line with your name (you can use '-s' flag for git commit to get that automatically injected) - The good practice is to provide some more details in the commit message why you're doing this change. Also, commit titles are usually prefixed with a subsystem name this commit touches, for example: bhyve: Change LPC slot from 1 to 31 Some guests (Windows, or whatever guests these are) require LPC on slot 31 to ... function properly (or whatever reason that is). So bind LPC to slot 31 instead of 1 which is used currently. Code specific comments below. This patch makes unittest fail. If you run 'gmake check' you'll notice one failing test: ... PASS: xml2vmxtest PASS: vmwarevertest FAIL: bhyvexml2argvtest PASS: bhyvexml2xmltest PASS: bhyveargv2xmltest ... To run this specific test and get some more verbose output you could use: VIR_TEST_DEBUG=3 ./tests/bhyvexml2argvtest This is because many tests expect lpc to use slot 1. You might manually update test data, or might regenerate it using the VIR_TEST_REGENERATE_OUTPUT environment variable (more details here: https://libvirt.org/hacking.html), but in this case it's necessary to carefully verify the result to make sure unneeded changes don't squash in. One more thing: I think this patch misses the following chunk: diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 2ed3013552..97688f5e70 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -97,7 +97,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, /* explicitly reserve slot 1 for LPC-ISA bridge */ memset(&lpc_addr, 0, sizeof(lpc_addr)); - lpc_addr.slot = 0x1; + lpc_addr.slot = 0x1f; if (virDomainPCIAddressReserveAddr(addrs, &lpc_addr, VIR_PCI_CONNECT_TYPE_PCI_DEVICE, 0) < 0) { Because now it is slot 31 that needs to be reserved. That will likely cause some more tests to fail because it changes the way address auto-assignment works. Feel free to reach me if you have any questions. Thanks, Roman Roman Bogorodskiy