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

Ivan Mishonov posted 1 patch 5 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180709141435.1857-1-ivan@conquex.com
Test syntax-check passed
src/bhyve/bhyve_command.c | 2 +-
src/bhyve/bhyve_device.c  | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
[libvirt] [PATCH] Changed LPC slot from 1 to 31
Posted by Ivan Mishonov 5 years, 8 months ago
---
 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
Re: [libvirt] [PATCH] Changed LPC slot from 1 to 31
Posted by Daniel P. Berrangé 5 years, 8 months ago
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
Re: [libvirt] [PATCH] Changed LPC slot from 1 to 31
Posted by Ivan Mishonov 5 years, 8 months ago
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
Re: [libvirt] [PATCH] Changed LPC slot from 1 to 31
Posted by Roman Bogorodskiy 5 years, 8 months ago
  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