9:14 a.m.
---
src/bhyve/bhyve_command.c | 2 +-
src/bhyve/bhyve_device.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 802997bd2d..e595b3d6c1 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -329,7 +329,7 @@ static int
bhyveBuildLPCArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
virCommandPtr cmd)
{
- virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
+ virCommandAddArgList(cmd, "-s", "31,lpc", NULL);
return 0;
}
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
index 03aa6c93bd..2ed3013552 100644
--- a/src/bhyve/bhyve_device.c
+++ b/src/bhyve/bhyve_device.c
@@ -48,9 +48,9 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
if (addr->domain == 0 && addr->bus == 0) {
if (addr->slot == 0) {
return 0;
- } else if (addr->slot == 1) {
+ } else if (addr->slot == 31) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("PCI bus 0 slot 1 is reserved for the implicit "
+ _("PCI bus 0 slot 31 is reserved for the implicit "
"LPC PCI-ISA bridge"));
return -1;
}
--
2.18.0
9:17 a.m.
On Mon, Jul 09, 2018 at 05:14:35PM +0300, Ivan Mishonov wrote:
---
src/bhyve/bhyve_command.c | 2 +-
src/bhyve/bhyve_device.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 802997bd2d..e595b3d6c1 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -329,7 +329,7 @@ static int
bhyveBuildLPCArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
virCommandPtr cmd)
{
- virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
+ virCommandAddArgList(cmd, "-s", "31,lpc", NULL);
return 0;
}
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
index 03aa6c93bd..2ed3013552 100644
--- a/src/bhyve/bhyve_device.c
+++ b/src/bhyve/bhyve_device.c
@@ -48,9 +48,9 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
if (addr->domain == 0 && addr->bus == 0) {
if (addr->slot == 0) {
return 0;
- } else if (addr->slot == 1) {
+ } else if (addr->slot == 31) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("PCI bus 0 slot 1 is reserved for the implicit "
+ _("PCI bus 0 slot 31 is reserved for the implicit
"
"LPC PCI-ISA bridge"));
return -1;
}
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
but i won't push this, as Roman should double check in case there was an
important reason he chose slot 1 originally.
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 :|
9:18 a.m.
Yes, I talked to him and he said he will review and test. Thanks
On 07/09/2018 05:17 PM, Daniel P. Berrangé wrote:
On Mon, Jul 09, 2018 at 05:14:35PM +0300, Ivan Mishonov wrote:
> ---
> src/bhyve/bhyve_command.c | 2 +-
> src/bhyve/bhyve_device.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 802997bd2d..e595b3d6c1 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -329,7 +329,7 @@ static int
> bhyveBuildLPCArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
> virCommandPtr cmd)
> {
> - virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
> + virCommandAddArgList(cmd, "-s", "31,lpc", NULL);
> return 0;
> }
>
> diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
> index 03aa6c93bd..2ed3013552 100644
> --- a/src/bhyve/bhyve_device.c
> +++ b/src/bhyve/bhyve_device.c
> @@ -48,9 +48,9 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> if (addr->domain == 0 && addr->bus == 0) {
> if (addr->slot == 0) {
> return 0;
> - } else if (addr->slot == 1) {
> + } else if (addr->slot == 31) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("PCI bus 0 slot 1 is reserved for the implicit
"
> + _("PCI bus 0 slot 31 is reserved for the implicit
"
> "LPC PCI-ISA bridge"));
> return -1;
> }
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
but i won't push this, as Roman should double check in case there was an
important reason he chose slot 1 originally.
Regards,
Daniel
12:41 p.m.
Ivan Mishonov wrote:
---
src/bhyve/bhyve_command.c | 2 +-
src/bhyve/bhyve_device.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
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.
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 802997bd2d..e595b3d6c1 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -329,7 +329,7 @@ static int
bhyveBuildLPCArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
virCommandPtr cmd)
{
- virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
+ virCommandAddArgList(cmd, "-s", "31,lpc", NULL);
return 0;
}
diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c
index 03aa6c93bd..2ed3013552 100644
--- a/src/bhyve/bhyve_device.c
+++ b/src/bhyve/bhyve_device.c
@@ -48,9 +48,9 @@ bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
if (addr->domain == 0 && addr->bus == 0) {
if (addr->slot == 0) {
return 0;
- } else if (addr->slot == 1) {
+ } else if (addr->slot == 31) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("PCI bus 0 slot 1 is reserved for the implicit "
+ _("PCI bus 0 slot 31 is reserved for the implicit
"
"LPC PCI-ISA bridge"));
return -1;
}
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
--
2.18.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
2326
days inactive
2328
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Ivan Mishonov -
Roman Bogorodskiy