Port-xen archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: [PATCH v3 7/7] libxl: Add interface for querying hypervisor about PCI topology



On 02/16/2015 08:45 AM, Dario Faggioli wrote:


@@ -1070,6 +1078,12 @@ void libxl_vminfo_list_free(libxl_vminfo *list, int nb_vm);
  libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out);
  void libxl_cputopology_list_free(libxl_cputopology *, int nb_cpu);

+#ifdef  LIBXL_HAVE_PCITOPO
+#define LIBXL_PCITOPOLOGY_INVALID_ENTRY (~(uint32_t)0)
+libxl_pcitopology *libxl_get_pci_topology(libxl_ctx *ctx, int *num_devs);
+void libxl_pcitopology_list_free(libxl_pcitopology *, int num_devs);
+#endif
+
Is there the need to gate new APIs like this? I've never done that, and
I don't think there is. AFAIUI, these are (apart from a few exceptions
like LIBXL_HAVE_NO_SUSPEND_RESUME) intended for being used by libxl
consumers, not libxl own implementation.

diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index e8b88b3..68d7b71 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -131,3 +131,17 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
  {
      return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
  }
+
+#ifdef  LIBXL_HAVE_PCITOPO
+int libxl__pci_numdevs(libxl__gc *gc)
+{
+    return ERROR_NI;
+}
+
+int libxl__pci_topology_init(libxl__gc *gc,
+                             physdev_pci_device_t *devs,
+                             int num_devs)
+{
+    return ERROR_NI;
+}
+#endif

And the same for internal functions, and elsewhere.

@@ -5209,12 +5214,37 @@ static void output_topologyinfo(void)
      printf("cpu:    core    socket     node\n");

      for (i = 0; i < nr; i++) {
-        if (info[i].core != LIBXL_CPUTOPOLOGY_INVALID_ENTRY)
+        if (cpuinfo[i].core != LIBXL_CPUTOPOLOGY_INVALID_ENTRY)
              printf("%3d:    %4d     %4d     %4d\n", i,
-                   info[i].core, info[i].socket, info[i].node);
+                   cpuinfo[i].core, cpuinfo[i].socket, cpuinfo[i].node);
+    }
+
+    libxl_cputopology_list_free(cpuinfo, nr);
+
+#ifdef  LIBXL_HAVE_PCITOPO
+    pciinfo = libxl_get_pci_topology(ctx, &nr);
+    if (cpuinfo == NULL) {
+        fprintf(stderr, "libxl_get_pci_topology failed.\n");
+        return;
      }

-    libxl_cputopology_list_free(info, nr);
+    printf("device topology        :\n");
+    printf("device           node\n");
+    for (i = 0; i < nr; i++) {
+        if (pciinfo[i].node != LIBXL_PCITOPOLOGY_INVALID_ENTRY) {
+            printf("%04x:%02x:%02x.%01x      %d\n", pciinfo[i].seg,
+                   pciinfo[i].bus,
+                   ((pciinfo[i].devfn >> 3) & 0x1f), (pciinfo[i].devfn & 7),
+                   pciinfo[i].node);
+            valid_devs++;
+        }
+    }
+
+    if (valid_devs == 0)
+        printf("No device topology data available\n");
+
+    libxl_pcitopology_list_free(pciinfo, nr);
+#endif

And for implementation too, I think.


I am not sure what the standard practice is for LIBXL_HAVE_ macros. I see a couple of examples where '#ifdef LIBXL_HAVE_*" is used in libxl code, which is why I have it here as well.

This is probably question for maintainers.



In fact, if I'm not missing something obvious, since we're always
distributing xl and libxl together, this will always be "#ifdef 1",
wouldn't it?

Also, ISTR that the first change that actually changes the API should
bump the MAJOR in libxl's Makefile. I'm not sure this change qualifies
as such, as you're adding stuff, not altering existing data structs
(e.g., by adding fields, etc). Personally, I think it does, but I'm
leaving this to tools maintainers.

libxl.h seems to suggest that API version is changed only when we make an incompatible change to the library. In my mind new interface is does not break compatibility so I didn't think a bump would be necessary.

-boris




Home | Main Index | Thread Index | Old Index