[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4 1/4] add hostdev passthrough common library



Chunyan Liu wrote:
> Thanks very much! Still two places to confirm:
>
> 2013/8/21 Daniel P. Berrange <berrange redhat com
> <mailto:berrange redhat com>>
>
>     On Mon, Aug 19, 2013 at 04:49:37PM -0400, cyliu suse com
>     <mailto:cyliu suse com> wrote:
>     > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>     > new file mode 100644
>     > index 0000000..1baa829
>     > --- /dev/null
>     > +++ b/src/util/virhostdev.c
>     > +
>     > +/* For virReportOOMError()  and virReportSystemError() */
>
>     No need for this comment - this is standard practice for every source
>     file
>
>     > +#define VIR_FROM_THIS VIR_FROM_NONE
>
>     > +
>     > +/* Same name as in virDriver. For special check. */
>     > +#define LIBXL_DRIVER_NAME "xenlight"
>     > +#define QEMU_DRIVER_NAME  "QEMU"
>
>     You're using this later to determine whether to use pci-back
>     vs pci-stub. 
>
>     I think it it would be preferrable to have the drivers pass
>     in the name of their desired stub driver instead.
>
>
> I'm afraid there are some problems:
> Currently there are two places:
>  1. if  <driver name=xx /> is not specified in pci hostdev .xml, use
> default stub driver. But to 'libxl' and 'qemu', the default stub
> driver is different (pciback vs pci-stub), so, need to check
> hypervisor driver name to decide default stub dirver name.
>  2. in detach-device, for 'qemu/kvm', it needs to check
> 'kvm_assigned_device', waiting for device cleanup. For 'libxl', it
> doesn't. So, need to check hypervisor driver name to add the extra
> processing.
>
> Besides, to 'qemu', not only the 'pci-stub' case, it could be
> 'pci-stub' or 'vfio'. To prepare domain hostdevs, just could not pass
> ONE stub driver name simply to virHostdev API (e.g,
> virHostdevPrepareDomainHostdevs).
>
> Any suggestions?

Perhaps these driver-specific items could be handled by callbacks into
the driver, similar to driver-specific XML parsing and formating
callbacks in the common domain_conf code.

>  
>
>     > +static int
>     > +virHostdevOnceInit(void)
>     > +{
>     > +    char ebuf[1024];
>     > +
>     > +    if (VIR_ALLOC(hostdevMgr) < 0)
>     > +        goto error;
>     > +
>     > +    if ((hostdevMgr->activePciHostdevs = virPCIDeviceListNew())
>     == NULL)
>     > +        goto error;
>     > +
>     > +    if ((hostdevMgr->activeUsbHostdevs = virUSBDeviceListNew())
>     == NULL)
>     > +        goto error;
>     > +
>     > +    if ((hostdevMgr->inactivePciHostdevs =
>     virPCIDeviceListNew()) == NULL)
>     > +        goto error;
>     > +
>     > +    if ((hostdevMgr->activeScsiHostdevs =
>     virSCSIDeviceListNew()) == NULL)
>     > +        goto error;
>     > +
>     > +    if (virAsprintf(&hostdevMgr->stateDir, "%s",
>     HOSTDEV_STATE_DIR) < 0)
>     > +        goto error;
>     > +
>     > +    if (virFileMakePath(hostdevMgr->stateDir) < 0) {
>     > +        VIR_ERROR(_("Failed to create state dir '%s': %s"),
>     > +                  hostdevMgr->stateDir, virStrerror(errno,
>     ebuf, sizeof(ebuf)));
>
>     You should be using virReportError here
>
>     > +        goto error;
>     > +    }
>     > +
>     > +    return 0;
>     > +
>     > +error:
>
>     You should free the partially initialized 'hostdevMgr' instance & all
>     its data
>
>     > +    return -1;
>     > +}
>     > +
>     > +VIR_ONCE_GLOBAL_INIT(virHostdev)
>     > +
>     > +virHostdevManagerPtr
>     > +virHostdevManagerGetDefault(void)
>     > +{
>     > +    virHostdevInitialize();
>
>     You should do
>
>        if (virHostdevInitialize() < 0)
>            return NULL;
>
>     > +    return hostdevMgr;
>     > +}
>     > +
>
>
>
>     > +
>     > +void
>     > +virHostdevReAttachUsbHostdevs(virHostdevManagerPtr mgr,
>     > +                              const char *drv_name,
>     > +                              const char *dom_name,
>     > +                              virDomainHostdevDefPtr *hostdevs,
>     > +                              int nhostdevs)
>     > +{
>     > +    size_t i;
>     > +
>     > +    virObjectLock(mgr->activeUsbHostdevs);
>
>     I wonder if we should get rid of the mutex locks in
>     the PCI / USB device list objects, and instead have
>     a single lock on the virHostdevManagerPtr instance.
>
> I noticed in qemu_hostdev.c, originally it used single driver lock,
> later changed to use pci/usb list object lock. Do you think a single
> lock is still preferred? If yes, I'll update.

I don't know the reason for that change, but it would seem desirable to
manage USB devices without needing to wait for some operation on a PCI
device to complete.

Regards,
Jim


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]