1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x

Thomas Huth posted 1 patch 5 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1551438626-12773-1-git-send-email-thuth@redhat.com
tools/virt-host-validate-common.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
[libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
Posted by Thomas Huth 5 years, 1 month ago 
When running virt-host-validate on an s390x host, the tool currently warns
that it is "Unknown if this platform has IOMMU support". We can use the
common check for entries in /sys/kernel/iommu_groups here, too, but it only
makes sense to check it if there are also PCI devices available. It's also
common on s390x that there are no PCI devices assigned to the LPAR, and in
that case there is no need for the PCI-related IOMMU, so without PCI devices
we should simply skip this test.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2:
 - Use virDirOpen() + virDirRead() instead of stat() - this should hopefully
   work now as expected.

 tools/virt-host-validate-common.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
index 73e3bdb..e27b558 100644
--- a/tools/virt-host-validate-common.c
+++ b/tools/virt-host-validate-common.c
@@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
     virBitmapPtr flags;
     struct stat sb;
     const char *bootarg = NULL;
-    bool isAMD = false, isIntel = false, isPPC = false;
+    bool isAMD = false, isIntel = false;
+    virArch arch = virArchFromHost();
+    struct dirent *dent;
+    DIR *dir;
+    int rc;
+
     flags = virHostValidateGetCPUFlags();
 
     if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX))
@@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
 
     virBitmapFree(flags);
 
-    isPPC = ARCH_IS_PPC64(virArchFromHost());
-
     if (isIntel) {
         virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support"));
         if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
@@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
                            "hardware platform");
             return -1;
         }
-    } else if (isPPC) {
+    } else if (ARCH_IS_PPC64(arch)) {
         /* Empty Block */
+    } else if (ARCH_IS_S390(arch)) {
+        /* On s390x, we skip the IOMMU check if there are no PCI devices
+         * (which is quite usual on s390x) */
+        if (!virDirOpen(&dir, "/sys/bus/pci/devices"))
+            return 0;
+        rc = virDirRead(dir, &dent, NULL);
+        VIR_DIR_CLOSE(dir);
+        if (rc <= 0)
+            return 0;
     } else {
         virHostMsgFail(level,
                        "Unknown if this platform has IOMMU support");
@@ -391,7 +403,7 @@ int virHostValidateIOMMU(const char *hvname,
 
     virHostMsgCheck(hvname, "%s", _("if IOMMU is enabled by kernel"));
     if (sb.st_nlink <= 2) {
-        if (!isPPC)
+        if (bootarg)
             virHostMsgFail(level,
                            "IOMMU appears to be disabled in kernel. "
                            "Add %s to kernel cmdline arguments", bootarg);
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
Posted by Bjoern Walk 5 years, 1 month ago 
Thomas Huth <thuth@redhat.com> [2019-03-01, 12:10PM +0100]:
> When running virt-host-validate on an s390x host, the tool currently warns
> that it is "Unknown if this platform has IOMMU support". We can use the
> common check for entries in /sys/kernel/iommu_groups here, too, but it only
> makes sense to check it if there are also PCI devices available. It's also
> common on s390x that there are no PCI devices assigned to the LPAR, and in
> that case there is no need for the PCI-related IOMMU, so without PCI devices
> we should simply skip this test.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Use virDirOpen() + virDirRead() instead of stat() - this should hopefully
>    work now as expected.

This seems to work fine.

Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
Posted by Boris Fiuczynski 5 years, 1 month ago 
Hi Thomas,
looks good and also tried it out on s390x.

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>


On 3/1/19 12:10 PM, Thomas Huth wrote:
> When running virt-host-validate on an s390x host, the tool currently warns
> that it is "Unknown if this platform has IOMMU support". We can use the
> common check for entries in /sys/kernel/iommu_groups here, too, but it only
> makes sense to check it if there are also PCI devices available. It's also
> common on s390x that there are no PCI devices assigned to the LPAR, and in
> that case there is no need for the PCI-related IOMMU, so without PCI devices
> we should simply skip this test.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2:
>   - Use virDirOpen() + virDirRead() instead of stat() - this should hopefully
>     work now as expected.
> 
>   tools/virt-host-validate-common.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
> index 73e3bdb..e27b558 100644
> --- a/tools/virt-host-validate-common.c
> +++ b/tools/virt-host-validate-common.c
> @@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
>       virBitmapPtr flags;
>       struct stat sb;
>       const char *bootarg = NULL;
> -    bool isAMD = false, isIntel = false, isPPC = false;
> +    bool isAMD = false, isIntel = false;
> +    virArch arch = virArchFromHost();
> +    struct dirent *dent;
> +    DIR *dir;
> +    int rc;
> +
>       flags = virHostValidateGetCPUFlags();
>   
>       if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX))
> @@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
>   
>       virBitmapFree(flags);
>   
> -    isPPC = ARCH_IS_PPC64(virArchFromHost());
> -
>       if (isIntel) {
>           virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support"));
>           if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
> @@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
>                              "hardware platform");
>               return -1;
>           }
> -    } else if (isPPC) {
> +    } else if (ARCH_IS_PPC64(arch)) {
>           /* Empty Block */
> +    } else if (ARCH_IS_S390(arch)) {
> +        /* On s390x, we skip the IOMMU check if there are no PCI devices
> +         * (which is quite usual on s390x) */
> +        if (!virDirOpen(&dir, "/sys/bus/pci/devices"))
> +            return 0;
> +        rc = virDirRead(dir, &dent, NULL);
> +        VIR_DIR_CLOSE(dir);
> +        if (rc <= 0)
> +            return 0;
>       } else {
>           virHostMsgFail(level,
>                          "Unknown if this platform has IOMMU support");
> @@ -391,7 +403,7 @@ int virHostValidateIOMMU(const char *hvname,
>   
>       virHostMsgCheck(hvname, "%s", _("if IOMMU is enabled by kernel"));
>       if (sb.st_nlink <= 2) {
> -        if (!isPPC)
> +        if (bootarg)
>               virHostMsgFail(level,
>                              "IOMMU appears to be disabled in kernel. "
>                              "Add %s to kernel cmdline arguments", bootarg);
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
Posted by Michal Privoznik 5 years, 1 month ago 
On 3/1/19 12:10 PM, Thomas Huth wrote:
> When running virt-host-validate on an s390x host, the tool currently warns
> that it is "Unknown if this platform has IOMMU support". We can use the
> common check for entries in /sys/kernel/iommu_groups here, too, but it only
> makes sense to check it if there are also PCI devices available. It's also
> common on s390x that there are no PCI devices assigned to the LPAR, and in
> that case there is no need for the PCI-related IOMMU, so without PCI devices
> we should simply skip this test.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2:
>   - Use virDirOpen() + virDirRead() instead of stat() - this should hopefully
>     work now as expected.
> 
>   tools/virt-host-validate-common.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c
> index 73e3bdb..e27b558 100644
> --- a/tools/virt-host-validate-common.c
> +++ b/tools/virt-host-validate-common.c
> @@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
>       virBitmapPtr flags;
>       struct stat sb;
>       const char *bootarg = NULL;
> -    bool isAMD = false, isIntel = false, isPPC = false;
> +    bool isAMD = false, isIntel = false;
> +    virArch arch = virArchFromHost();
> +    struct dirent *dent;
> +    DIR *dir;
> +    int rc;
> +
>       flags = virHostValidateGetCPUFlags();
>   
>       if (flags && virBitmapIsBitSet(flags, VIR_HOST_VALIDATE_CPU_FLAG_VMX))
> @@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
>   
>       virBitmapFree(flags);
>   
> -    isPPC = ARCH_IS_PPC64(virArchFromHost());
> -
>       if (isIntel) {
>           virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU support"));
>           if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
> @@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
>                              "hardware platform");
>               return -1;
>           }
> -    } else if (isPPC) {
> +    } else if (ARCH_IS_PPC64(arch)) {
>           /* Empty Block */
> +    } else if (ARCH_IS_S390(arch)) {
> +        /* On s390x, we skip the IOMMU check if there are no PCI devices
> +         * (which is quite usual on s390x) */
> +        if (!virDirOpen(&dir, "/sys/bus/pci/devices"))
> +            return 0;
> +        rc = virDirRead(dir, &dent, NULL);
> +        VIR_DIR_CLOSE(dir);

Is this diropen and dirread really necessary? I mean, would something 
like plain test of the path presence be okay, e.g. 
virFileExists("/sys/bus/pci/devices").

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
Posted by Thomas Huth 5 years, 1 month ago 
On 07/03/2019 18.15, Michal Privoznik wrote:
> On 3/1/19 12:10 PM, Thomas Huth wrote:
>> When running virt-host-validate on an s390x host, the tool currently
>> warns
>> that it is "Unknown if this platform has IOMMU support". We can use the
>> common check for entries in /sys/kernel/iommu_groups here, too, but it
>> only
>> makes sense to check it if there are also PCI devices available. It's
>> also
>> common on s390x that there are no PCI devices assigned to the LPAR,
>> and in
>> that case there is no need for the PCI-related IOMMU, so without PCI
>> devices
>> we should simply skip this test.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v2:
>>   - Use virDirOpen() + virDirRead() instead of stat() - this should
>> hopefully
>>     work now as expected.
>>
>>   tools/virt-host-validate-common.c | 22 +++++++++++++++++-----
>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/virt-host-validate-common.c
>> b/tools/virt-host-validate-common.c
>> index 73e3bdb..e27b558 100644
>> --- a/tools/virt-host-validate-common.c
>> +++ b/tools/virt-host-validate-common.c
>> @@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
>>       virBitmapPtr flags;
>>       struct stat sb;
>>       const char *bootarg = NULL;
>> -    bool isAMD = false, isIntel = false, isPPC = false;
>> +    bool isAMD = false, isIntel = false;
>> +    virArch arch = virArchFromHost();
>> +    struct dirent *dent;
>> +    DIR *dir;
>> +    int rc;
>> +
>>       flags = virHostValidateGetCPUFlags();
>>         if (flags && virBitmapIsBitSet(flags,
>> VIR_HOST_VALIDATE_CPU_FLAG_VMX))
>> @@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
>>         virBitmapFree(flags);
>>   -    isPPC = ARCH_IS_PPC64(virArchFromHost());
>> -
>>       if (isIntel) {
>>           virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU
>> support"));
>>           if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
>> @@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
>>                              "hardware platform");
>>               return -1;
>>           }
>> -    } else if (isPPC) {
>> +    } else if (ARCH_IS_PPC64(arch)) {
>>           /* Empty Block */
>> +    } else if (ARCH_IS_S390(arch)) {
>> +        /* On s390x, we skip the IOMMU check if there are no PCI devices
>> +         * (which is quite usual on s390x) */
>> +        if (!virDirOpen(&dir, "/sys/bus/pci/devices"))
>> +            return 0;
>> +        rc = virDirRead(dir, &dent, NULL);
>> +        VIR_DIR_CLOSE(dir);
> 
> Is this diropen and dirread really necessary?

Yes.

> I mean, would something
> like plain test of the path presence be okay, e.g.
> virFileExists("/sys/bus/pci/devices").

No, that's unfortunately not enough. The "/sys/bus/pci/devices"
directory is always present, even if there are no PCI devices available,
it's just empty in that case. So as far as I can see, the dirread is
necessary here.

 Thomas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools/virt-host-validate: Fix IOMMU check on s390x
Posted by Michal Privoznik 5 years, 1 month ago 
On 3/7/19 9:17 PM, Thomas Huth wrote:
> On 07/03/2019 18.15, Michal Privoznik wrote:
>> On 3/1/19 12:10 PM, Thomas Huth wrote:
>>> When running virt-host-validate on an s390x host, the tool currently
>>> warns
>>> that it is "Unknown if this platform has IOMMU support". We can use the
>>> common check for entries in /sys/kernel/iommu_groups here, too, but it
>>> only
>>> makes sense to check it if there are also PCI devices available. It's
>>> also
>>> common on s390x that there are no PCI devices assigned to the LPAR,
>>> and in
>>> that case there is no need for the PCI-related IOMMU, so without PCI
>>> devices
>>> we should simply skip this test.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    v2:
>>>    - Use virDirOpen() + virDirRead() instead of stat() - this should
>>> hopefully
>>>      work now as expected.
>>>
>>>    tools/virt-host-validate-common.c | 22 +++++++++++++++++-----
>>>    1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/virt-host-validate-common.c
>>> b/tools/virt-host-validate-common.c
>>> index 73e3bdb..e27b558 100644
>>> --- a/tools/virt-host-validate-common.c
>>> +++ b/tools/virt-host-validate-common.c
>>> @@ -337,7 +337,12 @@ int virHostValidateIOMMU(const char *hvname,
>>>        virBitmapPtr flags;
>>>        struct stat sb;
>>>        const char *bootarg = NULL;
>>> -    bool isAMD = false, isIntel = false, isPPC = false;
>>> +    bool isAMD = false, isIntel = false;
>>> +    virArch arch = virArchFromHost();
>>> +    struct dirent *dent;
>>> +    DIR *dir;
>>> +    int rc;
>>> +
>>>        flags = virHostValidateGetCPUFlags();
>>>          if (flags && virBitmapIsBitSet(flags,
>>> VIR_HOST_VALIDATE_CPU_FLAG_VMX))
>>> @@ -347,8 +352,6 @@ int virHostValidateIOMMU(const char *hvname,
>>>          virBitmapFree(flags);
>>>    -    isPPC = ARCH_IS_PPC64(virArchFromHost());
>>> -
>>>        if (isIntel) {
>>>            virHostMsgCheck(hvname, "%s", _("for device assignment IOMMU
>>> support"));
>>>            if (access("/sys/firmware/acpi/tables/DMAR", F_OK) == 0) {
>>> @@ -373,8 +376,17 @@ int virHostValidateIOMMU(const char *hvname,
>>>                               "hardware platform");
>>>                return -1;
>>>            }
>>> -    } else if (isPPC) {
>>> +    } else if (ARCH_IS_PPC64(arch)) {
>>>            /* Empty Block */
>>> +    } else if (ARCH_IS_S390(arch)) {
>>> +        /* On s390x, we skip the IOMMU check if there are no PCI devices
>>> +         * (which is quite usual on s390x) */
>>> +        if (!virDirOpen(&dir, "/sys/bus/pci/devices"))
>>> +            return 0;
>>> +        rc = virDirRead(dir, &dent, NULL);
>>> +        VIR_DIR_CLOSE(dir);
>>
>> Is this diropen and dirread really necessary?
> 
> Yes.
> 
>> I mean, would something
>> like plain test of the path presence be okay, e.g.
>> virFileExists("/sys/bus/pci/devices").
> 
> No, that's unfortunately not enough. The "/sys/bus/pci/devices"
> directory is always present, even if there are no PCI devices available,
> it's just empty in that case. So as far as I can see, the dirread is
> necessary here.

Ah, this was the missing piece. Okay, makes sense then.

I'm adding the following to the comment then, ACKing and pushing:

+        /* On s390x, we skip the IOMMU check if there are no PCI
+         * devices (which is quite usual on s390x). If there are
+         * no PCI devices the directory is still there but is
+         * empty. */


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.