---
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Roman Bogorodskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.