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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/07/2015 04:04 AM, Dario Faggioli wrote:
> On Mon, 2015-01-05 at 21:18 -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
> Also, can we see how an `xl info -n' looks, on an IONUMA system?
>
> > @@ -195,6 +195,24 @@ int xc_cputopoinfo(xc_interface *xch,
> >       return 0;
> >   }
> >   
> > +int xc_pcitopoinfo(xc_interface *xch,
> > +                   xc_pcitopoinfo_t *put_info)
> > +{
> > +    int ret;
> > +    DECLARE_SYSCTL;
> > +
> > +    sysctl.cmd = XEN_SYSCTL_pcitopoinfo;
> > +
> > +    memcpy(&sysctl.u.pcitopoinfo, put_info, sizeof(*put_info));
> > +
> > +    if ( (ret = do_sysctl(xch, &sysctl)) != 0 )
> > +        return ret;
> > +
> > +    memcpy(put_info, &sysctl.u.pcitopoinfo, sizeof(*put_info));
> > +
> > +    return 0;
> > +}
> > @@ -5121,6 +5121,64 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx 
> > *ctx, int *nb_cpu_out)
> >       return ret;
> >   }
> >   
> > +libxl_pcitopology *libxl_get_pci_topology(libxl_ctx *ctx, int *num_devs)
> > +{
> > +    GC_INIT(ctx);
> > +    xc_pcitopoinfo_t tinfo;
> > +    DECLARE_HYPERCALL_BUFFER(xen_sysctl_pcitopo_t, pcitopo);
> > +    libxl_pcitopology *ret = NULL;
> > +    int i, rc;
> > +
> I see from where this comes from. However, at least from new functions,
> I think we should avoid using things like DECLARE_HYPERCALL_BUFFER etc.,
> in libxl. They belong in libxc, IMO.
>
> This is basically what Andrew was doing here (although that was on
> xc_{topology,numa}info:
>
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/hwloc-support-experimental
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=blobdiff;f=tools/libxc/xc_misc.c;h=4f672cead5a4d2b1fc23bcddb6fb49e4d6ec72b5;hp=330345459176961cacc7fda506e843831fe7156a;hb=3585994405b6a73c137309dd4be91f48c71e4903;hpb=9a80d5056766535ac624774b96495f8b97b1d28b
>
> Basically, what I'm suggesting is to have xc_pcitopoinfo() to do most of
> the work, with libxl_get_pci_topology being just a wrapper to it.

Maybe then I should update libxl_get_cpu_topology() and 
libxl_get_numainfo() as well for code uniformity.
-boris


> In fact, if you grep/cscope for things like DECLARE_HYPERCALL_BUFFER in
> tools/libxl, the *only* results are these:
>
> libxl.c             libxl_get_cpu_topology                  5076 
> DECLARE_HYPERCALL_BUFFER(xc_cpu_to_core_t, coremap);
> libxl.c             libxl_get_cpu_topology                  5077 
> DECLARE_HYPERCALL_BUFFER(xc_cpu_to_socket_t, socketmap);
> libxl.c             libxl_get_cpu_topology                  5078 
> DECLARE_HYPERCALL_BUFFER(xc_cpu_to_node_t, nodemap);
> libxl.c             libxl_get_numainfo                      5142 
> DECLARE_HYPERCALL_BUFFER(xc_node_to_memsize_t, memsize);
> libxl.c             libxl_get_numainfo                      5143 
> DECLARE_HYPERCALL_BUFFER(xc_node_to_memfree_t, memfree);
> libxl.c             libxl_get_numainfo                      5144 
> DECLARE_HYPERCALL_BUFFER(uint32_t, node_dists);
>
> And I think we should work toward removing these too, rather than adding
> new ones! :-)
>
> Regards,
> Dario

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