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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Mon, 2015-02-09 at 15:04 -0500, Boris Ostrovsky wrote:
> .. and use this new interface to display it along with CPU topology
> and NUMA information when 'xl info -n' command is issued
> 
> The output will look like
> ...
> cpu_topology           :
> cpu:    core    socket     node
>   0:       0        0        0
> ...
> device topology        :
> device           node
> 0000:00:00.0      0
> 0000:00:01.0      0
> ...
> 
Cool, I like this! :-)

At some point, we may want to think about gathering most of/all NUMA
related information and create appropriate xl commands, but for now, I
think it is fine to have this info here.

> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -692,6 +692,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> libxl_mac *src);
>  #define LIBXL_HAVE_PSR_CMT 1
>  #endif
>  
> +/*
> + * LIBXL_HAVE_PCITOPO
> + *
> + * If this is defined, we have interfaces to query hypervisor about PCI 
> device
> + * topology
> + */
> +#define  LIBXL_HAVE_PCITOPO 1
> +
>
This is probably not a big deal, but the majority of the definitions of
these macros have a wording like this: "FOO indicates that ...". E.g.:

/*
 * LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY indicates that a 'cpumap_soft'
 * field (of libxl_bitmap type) is present in libxl_vcpuinfo,
 * containing the soft affinity of a vcpu.
 */
#define LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY 1

and homogeneity and consistency has some value here, I think.

In any case, you have an extra space between #define and
LIBXL_HAVE_PCITOPO. :-)

> @@ -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.

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.

Regards,
Dario

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel