Re: [libvirt] doc: write separate module for hostdev passthrough and in-use tracking

On 25/03/13 14:51, Chunyan Liu wrote: > > 2013/3/22 Osier Yang <jyang(a)redhat.com&gt;: >> >> On 2013年03月22日 17:36, Chunyan Liu wrote: >>> >>> Hi, List, >>> >>> As the mail I've sent a week before: >>> https://www.redhat.com/archives/libvir-list/2013-March/msg00730.html >>> I'm willing to push this work forward so that the passthrough APIs >>> could be reused by qemu driver and libxl driver (which doesn't support >>> pci passthrough yet and tries to add this function recently), or other >>> drivers. >>> >>> But since this work affacts a lot, I'm not sure if I can control it in >>> a correct way. I write a draft to describe what I'm considering how to >>> do, as in the following and in attachment. Hope to get your review, >>> comment and guidence to improve the work before start coding. Any >>> feedback will be very appreciated! >>> >>> Thanks! >>> Chunyan >>> >>> ------------------------ >>> DRAFT: >>> >>> Write separate module for hostdev passthrough >> >> >> Cool. we lacked this for a time. >> >> >>> 1. Purposes: >>> >>> * Move hostdev passthrough APIs from qemu_hostdev.ch to separate >>> module so >>> that they could be reused by other hypervisors too >>> >>> * Maintain global in-use state of hostdevs >> >> >> This is more important than the code-reuse. >> >> >>> >>> 2. Module design (draft): >>> >>> * New module name: hostdev_driver >>> >>> * New module files: hostdev_driver.ch hostdev_conf.ch >>> >>> * New Definitions: >>> >>> ## [src/driver.h] >>> >>> typedef struct _virHostdevDriver virHostdevDriver; >>> >>> typedef virHostdevDriver *virHostdevDriverPtr; >>> >>> struct _virHosedevDriver { >>> const char *name; >>> virDrvOpen open; >>> virDrvClose close; >>> >>> virDrvPrepareHostdevs prepareHostdevs; >>> virDrvPreparePciHostdevs preparePciHostdevs; >>> virDrvprepareUsbHostdevs prepareUsbHostdevs; >> >> >> In case of you want to expose prepareHostdevs, no need to expose >> preparePciHostdevs and prepareUsbHostdevs? > > Thanks very much for your comments. > > Exposing these two APIs is considering that some driver may supports > one but not another, so that it could call specific API. But we can > use support flag in prepareHostdevs to control that, in this way not > need these two APIs > >>> virDrvReattachHostdevs reattachHostdevs; >>> virDrvReattachPciHostdevs reattachPciHostdevs; >>> virDrvReattachUsbHostdevs reattachUsbHostdevs; >> >> >> Likewise. >> >> >>> virDrvGetActivePciHostdevList >>> getActivePciHostdevList; >>> virDrvGetActiveUsbHostdevList >>> getActiveUsbHostdevList; >>> virDrvGetDomainActivePciHostdevList >>> getDomainActivePciHostdevList; >>> virDrvGetDomainActiveUsbHostdevList >>> getDomainActiveUsbHostdevList; >> >> >> These APIs are useful for upper layer management too. I have once >> wanted to create similiar APIs, but only tended for qemu driver >> at that time. >> >> But except these 4 get APIs, others are only useful for other drivers >> (internally), useless for upper layer management. > > That's true. > >> Do we really want a >> driver instead of just an internal share module? Like src/nodeinfo.[ch], >> and with it we still can expose APIs like the 4 get APIs. > > Do you mean add src/hostdev.[ch] and do all work there? Think a while, > I think it can achieve too. Yes. > Then do we need to differentiate 4 get > APIs and other APIs? Not sure about you meaning here.

But you can take the existing APIs like virNodeSetMemoryParameters as examples. The API is implemented in src/nodeinfo.[ch], but each driver can use it. > >> >>> }; >>> >>> ## [src/hostdev/hostdev_conf.h] >>> >>> typedef struct _virHostdevDriverState virHostdevDriverState; >>> >>> typedef virHostdevDriverState *virHostdevDriverStatePtr; >>> >>> struct _virHostdevDriverState { >>> virMutex lock; >>> virPCIDeviceListPtr activePciHostdevs; >>> virPCIDeviceListPtr inactivePciHostdevs; >>> virUSBDeviceListPtr activeUsbHostdevs; >>> }; >>> >>> ## [src/hostdev/hostdev_driver.c] >>> >>> static virHostdevDriver hostdevDriver = { >>> .name = "hostdev", >>> .open = hostdevDriverOpen, >>> .close = hostdevDriverClose, >>> .prepareHostdevs = virPrepareHostdevs, >>> .preparePciHostdevs = virPreparePciHostdevs, >>> .prepareUsbHostdevs = virPrepareUsbHostdevs >>> .reattachHostdevs = virReattachHostdevs, >>> .reattachPciHostdevs = virReattachPciHostdevs, >>> .reattachUsbHostdevs = virReattachUsbHostdevs, >>> .getActivePciHostdevList = virGetActivePciHostdevList, >>> .getActiveUsbHostdevList = virGetActiveUsbHostdevList, >>> .getDomainActivePciHostdevList = >>> virGetDomainActivePciHostdevList, >>> .getDomainActiveUsbHostdevList = >>> virGetDomainActiveUsbHostdevList, >>> }; >>> >>> static virStateDriver hostdevStateDriver = { >>> .name = "hostdev", >>> .initialize = hostdevDriverStartup, >>> .cleanup = hostdevDriverCleanup, >>> .reload = hostdevDriverReload, >>> }; >>> >>> * Changed Definitions: >>> >>> struct _virPCIDevice { >>> ...... >>> >>> --- const char *used_by; /* The domain which uses the >>> device */ >>> +++ virDomainObjPtr used_by; /* include domname and conn >>> info */ >> >> >> Why need the "conn info"? Isn't a driver name enough here? > > Driver name is OK. Just need to fill unique info about the device is > used by which driver which domain. So I try to store virDomainObjPtr > to the used_by area, from this pointer, we can get driver name and > domain name. Do you think that's OK? Yeah, I think virDomainObjPtr is just overkill, you have to maintain the object's lifecycle.

> >> >>> ...... >>> }; >>> >>> struct _virUSBDevice { >>> ...... >>> --- const char *used_by; /* name of the domain using >>> this >>> dev */ >>> +++ virDomainObjPtr used_by; /* include domname and conn >>> info */ >>> }; >>> >>> * APIs: >>> >>> typedef int >>> (*virDrvPrepareHostdevs)(virHostdevDriverPtr driver, >>> virDomainObjPtr vm, >>> unsigned int flags); >>> /* >>> - workflow: >>> call PrepareHostdevPciDevices and >>> PrepareHostdevUsbDevices >>> to do specific work. >>> >>> - reference: >>> int qemuPrepareHostDevices(virQEMUDriverPtr driver, >>> virDomainDefPtr def, >>> bool coldBoot); >>> >>> - new parameter: >>> - flags: >>> - could set "coldBoot" for usb usage >>> - could set hypervisor tag since for qemu it will use >>> pci-stub, for libxl, it will use pciback. >>> */ >>> >>> typedef int >>> (*virDrvPreparePciHostdevs)(virHostdevDriverPtr driver, >>> virDomainObjPtr vm, >>> virDomainHostdevDefPtr *hostdevs, >>> int nhostdevs, >>> unsigned int flags); >>> /* >>> - workflow: >>> 1. check things (e.g. assignability to non-managed >>> device) >>> 2. detach managed device and reset work >>> 3. set usedby to 'vm', update activePciHostdevs and >>> inactivePciHostdevs >>> >>> - reference: >>> int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, >>> const char *name, >>> const unsigned char *uuid, >>> virDomainHostdevDefPtr *hostdevs, >>> int nhostdevs); >>> */ >>> >>> typedef int >>> (*virDrvprepareUsbHostdevs)((virHostdevDriverPtr driver, >>> virDomainObjPtr vm, >>> virUSBDeviceListPtr list); >>> /* >>> - workflow: >>> check usb device, set usedby to 'vm', update >>> activeUsbHostdevs >>> >>> - reference: >>> int qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, >>> const char *name, >>> virUSBDeviceListPtr list); >>> */ >>> >>> typedef void >>> (*virDrvReattachHostdevs)(virHostdevDriverPtr driver, >>> virDomainObjPtr vm); >>> /* >>> - workflow: >>> call reattachPciHostDevices and reattachUsbHostDevices >>> to >>> do specific work. >>> >>> - reference: >>> void qemuDomainReAttachHostDevices(virQEMUDriverPtr >>> driver, >>> virDomainDefPtr def) >>> */ >>> >>> typedef void >>> (*virDrvReattachPciHostdevs) (virHostdevDriverPtr driver, >>> virDomainObjPtr vm, >>> virDomainHostdevDefPtr >>> *hostdevs, >>> int nhostdevs); >>> /* >>> - workflow: >>> unbind and bind to original driver, free >>> usedby, activePciHostdevs and inactivePciHostdevs. >>> >>> - reference: >>> void >>> qemuDomainReAttachHostdevDevices(virQEMUDriverPtrdriver, >>> const char *name, >>> virDomainHostdevDefPtr >>> *hostdevs, >>> int nhostdevs) >>> */ >>> >>> typedef void >>> (*virDrvReattachUsbHostdevs) (virHostdevDriverPtr driver, >>> virDomainObjPtr vm, >>> virDomainHostdevDefPtr *hostdevs, >>> int nhostdevs); >>> /* >>> - workflow: >>> free usedby, activePciHostdevs and inactivePciHostdevs. >>> >>> - reference: >>> static void >>> qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr >>> driver, >>> const char *name, >>> virDomainHostdevDefPtr *hostdevs, >>> int nhostdevs) >>> */ >>> >>> typedef virPCIDeviceListPtr >>> (*virDrvGetActivePciHostdevList) (virHostdevDriverPtr driver); >>> /* >>> - could be used to check if a pci hostdev is already >>> in-use. >>> */ >>> >>> typedef virUSBDeviceListPtr >>> (*virDrvGetActiveUsbHostdevList) (virHostdevDriverPtr driver); >>> /* >>> - could be used to check if a usb hostdev is already >>> in-use. >>> */ >>> >>> typedef virPCIDeviceListPtr >>> (*virDrvGetDomainActivePciHostdevList) (virHostdevDriverPtr >>> driver); >>> /* >>> - could be used to reattach all in-use pci hostdevs by a >>> domain (e.g. when a domain shutdown) >>> */ >>> >>> typedef virUSBDeviceListPtr >>> (*virDrvGetDomainActiveUsbHostdevList) (virHostdevDriverPtr >>> driver); >>> /* >>> - could be used to reattach all in-use usb hostdevs by a >>> domain >>> */ >> >> >> So it needs to expose the virPCI{USB}DeviceList to public. Not sure how >> useful it will be to expose the struct, instead of the ID of the >> devices. > > The case I was considering here is: when reattaching hostdevs in vm > shutoff, 'managed' or 'non-managed' info is also needed. But in this > case, it is internally used, we can also handle that in > virDrvReattachHostdevs. > Could "managed=yes/no" info be used somewhere else? If not, then seems > ID is enough. "managed=yes/no" is only used by attaching/detaching, nodedev-dettach and nodedev-reattach. > >> These APIs could be used for node device driver. To filter the active/ >> inactive devices. >> >> >>> 3. Use the module >>> >>> * Register hostdev driver >>> - define 'virHostdevDriverTab' and 'virHostdevDriverTabCount' >>> - add hostdev driver register work in libvirtd.c >>> daemonInitialize() >>> >>> * add hostdev driver areas to _virConnect >>> struct _virConnect { >>> ....... >>> virNWFilterDriverPtr nwfilterDriver; >>> +++ virHostdevDriverPtr hostdevDriver; >>> >>> ....... >>> void * nwfilterPrivateData; >>> +++ void * hostdevPrivateData; >>> ....... >>> } >>> >>> >>> * add conn->hostdevDriver parser in libvirt.c: do_open() >>> Then hypervisor drivers can get use of hostdev driver APIs. >>> >>> * moments that could be affacted: >>> - domain start/shutoff >>> - attach/detach hostdev to domain >> >> >> And libvirt restarting. >> >>> -------------------------------------------- >>> >>> >>> >>> -- >>> libvir-list mailing list >>> libvir-list(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/libvir-list >> >> >>