WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 2 of 3] xl, implement pci-list-assignable-devices

On Wed, 2010-07-28 at 18:39 +0100, Stefano Stabellini wrote:
> > +                }
> > +                libxl_free(ctx, bdf);
> > +                libxl_free(ctx, devpath);
> > +            }
> > +            libxl_free(ctx, num_devs);
> > +        }
> > +        libxl_free(ctx, path);
> > +    }
> > +    libxl_free(ctx, domlist);
> > +
> > +    return pcidevs;
> > +}
> 
> You shouldn't use libxl_free explicitly on variables allocated in the
> context. Actually I like explicit free's but we have to keep the style
> consistent, so the memory management refactoring should come in as a
> separate patch, and here there shouldn't be any libxl_free's.
> The realloc is correct because it is used on a variable that is going
> to be returned to the caller.

OK, this was written before the idea for refactoring memory management
and just done out of habit. Are you saying just remove the libxl_free's
and re-submit it like that?

> Also I would prefer this function to return an integer and take the
> libxl_device_pci array as a parameter.

Hmm, OK.

> > +libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, 
> > int *num)
> > +{
> > +    libxl_device_pci *pcidevs = NULL;
> > +    libxl_device_pci *assigned;
> > +    int num_assigned;
> > +
> > +    *num = 0;
> > +
> > +    assigned = get_all_assigned_devices(ctx, &num_assigned);
> > +
> > +    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> > +                              "/sys/bus/pci/drivers/pciback", num);
> > +    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> > +                              "/sys/bus/pci/drivers/pcistub", num);
> > +
> > +    free(assigned);
> > +    return pcidevs;
> > +}
> > +
> 
> Same comment about returning an integer, also I don't like the way you
> are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that
> should be an implementation detail of scan_sys_pcidir that callers
> shouldn't rely upon.

Actually it's a static function with only one caller, this makes the
implementation of list_assignable as simple as possible.

> Consider that devices bound to pcistub cannot be assigned to PV guests,
> so I would remove scan_sys_pcidir on pcistub from here.
> Please define "/sys/bus/pci/drivers/pciback" in a macro.

But they can be assigned to HVM guests? Nothing in the add-device path
uses this list of devices anyway. What actually happens is a check
whether a device appears in the get_all_assigned_devices() list. Even
after this patch-set the user can always specify BDF's which aren't
really assignable (eg. the Host's PCI-to-ISA bridge or PCI root for
maximum amusement) That is to be addressed in a future patch.

Whether-or-not a specific device can be added is an issue for the
add-device path. I think that the best approach there would then be just
to stat the relevant sysfs paths to make sure the device is available.

As for the macro, of course, I will change that.

Thanks for the comments.

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel