Re: [PATCH v3 4/6] i386/pc: relocate 4g start to 1T where applicable

[Joao Martins]

On 2/23/22 23:35, Joao Martins wrote:
> On 2/23/22 21:22, Michael S. Tsirkin wrote:
>>> +static void x86_update_above_4g_mem_start(PCMachineState *pcms,
>>> +                                          uint64_t pci_hole64_size)
>>> +{
>>> +    X86MachineState *x86ms = X86_MACHINE(pcms);
>>> +    uint32_t eax, vendor[3];
>>> +
>>> +    host_cpuid(0x0, 0, &eax, &vendor[0], &vendor[2], &vendor[1]);
>>> +    if (!IS_AMD_VENDOR(vendor)) {
>>> +        return;
>>> +    }
>>
>> Wait a sec, should this actually be tying things to the host CPU ID?
>> It's really about what we present to the guest though,
>> isn't it?
> 
> It was the easier catch all to use cpuid without going into
> Linux UAPI specifics. But it doesn't have to tie in there, it is only
> for systems with an IOMMU present.
> 
>> Also, can't we tie this to whether the AMD IOMMU is present?
>>
> I think so, I can add that. Something like a amd_iommu_exists() helper
> in util/vfio-helpers.c which checks if there's any sysfs child entries
> that start with ivhd in /sys/class/iommu/. Given that this HT region is
> hardcoded in iommu reserved regions since >=4.11 (to latest) I don't think 
> it's
> even worth checking the range exists in:
> 
>       /sys/kernel/iommu_groups/0/reserved_regions
> 
> (Also that sysfs ABI is >= 4.11 only)

Here's what I have staged in local tree, to address your comment.

Naturally the first chunk is what's affected by this patch the rest is a
precedessor patch to introduce qemu_amd_iommu_is_present(). Seems to pass
all the tests and what not.

I am not entirely sure this is the right place to put such a helper, open
to suggestions. wrt to the naming of the helper, I tried to follow the rest
of the file's style.

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a9be5d33a291..2ea4430d5dcc 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -868,10 +868,8 @@ static void x86_update_above_4g_mem_start(PCMachineState 
*pcms,
                                           uint64_t pci_hole64_size)
 {
     X86MachineState *x86ms = X86_MACHINE(pcms);
-    uint32_t eax, vendor[3];

-    host_cpuid(0x0, 0, &eax, &vendor[0], &vendor[2], &vendor[1]);
-    if (!IS_AMD_VENDOR(vendor)) {
+    if (!qemu_amd_iommu_is_present()) {
         return;
     }

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 7bcce3bceb0f..eb4ea071ecec 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -637,6 +637,15 @@ char *qemu_get_host_name(Error **errp);
  */
 size_t qemu_get_host_physmem(void);

+/**
+ * qemu_amd_iommu_is_present:
+ *
+ * Operating system agnostic way of querying if an AMD IOMMU
+ * is present.
+ *
+ */
+bool qemu_amd_iommu_is_present(void);
+
 /*
  * Toggle write/execute on the pages marked MAP_JIT
  * for the current thread.
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f2be7321c59f..54cef21217c4 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -982,3 +982,32 @@ size_t qemu_get_host_physmem(void)
 #endif
     return 0;
 }
+
+bool qemu_amd_iommu_is_present(void)
+{
+    bool found = false;
+#ifdef CONFIG_LINUX
+    struct dirent *entry;
+    char *path;
+    DIR *dir;
+
+    path = g_strdup_printf("/sys/class/iommu");
+    dir = opendir(path);
+    if (!dir) {
+        g_free(path);
+        return found;
+    }
+
+    do {
+            entry = readdir(dir);
+            if (entry && !strncmp(entry->d_name, "ivhd", 4)) {
+                found = true;
+                break;
+            }
+    } while (entry);
+
+    g_free(path);
+    closedir(dir);
+#endif
+    return found;
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index af559ef3398d..c08826e7e19b 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -652,3 +652,8 @@ size_t qemu_get_host_physmem(void)
     }
     return 0;
 }
+
+bool qemu_amd_iommu_is_present(void)
+{
+    return false;
+}