From: Timur Tabi <hidden> Date: 2011-06-01 18:37:14
The Freescale hypervisor management driver provides several services to
drivers and applications related to the Freescale hypervisor:
1. An ioctl interface for querying and managing partitions
2. A file interface to reading incoming doorbells
3. An interrupt handler for shutting down the partition upon receiving the
shutdown doorbell from a manager partition
4. An interface for receiving callbacks when a managed partition shuts down.
Signed-off-by: Timur Tabi <redacted>
---
drivers/misc/Kconfig | 7 +
drivers/misc/Makefile | 1 +
drivers/misc/fsl_hypervisor.c | 941 ++++++++++++++++++++++++++++++++++++++++
include/linux/Kbuild | 1 +
include/linux/fsl_hypervisor.h | 203 +++++++++
5 files changed, 1153 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/fsl_hypervisor.c
create mode 100644 include/linux/fsl_hypervisor.h
@@ -0,0 +1,941 @@+/** @file+*FreescaleHypervisorManagementDriver+*+*ThisdrivercontainsfunctionstosupporttheFreescalehypervisor.+*/+/* Copyright (C) 2008-2010 Freescale Semiconductor, Inc.+*Author:TimurTabi<timur@freescale.com>+*+*ThisfileislicensedunderthetermsoftheGNUGeneralPublicLicense+*version2.Thisprogramislicensed"as is"withoutanywarrantyofany+*kind,whetherexpressorimplied.+*/++#include<linux/kernel.h>+#include<linux/module.h>+#include<linux/init.h>+#include<linux/types.h>+#include<linux/err.h>+#include<linux/fs.h>+#include<linux/miscdevice.h>+#include<linux/mm.h>+#include<linux/pagemap.h>+#include<linux/slab.h>+#include<linux/poll.h>+#include<linux/of.h>+#include<linux/reboot.h>+#include<linux/uaccess.h>+#include<linux/notifier.h>++#include<linux/io.h>+#include<asm/fsl_hcalls.h>++#include<linux/fsl_hypervisor.h>++staticBLOCKING_NOTIFIER_HEAD(failover_subscribers);++/**+*ioctl_restart:ioctlinterfaceforFSL_HV_IOCTL_PARTITION_RESTART+*+*Restartarunningpartition+*/+staticlongioctl_restart(structfsl_hv_ioctl_restart__user*p)+{+structfsl_hv_ioctl_restartparam;++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_restart)))+return-EFAULT;++param.ret=fh_partition_restart(param.partition);++if(copy_to_user(&p->ret,¶m.ret,sizeof(__u32)))+return-EFAULT;++return0;+}++/**+*ioctl_status:ioctlinterfaceforFSL_HV_IOCTL_PARTITION_STATUS+*+*Querythestatusofapartition+*/+staticlongioctl_status(structfsl_hv_ioctl_status__user*p)+{+structfsl_hv_ioctl_statusparam;+u32status;++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_status)))+return-EFAULT;++param.ret=fh_partition_get_status(param.partition,&status);+if(!param.ret)+param.status=status;++if(copy_to_user(p,¶m,sizeof(structfsl_hv_ioctl_status)))+return-EFAULT;++return0;+}++/**+*ioctl_start:ioctlinterfaceforFSL_HV_IOCTL_PARTITION_START+*+*Startastoppedpartition.+*/+staticlongioctl_start(structfsl_hv_ioctl_start__user*p)+{+structfsl_hv_ioctl_startparam;++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_start)))+return-EFAULT;++param.ret=fh_partition_start(param.partition,param.entry_point,+param.load);++if(copy_to_user(&p->ret,¶m.ret,sizeof(__u32)))+return-EFAULT;++return0;+}++/**+*ioctl_stop:ioctlinterfaceforFSL_HV_IOCTL_PARTITION_STOP+*+*Stoparunningpartition+*/+staticlongioctl_stop(structfsl_hv_ioctl_stop__user*p)+{+structfsl_hv_ioctl_stopparam;++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_stop)))+return-EFAULT;++param.ret=fh_partition_stop(param.partition);++if(copy_to_user(&p->ret,¶m.ret,sizeof(__u32)))+return-EFAULT;++return0;+}++/**+*ioctl_memcpy:ioctlinterfaceforFSL_HV_IOCTL_MEMCPY+*+*TheFH_MEMCPYhypercalltakesanarrayofaddress/address/sizestructures+*torepresentthedatabeingcopied.Asaconveniencetotheuser,this+*ioctltakesauser-createbufferandapointertoaguestphysically+*contiguousbufferintheremotepartition,andcreatesthe+*address/address/sizearrayforthehypercall.+*/+staticlongioctl_memcpy(structfsl_hv_ioctl_memcpy__user*p)+{+structfsl_hv_ioctl_memcpyparam;++structpage**pages=NULL;+void*sg_list_unaligned=NULL;+structfh_sg_list*sg_list=NULL;++unsignedintnr_pages;+unsignedlonglb_offset;/* Offset within a page of the local buffer */++unsignedinti;+longret=0;+phys_addr_tremote_paddr;/* The next address in the remote buffer */+uint32_tcount;/* The number of bytes left to copy */++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_memcpy)))+return-EFAULT;++/* One partition must be local, the other must be remote. In other+words,ifsourceandtargetareboth-1,orarebothnot-1,then+returnanerror.*/+if((param.source==-1)==(param.target==-1))+return-EINVAL;++/*+*Thearrayofpagesreturnedbyget_user_pages()coversonly+*page-alignedmemory.Sincetheuserbufferisprobablynot+*page-aligned,weneedtohandlethediscrepancy.+*+*WecalculatetheoffsetwithinapageoftheS/Glist,andmake+*adjustmentsaccordingly.Thiswillresultinapagelistthatlooks+*likethis:+*+*----<--firstpagestartsbeforethebuffer+*||+*|////|-> ----+*|////| | |+*----||+*||+*----||+*|////| | |+*|////| | |+*|////| | |+*----||+*||+*----||+*|////| | |+*|////| | |+*|////| | |+*----||+*||+*----||+*|////| | |+*|////|-> ----+*||<--lastpageendsafterthebuffer+*----+*+*Thedistancebetweenthestartofthefirstpageandthestartofthe+*bufferislb_offset.Thehashed(///) areas are the parts of the+*pagelistthatcontaintheactualbuffer.+*+*Theadvantageofthisapproachisthatthenumberofpagesis+*equaltothenumberofentriesintheS/Glistthatwegivetothe+*hypervisor.+*/+lb_offset=param.local_vaddr&(PAGE_SIZE-1);+nr_pages=(param.count+lb_offset+PAGE_SIZE-1)>>PAGE_SHIFT;++/* Allocate the buffers we need */++/* pages is an array of struct page pointers that's initialized by+get_user_pages()*/+pages=kzalloc(nr_pages*sizeof(structpage*),GFP_KERNEL);+if(!pages){+pr_debug("fsl-hv: could not allocate page list\n");+return-ENOMEM;+}++/* sg_list is the list of fh_sg_list objects that we pass to the+hypervisor*/+sg_list_unaligned=kmalloc(nr_pages*sizeof(structfh_sg_list)++sizeof(structfh_sg_list)-1,GFP_KERNEL);+if(!sg_list_unaligned){+pr_debug("fsl-hv: could not allocate S/G list\n");+return-ENOMEM;+}+sg_list=PTR_ALIGN(sg_list_unaligned,sizeof(structfh_sg_list));++/* Get the physical addresses of the source buffer */+down_read(¤t->mm->mmap_sem);+ret=get_user_pages(current,current->mm,+param.local_vaddr-lb_offset,nr_pages,+(param.source==-1)?READ:WRITE,+0,pages,NULL);+up_read(¤t->mm->mmap_sem);++if(ret!=nr_pages){+/* get_user_pages() failed */+pr_debug("fsl-hv: could not lock source buffer\n");+ret=-EACCES;+gotoexit;+}++/* reset ret here */+ret=0;++/* Build the fh_sg_list[] array. The first page is special+becauseit'smisaligned.*/+if(param.source==-1){+sg_list[0].source=page_to_phys(pages[0])+lb_offset;+sg_list[0].target=param.remote_paddr;+}else{+sg_list[0].source=param.remote_paddr;+sg_list[0].target=page_to_phys(pages[0])+lb_offset;+}+sg_list[0].size=min_t(uint64_t,param.count,PAGE_SIZE-lb_offset);++remote_paddr=param.remote_paddr+sg_list[0].size;+count=param.count-sg_list[0].size;++for(i=1;i<nr_pages;i++){+if(param.source==-1){+/* local to remote */+sg_list[i].source=page_to_phys(pages[i]);+sg_list[i].target=remote_paddr;+}else{+/* remote to local */+sg_list[i].source=remote_paddr;+sg_list[i].target=page_to_phys(pages[i]);+}+sg_list[i].size=min_t(uint64_t,count,PAGE_SIZE);++remote_paddr+=sg_list[i].size;+count-=sg_list[i].size;+}++param.ret=fh_partition_memcpy(param.source,param.target,+virt_to_phys(sg_list),nr_pages);++exit:+if(pages){+for(i=0;i<nr_pages;i++)+if(pages[i])+page_cache_release(pages[i]);+}++kfree(sg_list_unaligned);+kfree(pages);++if(!ret)+if(copy_to_user(&p->ret,¶m.ret,sizeof(__u32)))+return-EFAULT;++returnret;+}++/**+*ioctl_doorbell:ioctlinterfaceforFSL_HV_IOCTL_DOORBELL+*+*Ringadoorbell+*/+staticlongioctl_doorbell(structfsl_hv_ioctl_doorbell__user*p)+{+structfsl_hv_ioctl_doorbellparam;++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_doorbell)))+return-EFAULT;++param.ret=ev_doorbell_send(param.doorbell);++if(copy_to_user(&p->ret,¶m.ret,sizeof(__u32)))+return-EFAULT;++return0;+}++staticchar*strdup_from_user(constchar__user*ustr,size_tmax)+{+size_tlen;+char*str;++len=strnlen_user(ustr,max);+if(len>max)+returnERR_PTR(-ENAMETOOLONG);++str=kmalloc(len,GFP_KERNEL);+if(!str)+returnERR_PTR(-ENOMEM);++if(copy_from_user(str,ustr,len))+returnERR_PTR(-EFAULT);++returnstr;+}++staticlongioctl_dtprop(structfsl_hv_ioctl_prop__user*p,intset)+{+structfsl_hv_ioctl_propparam;+char__user*upath,*upropname;+void__user*upropval;+char*path=NULL,*propname=NULL;+void*propval=NULL;+intret=0;++/* Get the parameters from the user */+if(copy_from_user(¶m,p,sizeof(structfsl_hv_ioctl_prop)))+return-EFAULT;++upath=(char__user*)(uintptr_t)param.path;+upropname=(char__user*)(uintptr_t)param.propname;+upropval=(void__user*)(uintptr_t)param.propval;++path=strdup_from_user(upath,FH_DTPROP_MAX_PATHLEN);+if(IS_ERR(path)){+ret=PTR_ERR(path);+gotoout;+}++propname=strdup_from_user(upropname,FH_DTPROP_MAX_PATHLEN);+if(IS_ERR(propname)){+ret=PTR_ERR(propname);+gotoout;+}++if(param.proplen>FH_DTPROP_MAX_PROPLEN){+ret=-EINVAL;+gotoout;+}++propval=kmalloc(param.proplen,GFP_KERNEL);+if(!propval){+ret=-ENOMEM;+gotoout;+}++if(set){+if(copy_from_user(propval,upropval,param.proplen)){+ret=-EFAULT;+gotoout;+}++param.ret=fh_partition_set_dtprop(param.handle,+virt_to_phys(path),+virt_to_phys(propname),+virt_to_phys(propval),+param.proplen);+}else{+param.ret=fh_partition_get_dtprop(param.handle,+virt_to_phys(path),+virt_to_phys(propname),+virt_to_phys(propval),+¶m.proplen);++if(param.ret==0){+if(copy_to_user(upropval,propval,param.proplen)||+put_user(param.proplen,&p->proplen)){+ret=-EFAULT;+gotoout;+}+}+}++if(put_user(param.ret,&p->ret))+ret=-EFAULT;++out:+kfree(path);+kfree(propval);+kfree(propname);++returnret;+}++/**+*fsl_hv_ioctl:ioctlmainentrypoint+*/+staticlongfsl_hv_ioctl(structfile*file,unsignedintcmd,+unsignedlongargaddr)+{+unionfsl_hv_ioctl_param__user*arg=+(unionfsl_hv_ioctl_param__user*)argaddr;+longret;++/* Make sure the application is called the right driver. */+if(_IOC_TYPE(cmd)!=0){+pr_debug("fsl-hv: ioctl type %u should be 0\n",_IOC_TYPE(cmd));+return-EINVAL;+}++/* Make sure the application set the direction flag correctly. */+if(_IOC_DIR(cmd)!=(_IOC_READ|_IOC_WRITE)){+pr_debug("fsl-hv: ioctl direction should be _IOWR\n");+return-EINVAL;+}++/* Make sure the application is passing the right structure to us. */+if(_IOC_SIZE(cmd)<sizeof(unionfsl_hv_ioctl_param)){+pr_debug("fsl-hv: ioctl size %u is too small (should be %u)\n",+_IOC_SIZE(cmd),sizeof(unionfsl_hv_ioctl_param));+return-EINVAL;+}++switch(_IOC_NR(cmd)){+caseFSL_HV_IOCTL_PARTITION_RESTART:+ret=ioctl_restart(&arg->restart);+break;+caseFSL_HV_IOCTL_PARTITION_GET_STATUS:+ret=ioctl_status(&arg->status);+break;+caseFSL_HV_IOCTL_PARTITION_START:+ret=ioctl_start(&arg->start);+break;+caseFSL_HV_IOCTL_PARTITION_STOP:+ret=ioctl_stop(&arg->stop);+break;+caseFSL_HV_IOCTL_MEMCPY:+ret=ioctl_memcpy(&arg->memcpy);+break;+caseFSL_HV_IOCTL_DOORBELL:+ret=ioctl_doorbell(&arg->doorbell);+break;+caseFSL_HV_IOCTL_GETPROP:+ret=ioctl_dtprop(&arg->prop,0);+break;+caseFSL_HV_IOCTL_SETPROP:+ret=ioctl_dtprop(&arg->prop,1);+break;+default:+pr_debug("fsl-hv: unknown ioctl %u\n",cmd);+ret=-ENOIOCTLCMD;+break;+}++returnret;+}++/* Linked list of processes that have us open */+structlist_headdb_list;++/* spinlock for db_list */+staticDEFINE_SPINLOCK(db_list_lock);++/* The size of the doorbell event queue. This must be a power of two. */+#define QSIZE 16++/* Returns the next head/tail pointer, wrapping around the queue if necessary */+#define nextp(x) (((x) + 1) & (QSIZE - 1))++/* Per-open data structure */+structdoorbell_queue{+structlist_headlist;+spinlock_tlock;+wait_queue_head_twait;+unsignedinthead;+unsignedinttail;+uint32_tq[QSIZE];+};++/* Linked list of ISRs that we registered */+structlist_headisr_list;++/* Per-ISR data structure */+structdoorbell_isr{+structlist_headlist;+unsignedintirq;+uint32_tdoorbell;/* The doorbell handle */+uint32_tpartition;/* The partition handle, if used */+structwork_structwork;+};++/**+*fsl_hv_isr-interrupthandlerforalldoorbells+*@paramirq-theIRQ(a.k.a.receivehandle)+*+*Weusethesameinterrupthandlerforalldoorbells.Wheneveradoorbell+*isrung,andwereceiveaninterrupt,wejustputthehandleforthat+*doorbell(passedtousas*data)intoallofthequeues.+*+*/+staticirqreturn_tfsl_hv_isr(intirq,void*data)+{+structdoorbell_queue*dbq;+unsignedlongflags;++/* Prevent another core from modifying db_list */+spin_lock_irqsave(&db_list_lock,flags);++list_for_each_entry(dbq,&db_list,list){+if(dbq->head!=nextp(dbq->tail)){+dbq->q[dbq->tail]=(uint32_t)(uintptr_t)data;+/* This memory barrier eliminates the need to grab+*thespinlockfordbq.+*/+smp_wmb();+dbq->tail=nextp(dbq->tail);+wake_up_interruptible(&dbq->wait);+}+}++spin_unlock_irqrestore(&db_list_lock,flags);++returnIRQ_HANDLED;+}++/**+*fsl_hv_state_change_work_func--statechangeworkerfunction+*+*Thestatechangenotificationarrivesinaninterrupt,butwecan'tcall+*blocking_notifier_call_chain()inaninterrupthandler.Wecouldcall+*atomic_notifier_call_chain(),butthatwouldrequiretheclients'call-back+*functiontorunininterruptcontext.Sincewedon'twanttoimposethat+*restrictionontheclients,wecreateaworkqueuetoprocessthe+*notificationinkernelcontext.+*/+staticvoidfsl_hv_state_change_work_func(structwork_struct*work)+{+structdoorbell_isr*dbisr=+container_of(work,structdoorbell_isr,work);++blocking_notifier_call_chain(&failover_subscribers,dbisr->partition,+NULL);+}++/**+*fsl_hv_state_change_isr-interrupthandlerforstate-changedoorbells+*/+staticirqreturn_tfsl_hv_state_change_isr(intirq,void*data)+{+unsignedintstatus;+structdoorbell_isr*dbisr=data;+intret;++/* Determine the new state, and if it's stopped, notify the clients. */+ret=fh_partition_get_status(dbisr->partition,&status);+if(!ret&&(status==FH_PARTITION_STOPPED))+schedule_work(&dbisr->work);++/* Call the normal handler */+returnfsl_hv_isr(irq,(void*)(uintptr_t)dbisr->doorbell);+}++/**+*fsl_hv_poll-returnsabitmaskindicatingwhetherareadwillblock+*+*@returnunsignedint+*/+staticunsignedintfsl_hv_poll(structfile*filp,structpoll_table_struct*p)+{+structdoorbell_queue*dbq=filp->private_data;+unsignedlongflags;+unsignedintmask;++spin_lock_irqsave(&dbq->lock,flags);++poll_wait(filp,&dbq->wait,p);+mask=(dbq->head==dbq->tail)?0:(POLLIN|POLLRDNORM);++spin_unlock_irqrestore(&dbq->lock,flags);++returnmask;+}++/**+*fsl_hv_read-returnthehandlesforanyincomingdoorbells+*+*Iftherearedoorbellhandlesinthequeueforthisopeninstance,then+*returnthemtothecallerasanarrayof32-bitintegers.Otherwise,+*blockuntilthereisatleastonehandletoreturn.+*/+staticssize_tfsl_hv_read(structfile*filp,char__user*buf,size_tlen,+loff_t*off)+{+structdoorbell_queue*dbq=filp->private_data;+uint32_t__user*p=(uint32_t__user*)buf;/* for put_user() */+unsignedlongflags;+ssize_tcount=0;++/* Make sure we stop when the user buffer is full. */+while(len>=sizeof(uint32_t)){+uint32_tdbell;/* Local copy of doorbell queue data */++spin_lock_irqsave(&dbq->lock,flags);++/* If the queue is empty, then either we're done or we need+*toblock.IftheapplicationspecifiedO_NONBLOCK,then+*wereturntheappropriateerrorcode.+*/+if(dbq->head==dbq->tail){+spin_unlock_irqrestore(&dbq->lock,flags);+if(count)+break;+if(filp->f_flags&O_NONBLOCK)+return-EAGAIN;+if(wait_event_interruptible(dbq->wait,+dbq->head!=dbq->tail))+return-ERESTARTSYS;+continue;+}++/* Even though we have an smp_wmb() in the ISR, the core+*mightspeculativelyexecutethe"dbell = ..."belowwhile+*it'sevaluatingtheif-statementabove.Inthatcase,the+*valueputintodbellcouldbestaleifthecoreacceptsthe+*speculation.Topreventthat,weneedareadmemorybarrier+*hereaswell.+*/+smp_rmb();++/* Copy the data to a temporary local buffer, because+*wecan'tcallcopy_to_user()frominsideaspinlock+*/+dbell=dbq->q[dbq->head];+dbq->head=nextp(dbq->head);++spin_unlock_irqrestore(&dbq->lock,flags);++if(put_user(dbell,p))+return-EFAULT;+p++;+count+=sizeof(uint32_t);+len-=sizeof(uint32_t);+}++returncount;+}++/**+*fsl_hv_open-openthedriver+*+*Openthedriverandprepareforreadingdoorbells.+*+*Everytimeanapplicationopensthedriver,wecreateadoorbellqueue+*forthatfilehandle.Thisqueueisusedforanyincomingdoorbells.+*/+staticintfsl_hv_open(structinode*inode,structfile*filp)+{+structdoorbell_queue*dbq;+unsignedlongflags;+intret=0;++dbq=kzalloc(sizeof(structdoorbell_queue),GFP_KERNEL);+if(!dbq){+pr_err("fsl-hv: out of memory\n");+return-ENOMEM;+}++spin_lock_init(&dbq->lock);+init_waitqueue_head(&dbq->wait);++spin_lock_irqsave(&db_list_lock,flags);+list_add(&dbq->list,&db_list);+spin_unlock_irqrestore(&db_list_lock,flags);++filp->private_data=dbq;++returnret;+}++/**+*fsl_hv_close-closethedriver+*/+staticintfsl_hv_close(structinode*inode,structfile*filp)+{+structdoorbell_queue*dbq=filp->private_data;+unsignedlongflags;++intret=0;++spin_lock_irqsave(&db_list_lock,flags);+list_del(&dbq->list);+spin_unlock_irqrestore(&db_list_lock,flags);++kfree(dbq);++returnret;+}++staticconststructfile_operationsfsl_hv_fops={+.owner=THIS_MODULE,+.open=fsl_hv_open,+.release=fsl_hv_close,+.poll=fsl_hv_poll,+.read=fsl_hv_read,+.unlocked_ioctl=fsl_hv_ioctl,+};++staticstructmiscdevicefsl_hv_misc_dev={+MISC_DYNAMIC_MINOR,+"fsl-hv",+&fsl_hv_fops+};++staticDECLARE_WORK(power_off,(work_func_t)kernel_power_off);++staticirqreturn_tfsl_hv_shutdown_isr(intirq,void*data)+{+schedule_work(&power_off);++/* We should never get here */+returnIRQ_NONE;+}++/**+*get_parent_handle--returnsthehandleoftheparentofthegivennode+*+*Thehandleisthevalueofthe'reg'property+*/+staticintget_parent_handle(structdevice_node*np)+{+structdevice_node*parent;+constuint32_t*prop;+intlen;++parent=of_get_parent(np);+if(!parent)+/* It's not really possible for this to fail */+return-ENODEV;++prop=of_get_property(parent,"reg",&len);+of_node_put(parent);++if(!prop||(len!=sizeof(uint32_t)))+/* This can happen only if the node is malformed */+return-ENODEV;++return*prop;+}++/**+*fsl_hv_failover_register--registeracallbackforfailoverevents+*+*Thisfunctioniscalledbydevicedriverstoregistertheircallback+*functionsforfail-overevents.+*/+intfsl_hv_failover_register(structnotifier_block*nb)+{+returnblocking_notifier_chain_register(&failover_subscribers,nb);+}+EXPORT_SYMBOL(fsl_hv_failover_register);++/**+*fsl_hv_failover_unregister--unregisteracallbackforfailoverevents+*/+intfsl_hv_failover_unregister(structnotifier_block*nb)+{+returnblocking_notifier_chain_unregister(&failover_subscribers,nb);+}+EXPORT_SYMBOL(fsl_hv_failover_unregister);++/**+*has_fsl_hypervisor-returnTRUEifwe'rerunningunderFSLhypervisor+*+*Thisfunctioncheckstoseeifwe'rerunningundertheFreescale+*hypervisor,andreturnszeroifwe'renot,ornon-zeroifweare.+*+*First,itchecksifMSR[GS]==1,whichmeanswe'rerunningundersome+*hypervisor.Thenitchecksifthereisahypervisornodeinthedevice+*tree.Currently,thatmeansthereneedstobeanodeintherootcalled+*"hypervisor"andwhichhasapropertynamed"fsl,hv-version".+*/+staticinthas_fsl_hypervisor(void)+{+structdevice_node*node;+intret;++if(!(mfmsr()&MSR_GS))+return0;++node=of_find_node_by_path("/hypervisor");+if(!node)+return0;++ret=of_find_property(node,"fsl,hv-version",NULL)!=NULL;++of_node_put(node);++returnret;+}++/**+*fsl_hypervisor_init:Freescalehypervisormanagementdriverinit+*+*Thisfunctioniscalledwhenthismoduleisloaded.+*+*Registerourselvesasamiscellaneousdriver.Thiswillregisterthe+*fopsstructureandcreatetherightsysfsentriesforudev.+*/+staticint__initfsl_hypervisor_init(void)+{+structdevice_node*np;+structdoorbell_isr*dbisr,*n;+intret;++pr_info("Freescale hypervisor management driver\n");++if(!has_fsl_hypervisor()){+pr_info("fsl-hv: no hypervisor found\n");+return-ENODEV;+}++ret=misc_register(&fsl_hv_misc_dev);+if(ret){+pr_err("fsl-hv: cannot register device\n");+returnret;+}++INIT_LIST_HEAD(&db_list);+INIT_LIST_HEAD(&isr_list);++for_each_compatible_node(np,NULL,"epapr,hv-receive-doorbell"){+unsignedintirq;+constuint32_t*handle;++handle=of_get_property(np,"interrupts",NULL);+irq=irq_of_parse_and_map(np,0);+if(!handle||(irq==NO_IRQ)){+pr_err("fsl-hv: no 'interrupts' property in %s node\n",+np->full_name);+continue;+}++dbisr=kzalloc(sizeof(*dbisr),GFP_KERNEL);+if(!dbisr)+gotoout_of_memory;++dbisr->irq=irq;+dbisr->doorbell=*handle;+INIT_WORK(&dbisr->work,fsl_hv_state_change_work_func);++if(of_device_is_compatible(np,"fsl,hv-shutdown-doorbell")){+/* The shutdown doorbell gets its own ISR */+ret=request_irq(irq,fsl_hv_shutdown_isr,0,+np->name,dbisr);+}elseif(of_device_is_compatible(np,+"fsl,hv-state-change-doorbell")){+/* The state change doorbell triggers a notification if+*thestateofthemanagedpartitionchangesto+*"stopped".Weneedaseparateinterrupthandlerfor+*that,andwealsoneedtoknowthehandleofthe+*targetpartition,notjustthehandleofthe+*doorbell.+*/+dbisr->partition=ret=get_parent_handle(np);+if(ret<0){+pr_err("fsl-hv: node %s has missing or "+"malformed parent\n",np->full_name);+kfree(dbisr);+continue;+}+ret=request_irq(irq,fsl_hv_state_change_isr,0,+np->name,dbisr);+}else+ret=request_irq(irq,fsl_hv_isr,0,np->name,dbisr);++if(ret<0){+pr_err("fsl-hv: could not request irq %u for node %s\n",+irq,np->full_name);+kfree(dbisr);+continue;+}++list_add(&dbisr->list,&isr_list);++pr_info("fsl-hv: registered handler for doorbell %u\n",+*handle);+}++return0;++out_of_memory:+list_for_each_entry_safe(dbisr,n,&isr_list,list){+free_irq(dbisr->irq,dbisr);+list_del(&dbisr->list);+kfree(dbisr);+}++misc_deregister(&fsl_hv_misc_dev);++return-ENOMEM;+}++/**+*fsl_hypervisor_exit:Freescalehypervisormanagementdrivertermination+*+*Thisfunctioniscalledwhenthisdriverisunloaded.+*/+staticvoid__exitfsl_hypervisor_exit(void)+{+structdoorbell_isr*dbisr,*n;++list_for_each_entry_safe(dbisr,n,&isr_list,list){+free_irq(dbisr->irq,dbisr);+list_del(&dbisr->list);+kfree(dbisr);+}++misc_deregister(&fsl_hv_misc_dev);+}++module_init(fsl_hypervisor_init);+module_exit(fsl_hypervisor_exit);++MODULE_AUTHOR("Timur Tabi <timur@freescale.com>");+MODULE_DESCRIPTION("Freescale hypervisor management driver");+MODULE_LICENSE("GPL v2");
@@ -0,0 +1,203 @@+/*+*Freescalehypervisorioctlinterface+*+*Copyright(C)2008-2011FreescaleSemiconductor,Inc.+*+*ThisfileislicensedunderthetermsoftheGNUGeneralPublicLicense+*version2.Thisprogramislicensed"as is"withoutanywarrantyofany+*kind,whetherexpressorimplied.+*+*ThisfileisusedbytheFreescalehypervisormanagementdriver.Itcan+*alsobeincludedbyapplicationsthatneedtocommunicatewiththedriver+*viatheioctlinterface.+*/++#ifndef FSL_HYPERVISOR_H+#define FSL_HYPERVISOR_H++#include<linux/types.h>++/**+*Freescalehypervisorioctlparameter+*/+unionfsl_hv_ioctl_param{++/**+*@ret:Returnvalue.+*+*Thisisalwaysthefirstwordofanystructure.+*/+__u32ret;++/**+*structfsl_hv_ioctl_restart:restartapartition+*@ret:returnerrorcodefromthehypervisor+*@partition:theIDofthepartitiontorestart,or-1forthe+*callingpartition+*+*UsedbyFSL_HV_IOCTL_PARTITION_RESTART+*/+structfsl_hv_ioctl_restart{+__u32ret;+__u32partition;+}restart;++/**+*structfsl_hv_ioctl_status:getapartition'sstatus+*@ret:returnerrorcodefromthehypervisor+*@partition:theIDofthepartitiontoquery,or-1forthe+*callingpartition+*@status:Thereturnedstatusofthepartition+*+*UsedbyFSL_HV_IOCTL_PARTITION_GET_STATUS+*+*Valuesof'status':+*0=Stopped+*1=Running+*2=Starting+*3=Stopping+*/+structfsl_hv_ioctl_status{+__u32ret;+__u32partition;+__u32status;+}status;++/**+*structfsl_hv_ioctl_start:startapartition+*@ret:returnerrorcodefromthehypervisor+*@partition:theIDofthepartitiontocontrol+*@entry_point:TheoffsetwithintheguestIMAtostartexecution+*@load:Ifnon-zero,reloadthepartition'simagesbeforestarting+*+*UsedbyFSL_HV_IOCTL_PARTITION_START+*/+structfsl_hv_ioctl_start{+__u32ret;+__u32partition;+__u32entry_point;+__u32load;+}start;++/**+*structfsl_hv_ioctl_stop:stopapartition+*@ret:returnerrorcodefromthehypervisor+*@partition:theIDofthepartitiontostop,or-1forthecalling+*partition+*+*UsedbyFSL_HV_IOCTL_PARTITION_STOP+*/+structfsl_hv_ioctl_stop{+__u32ret;+__u32partition;+}stop;++/**+*structfsl_hv_ioctl_memcpy:copymemorybetweenpartitions+*@ret:returnerrorcodefromthehypervisor+*@source:thepartitionIDofthesourcepartition,or-1forthis+*partition+*@target:thepartitionIDofthetargetpartition,or-1forthis+*partition+*@local_addr:user-spacevirtualaddressofabufferinthelocal+*partition+*@remote_addr:guestphysicaladdressofabufferinthe+*remotepartition+*@count:thenumberofbytestocopy.Boththelocalandremote+*buffersmustbeatleast'count'byteslong+*+*UsedbyFSL_HV_IOCTL_MEMCPY+*+*The'local'partitionisthepartitionthatcallsthisioctl.The+*'remote'partitionisadifferentpartition.Thedataiscopiedfrom+*the'source'paritition'tothe'target'partition.+*+*Thebufferintheremotepartitionmustbeguestphysically+*contiguous.+*+*Thisioctldoesnotsupportcopyingmemorybetweentworemote+*partitionsorwithinthesamepartition,soeither'source'or+*'target'(butnotboth)mustbe-1.Inotherwords,either+*+*source==localandtarget==remote+*or+*source==remoteandtarget==local+*/+structfsl_hv_ioctl_memcpy{+__u32ret;+__u32source;+__u32target;+__u64local_vaddr;+__u64remote_paddr;+__u64count;+}memcpy;++/**+*structfsl_hv_ioctl_doorbell:ringadoorbell+*@ret:returnerrorcodefromthehypervisor+*@doorbell:thehandleofthedoorbelltoringdoorbell+*+*UsedbyFSL_HV_IOCTL_DOORBELL+*/+structfsl_hv_ioctl_doorbell{+__u32ret;+__u32doorbell;+}doorbell;++/**+*structfsl_hv_ioctl_prop:get/setadevicetreeproperty+*@ret:returnerrorcodefromthehypervisor+*@handle:handleofpartitionwhosetreetoaccess+*@path:virtualaddressofpathnameofnodetoaccess+*@propname:virtualaddressofnameofpropertytoaccess+*@propval:virtualaddressofpropertydatabuffer+*@proplen:Sizeofpropertydatabuffer+*+*UsedbyFSL_HV_IOCTL_DOORBELL+*/+structfsl_hv_ioctl_prop{+__u32ret;+__u32handle;+__u64path;+__u64propname;+__u64propval;+__u32proplen;+}prop;+};++/*+*ioctlcommands.+*/+enum{+FSL_HV_IOCTL_PARTITION_RESTART=1,/* Boot another partition */+FSL_HV_IOCTL_PARTITION_GET_STATUS=2,/* Boot another partition */+FSL_HV_IOCTL_PARTITION_START=3,/* Boot another partition */+FSL_HV_IOCTL_PARTITION_STOP=4,/* Stop this or another partition */+FSL_HV_IOCTL_MEMCPY=5,/* Copy data from one partition to another */+FSL_HV_IOCTL_DOORBELL=6,/* Ring a doorbell */++/* Get a property from another guest's device tree */+FSL_HV_IOCTL_GETPROP=7,++/* Set a property in another guest's device tree */+FSL_HV_IOCTL_SETPROP=8,+};++#ifdef __KERNEL__++/**+*fsl_hv_event_register--registeracallbackforfailoverevents+*+*Thisfunctioniscalledbydevicedriverstoregistertheircallback+*functionsforfail-overevents.+*/+intfsl_hv_failover_register(structnotifier_block*nb);++/**+*fsl_hv_event_unregister--unregisteracallbackforfailoverevents+*/+intfsl_hv_failover_unregister(structnotifier_block*nb);++#endif++#endif
O> + /* One partition must be local, the other must be remote. In other
+ words, if source and target are both -1, or are both not -1, then
+ return an error. */
+ if ((param.source == -1) == (param.target == -1))
+ return -EINVAL;
Excess brackets (I just mention that one in passing)
+ /* pages is an array of struct page pointers that's initialized by
+ get_user_pages() */
+ pages = kzalloc(nr_pages * sizeof(struct page *), GFP_KERNEL);
+ if (!pages) {
+ pr_debug("fsl-hv: could not allocate page list\n");
+ return -ENOMEM;
+ }
pages allocated
+
+ /* sg_list is the list of fh_sg_list objects that we pass to the
+ hypervisor */
+ sg_list_unaligned = kmalloc(nr_pages * sizeof(struct fh_sg_list) +
+ sizeof(struct fh_sg_list) - 1, GFP_KERNEL);
+ if (!sg_list_unaligned) {
+ pr_debug("fsl-hv: could not allocate S/G list\n");
but not freed on this path
Although the other paths look fine.
+exit:
+ if (pages) {
+ for (i = 0; i < nr_pages; i++)
+ if (pages[i])
+ page_cache_release(pages[i]);
+ }
+
+ kfree(sg_list_unaligned);
+ kfree(pages);
-ENOTTY
(-ENOIOCTLCMD is an internal indicator designed so driver layers can say
'dunno, try the next layer up')
+/* Linked list of processes that have us open */
+struct list_head db_list;
static ?
+ * We use the same interrupt handler for all doorbells. Whenever a doorbell
+ * is rung, and we receive an interrupt, we just put the handle for that
+ * doorbell (passed to us as *data) into all of the queues.
I wonder if these should be presented as IRQs or whether that makes no
sense ?
+static irqreturn_t fsl_hv_state_change_isr(int irq, void *data)
+{
+ unsigned int status;
+ struct doorbell_isr *dbisr = data;
+ int ret;
+
+ /* Determine the new state, and if it's stopped, notify the clients. */
+ ret = fh_partition_get_status(dbisr->partition, &status);
+ if (!ret && (status == FH_PARTITION_STOPPED))
+ schedule_work(&dbisr->work);
+
+ /* Call the normal handler */
+ return fsl_hv_isr(irq, (void *) (uintptr_t) dbisr->doorbell);
+}
Would a threaded IRQ clean this up by avoiding the queue/work stuff ?
+static irqreturn_t fsl_hv_shutdown_isr(int irq, void *data)
+{
+ schedule_work(&power_off);
+
+ /* We should never get here */
Probably worth printing something if you do (panic(...) ?)
From: Timur Tabi <hidden> Date: 2011-06-01 20:26:45
Alan Cox wrote:
O> + /* One partition must be local, the other must be remote. In other
quoted
+ words, if source and target are both -1, or are both not -1, then
+ return an error. */
+ if ((param.source == -1) == (param.target == -1))
+ return -EINVAL;
Excess brackets (I just mention that one in passing)'
Do you mean excess parentheses? If so, then I don't see how. "(param.source ==
-1)" and "(param.target == -1)" are expressions that return a boolean. I'm
comparing the two boolean results to see if they're equal. I want to make sure
that the compiler doesn't do something like this:
if (param.source == (-1 == (param.target == -1)))
I don't even know what that means, but it's not what I want.
This belongs on lib/ if its of general use which I think it perhaps is
and if we don't have one already.
Where exactly in lib/ should it go? lib/strings.c seems too low-level for a
function like this. lib/string_helpers.c is for sprintf-like functions. And
it's too generic for lib/powerpc/
-ENOTTY
(-ENOIOCTLCMD is an internal indicator designed so driver layers can say
'dunno, try the next layer up')
quoted
+/* Linked list of processes that have us open */
+struct list_head db_list;
static ?
quoted
+ * We use the same interrupt handler for all doorbells. Whenever a doorbell
+ * is rung, and we receive an interrupt, we just put the handle for that
+ * doorbell (passed to us as *data) into all of the queues.
I wonder if these should be presented as IRQs or whether that makes no
sense ?
Well, the "handles" are supposed to be just unique numbers. In this case, they
are IRQs, but we don't want to expose that. The application is supposed to scan
the device tree looking for the doorbell nodes that it wants, and in those nodes
there is a 'reg' property that contains the handle for that doorbell. So the
numbers just need to match. What they represent is not relevant.
quoted
+static irqreturn_t fsl_hv_state_change_isr(int irq, void *data)
+{
+ unsigned int status;
+ struct doorbell_isr *dbisr = data;
+ int ret;
+
+ /* Determine the new state, and if it's stopped, notify the clients. */
+ ret = fh_partition_get_status(dbisr->partition, &status);
+ if (!ret && (status == FH_PARTITION_STOPPED))
+ schedule_work(&dbisr->work);
+
+ /* Call the normal handler */
+ return fsl_hv_isr(irq, (void *) (uintptr_t) dbisr->doorbell);
+}
Would a threaded IRQ clean this up by avoiding the queue/work stuff ?
Yes, I think so. V3 coming up.
--
Timur Tabi
Linux kernel developer at Freescale
On Wed, 1 Jun 2011 15:24:51 -0500
Timur Tabi [off-list ref] wrote:
Alan Cox wrote:
quoted
O> + /* One partition must be local, the other must be remote. In other
quoted
+ words, if source and target are both -1, or are both not -1, then
+ return an error. */
+ if ((param.source == -1) == (param.target == -1))
+ return -EINVAL;
Excess brackets (I just mention that one in passing)'
Do you mean excess parentheses? If so, then I don't see how. "(param.source ==
-1)" and "(param.target == -1)" are expressions that return a boolean. I'm
comparing the two boolean results to see if they're equal
Ok - it's a bit non obvious but yes fair enough and it is commented.
Where exactly in lib/ should it go? lib/strings.c seems too low-level for a
function like this. lib/string_helpers.c is for sprintf-like functions. And
it's too generic for lib/powerpc/
I'd submit it to lib/string personally but I'm not sure where would be
better. If someone doesn't like lib/string they can suggest a better
place 8)
Well, the "handles" are supposed to be just unique numbers. In this case, they
are IRQs, but we don't want to expose that. The application is supposed to scan
the device tree looking for the doorbell nodes that it wants, and in those nodes
there is a 'reg' property that contains the handle for that doorbell. So the
numbers just need to match. What they represent is not relevant.
Ok so they are always going to be exposed to users not to drivers, in
which case yes it makes sense. If they are going to get used by kernel
drivers as well an IRQ interface would probably also make sense.
Alan
Memory leak on the EFAULT path
If strnlen_user gets an exception, it'll return zero, causing a
zero-length kmalloc. Will kmalloc(0, ...) return NULL? If so, a bad
user pointer would result in -ENOMEM rather than -EFAULT.
-ENOTTY
(-ENOIOCTLCMD is an internal indicator designed so driver layers can say
'dunno, try the next layer up')
There's a check for -ENOIOCTLCMD in vfs_ioctl() -- though it generates
-EINVAL rather than -ENOTTY (why?).
Using -ENOIOCTLCMD consistently would make it easier to refactor a
toplevel ioctl handler into a nested one, plus consistency is good in
general.
quoted
+ * We use the same interrupt handler for all doorbells. Whenever a doorbell
+ * is rung, and we receive an interrupt, we just put the handle for that
+ * doorbell (passed to us as *data) into all of the queues.
I wonder if these should be presented as IRQs or whether that makes no
sense ?
They are presented as IRQs. This driver registers the same handler for all
doorbell IRQs, and pipes the notifications up to userspace, but you could
also directly register a kernel handler for an individual doorbell IRQ.
quoted
+static irqreturn_t fsl_hv_shutdown_isr(int irq, void *data)
+{
+ schedule_work(&power_off);
+
+ /* We should never get here */
Probably worth printing something if you do (panic(...) ?)
I don't think the comment is accurate. We've just scheduled the workqueue,
not waited for it to complete.
Timur, shouldn't this schedule orderly_poweroff rather than
kernel_power_off?
-Scott
The Freescale hypervisor management driver provides several services to
drivers and applications related to the Freescale hypervisor:
1. An ioctl interface for querying and managing partitions
2. A file interface to reading incoming doorbells
3. An interrupt handler for shutting down the partition upon receiving the
shutdown doorbell from a manager partition
4. An interface for receiving callbacks when a managed partition shuts down.
Signed-off-by: Timur Tabi <redacted>
---
drivers/misc/Kconfig | 7 +
drivers/misc/Makefile | 1 +
drivers/misc/fsl_hypervisor.c | 941 ++++++++++++++++++++++++++++++++++++++++
include/linux/Kbuild | 1 +
include/linux/fsl_hypervisor.h | 203 +++++++++
5 files changed, 1153 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/fsl_hypervisor.c
create mode 100644 include/linux/fsl_hypervisor.h
I think drivers/misc is not the right place for this, but I'm not completely
sure what is. drivers/firmware would be better at least, but virt/fsl might
also be ok.
I'm not convinced that an ioctl interface is the right way to work with
device tree properties. A more natural way would be to export it as
a file system, or maybe as a flattened device tree blob (the latter option
would require changing the hypervisor interface, which might not be
possible).
+/**
+ * fsl_hv_ioctl: ioctl main entry point
+ */
+static long fsl_hv_ioctl(struct file *file, unsigned int cmd,
+ unsigned long argaddr)
+{
+ union fsl_hv_ioctl_param __user *arg =
+ (union fsl_hv_ioctl_param __user *)argaddr;
+ long ret;
+
For an ioctl, please follow the normal pattern of defining a separate
structure for each case, no union.
You can use a void __user * in the common ioctl function, and pass that
to the typed argument list in the specific functions.
Arnd
I'm not convinced that an ioctl interface is the right way to work with
device tree properties. A more natural way would be to export it as
a file system, or maybe as a flattened device tree blob (the latter option
would require changing the hypervisor interface, which might not be
possible).
I wanted to have the hypervisor take an update dtb (we already have special
meta-properties for things like deletion as part of the hv config
mechanism). But others on the project wanted to keep it simple, and so
get/set property it was. :-/
It's unlikely to change at this point without a real need.
As for a filesystem interface, it's not a good match either. You can't
iterate over anything to read out the full tree from the hv. You can't
delete anything. You can't create empty nodes. The hv interface was meant
to enable some specific management actions, rather than to provide general
device tree access. This driver is a thin wrapper around the management
hcalls.
There would still be other ioctls needed for starting/stopping the
partition, etc.
quoted
+/**
+ * fsl_hv_ioctl: ioctl main entry point
+ */
+static long fsl_hv_ioctl(struct file *file, unsigned int cmd,
+ unsigned long argaddr)
+{
+ union fsl_hv_ioctl_param __user *arg =
+ (union fsl_hv_ioctl_param __user *)argaddr;
+ long ret;
+
For an ioctl, please follow the normal pattern of defining a separate
structure for each case, no union.
And have fsl_hypervisor.h provide the full set of proper ioctl numbers, with
the specific struct for each ioctl referenced, rather than having the client program do
"ioctl(f, _IOWR(0, cmd, union fsl_hv_ioctl_param), p)".
-Scott
From: Timur Tabi <hidden> Date: 2011-06-02 21:28:54
Arnd Bergmann wrote:
I think drivers/misc is not the right place for this, but I'm not completely
sure what is. drivers/firmware would be better at least, but virt/fsl might
also be ok.
I don't think it's correct to think of a hypervisor as firmware, so I don't
think drivers/firmware is better.
I'm not sure that creating virt/fsl and putting the driver in there is a good
idea, because it will be the only driver in that directory. Unlike KVM, this
driver is just a collection of front-ends to our hypervisor API. The actual
hypervisor is completely separate. That's why I put it in drivers/misc, because
it's just a single driver with a miscellaneous collection of interfaces.
We also don't want to have any Kconfig options that "turn on" hypervisor
support. I've intentionally made support for the hypervisor part of the normal
platform code, and the device tree is used to determine whether that code is
active or not.
So virt/fsl seems like overkill to me.
I'm not convinced that an ioctl interface is the right way to work with
device tree properties. A more natural way would be to export it as
a file system, or maybe as a flattened device tree blob (the latter option
would require changing the hypervisor interface, which might not be
possible).
As Scott said, this is just a front-end to the hypervisor API, and we already
have applications that depend on the ioctl interface. Considering that this
driver is specific to the Freescale hypervisor, so I don't see any harm in using
ioctls.
For an ioctl, please follow the normal pattern of defining a separate
structure for each case, no union.
Ok. This will break our existing applications, but it's a minor fix.
--
Timur Tabi
Linux kernel developer at Freescale
From: Timur Tabi <hidden> Date: 2011-06-03 14:44:31
Arnd Bergmann wrote:
For an ioctl, please follow the normal pattern of defining a separate
structure for each case, no union.
You can use a void __user * in the common ioctl function, and pass that
to the typed argument list in the specific functions.
I have a GPL question. This header file is currently licensed under the GPL v2
only. Does that mean that any application that includes this header file so
that it can talk to the driver/hypervisor also needs to be licensed under the GPL?
--
Timur Tabi
Linux kernel developer at Freescale
For an ioctl, please follow the normal pattern of defining a separate
structure for each case, no union.
You can use a void __user * in the common ioctl function, and pass that
to the typed argument list in the specific functions.
I have a GPL question. This header file is currently licensed under the GPL v2
only. Does that mean that any application that includes this header file so
that it can talk to the driver/hypervisor also needs to be licensed under the GPL?
If you have a license question, ask your lawyer.
Common answers that you would hear are:
* User space interfaces of the kernel are excluded from the License by the
"normal system call" exception in linux/COPYING.
* If the header files only contain interfaces but no code, they are not
copyrighted.
* If you want to ship a copy of the file with a user space application source,
but have to make it available under multiple licenses to do that, e.g. dual
GPL/BSD.
Arnd
I think drivers/misc is not the right place for this, but I'm not completely
sure what is. drivers/firmware would be better at least, but virt/fsl might
also be ok.
I don't think it's correct to think of a hypervisor as firmware, so I don't
think drivers/firmware is better.
I'm not sure that creating virt/fsl and putting the driver in there is a good
idea, because it will be the only driver in that directory. Unlike KVM, this
driver is just a collection of front-ends to our hypervisor API. The actual
hypervisor is completely separate. That's why I put it in drivers/misc, because
it's just a single driver with a miscellaneous collection of interfaces.
Ok, fair enough. If nobody has a strong preference any other way, just make it
drivers/firmware then.
We also don't want to have any Kconfig options that "turn on" hypervisor
support. I've intentionally made support for the hypervisor part of the normal
platform code, and the device tree is used to determine whether that code is
active or not.
So virt/fsl seems like overkill to me.
Ok. Obviously, you still have the option to move the code later if you require
more interfaces to talk to your hypervisor, and then you can still add virt/fsl,
so no problem.
quoted
I'm not convinced that an ioctl interface is the right way to work with
device tree properties. A more natural way would be to export it as
a file system, or maybe as a flattened device tree blob (the latter option
would require changing the hypervisor interface, which might not be
possible).
As Scott said, this is just a front-end to the hypervisor API, and we already
have applications that depend on the ioctl interface. Considering that this
driver is specific to the Freescale hypervisor, so I don't see any harm in using
ioctls.
Good interface design is always important, one of the reasons is serving as
an example for other people doing similar drivers. There are a lot of
companies writing hypervisors and they all need interfaces like this.
quoted
For an ioctl, please follow the normal pattern of defining a separate
structure for each case, no union.
Ok. This will break our existing applications, but it's a minor fix.
Ok. Better to break it now than accidentally break it later because of
some side-effect of a strange interface, e.g. when the union would change in
size.
Arnd
From: Timur Tabi <hidden> Date: 2011-06-03 15:28:13
Arnd Bergmann wrote:
quoted
quoted
I don't think it's correct to think of a hypervisor as firmware, so I don't
think drivers/firmware is better.
I'm not sure that creating virt/fsl and putting the driver in there is a good
idea, because it will be the only driver in that directory. Unlike KVM, this
driver is just a collection of front-ends to our hypervisor API. The actual
hypervisor is completely separate. That's why I put it in drivers/misc, because
it's just a single driver with a miscellaneous collection of interfaces.
Ok, fair enough. If nobody has a strong preference any other way, just make it
drivers/firmware then.
Did you mean to say drivers/misc?
--
Timur Tabi
Linux kernel developer at Freescale
I wanted to have the hypervisor take an update dtb (we already have special
meta-properties for things like deletion as part of the hv config
mechanism). But others on the project wanted to keep it simple, and so
get/set property it was. :-/
It's unlikely to change at this point without a real need.
As for a filesystem interface, it's not a good match either.
You can't iterate over anything to read out the full tree from the hv.
kexec iterates over /proc/device-tree to create a dts blob.
You can't delete anything.
rm, rmdir
You can't create empty nodes.
mkdir
The hv interface was meant to enable some specific management actions,
rather than to provide general device tree access. This driver is a thin
wrapper around the management hcalls.
A file system would be a slightly more abstract interface to do the
same thing, I gues.
There would still be other ioctls needed for starting/stopping the
partition, etc.
Right, although you could model them as a file interface as well.
KVMfs is one example doing that.
Arnd
From: Scott Wood <hidden> Date: 2011-06-03 16:22:18
On Fri, 3 Jun 2011 17:28:43 +0200
Arnd Bergmann [off-list ref] wrote:
On Thursday 02 June 2011, Scott Wood wrote:
quoted
I wanted to have the hypervisor take an update dtb (we already have special
meta-properties for things like deletion as part of the hv config
mechanism). But others on the project wanted to keep it simple, and so
get/set property it was. :-/
It's unlikely to change at this point without a real need.
As for a filesystem interface, it's not a good match either.
You can't iterate over anything to read out the full tree from the hv.
kexec iterates over /proc/device-tree to create a dts blob.
That's irrelevant, because we're not talking about that device tree. We're
talking about the device tree of another hypervisor guest.
quoted
You can't delete anything.
rm, rmdir
quoted
You can't create empty nodes.
mkdir
I know how to operate a filesystem. You can't do these operations *on
another guest's device tree through the hv interface*.
quoted
There would still be other ioctls needed for starting/stopping the
partition, etc.
Right, although you could model them as a file interface as well.
KVMfs is one example doing that.
And what would be the benefit of this major restructuring and added
complexity?
-Scott
I don't think it's correct to think of a hypervisor as firmware, so I don't
think drivers/firmware is better.
I'm not sure that creating virt/fsl and putting the driver in there is a good
idea, because it will be the only driver in that directory. Unlike KVM, this
driver is just a collection of front-ends to our hypervisor API. The actual
hypervisor is completely separate. That's why I put it in drivers/misc, because
it's just a single driver with a miscellaneous collection of interfaces.
quoted
Ok, fair enough. If nobody has a strong preference any other way, just make it
drivers/firmware then.
Did you mean to say drivers/misc?
Sorry, I misread your first sentence above. I thought you said that you prefer
drivers/firmware over virt/fsl. drivers/misc is definitely the wrong
place for this, please choose a better one. Maybe drivers/virt/ ?
Arnd
From: Timur Tabi <hidden> Date: 2011-06-06 15:48:14
Arnd Bergmann wrote:
Sorry, I misread your first sentence above. I thought you said that you prefer
drivers/firmware over virt/fsl. drivers/misc is definitely the wrong
place for this, please choose a better one. Maybe drivers/virt/ ?
I'll be more than happy to go with the consensus, but I don't think it makes
sense to create a new directory just for this one, limited-use driver. You're
the only person who's complained about drivers/misc. I'm pretty sure that if I
put it in drivers/virt, I'll get more complaints.
I still don't understand what's wrong with drivers/misc, especially since my
driver registers as a "misc" driver.
--
Timur Tabi
Linux kernel developer at Freescale
On Fri, 3 Jun 2011 17:28:43 +0200
Arnd Bergmann [off-list ref] wrote:
quoted
On Thursday 02 June 2011, Scott Wood wrote:
quoted
I wanted to have the hypervisor take an update dtb (we already have special
meta-properties for things like deletion as part of the hv config
mechanism). But others on the project wanted to keep it simple, and so
get/set property it was. :-/
It's unlikely to change at this point without a real need.
As for a filesystem interface, it's not a good match either.
You can't iterate over anything to read out the full tree from the hv.
kexec iterates over /proc/device-tree to create a dts blob.
That's irrelevant, because we're not talking about that device tree. We're
talking about the device tree of another hypervisor guest.
I understand that it's a different device tree. That doesn't mean we
can't use the same tools.
quoted
quoted
You can't delete anything.
rm, rmdir
quoted
You can't create empty nodes.
mkdir
I know how to operate a filesystem. You can't do these operations *on
another guest's device tree through the hv interface*.
Why not? From a device driver perspective, it's not much of a difference
whether you export a device (or hypervisor, firmware, ...) setting
as an ioctl or an inode operation.
quoted
quoted
There would still be other ioctls needed for starting/stopping the
partition, etc.
Right, although you could model them as a file interface as well.
KVMfs is one example doing that.
And what would be the benefit of this major restructuring and added
complexity?
I think it would be a slightly better abstraction, and the complexity
is not as big as you make it sound. I'm mainly countering your statement
that it would be a bad interface or that would not possible to do.
I'm not that opposed to having an ioctl interface for your hypervisor
interface, but I am opposed to making design choices based on
a bad representations of facts or not having considered the options
that other people suggest.
Arnd
Sorry, I misread your first sentence above. I thought you said that you prefer
drivers/firmware over virt/fsl. drivers/misc is definitely the wrong
place for this, please choose a better one. Maybe drivers/virt/ ?
I'll be more than happy to go with the consensus, but I don't think it makes
sense to create a new directory just for this one, limited-use driver. You're
the only person who's complained about drivers/misc. I'm pretty sure that if I
put it in drivers/virt, I'll get more complaints.
I still don't understand what's wrong with drivers/misc, especially since my
driver registers as a "misc" driver.
I basically think that drivers/misc is wrong for most of the stuff that is
already in there, either because the drivers actually fit into a subsystem
together with other drivers or because they contain rather horrible code.
The idea that drivers using misc_register belong into drivers/misc is a
common misconception. Traditionally they go to drivers/char, which would
still be a better choice, and most "misc" drivers are actually part of a
proper subsystem, while most drivers in drivers/misc don't have a character
device interface.
When we talked about the situation of drivers/misc and drivers/char at
one of the recent conferences, a broad consensus was that they are in
need of a maintainer, which I foolishly signed up for. Deepak wanted
to send an update to the MAINTAINERS file for this (I guess I can do
that too, since he must have forgotten about it), but the main idea is
that I'm there to say no to any driver that someone tries to add there,
unless there are really good reasons why it is actually a good place
to live for that driver.
Arnd
From: Timur Tabi <hidden> Date: 2011-06-06 16:09:47
Arnd Bergmann wrote:
When we talked about the situation of drivers/misc and drivers/char at
one of the recent conferences, a broad consensus was that they are in
need of a maintainer, which I foolishly signed up for. Deepak wanted
to send an update to the MAINTAINERS file for this (I guess I can do
that too, since he must have forgotten about it), but the main idea is
that I'm there to say no to any driver that someone tries to add there,
unless there are really good reasons why it is actually a good place
to live for that driver.
Can you give me an example of a driver that *does* belong in drivers/misc?
Frankly, I just don't see what's wrong with a repository of various drivers that
don't really belong anywhere else.
And what about my concern that my driver will be the only one in drivers/virt?
--
Timur Tabi
Linux kernel developer at Freescale
When we talked about the situation of drivers/misc and drivers/char at
one of the recent conferences, a broad consensus was that they are in
need of a maintainer, which I foolishly signed up for. Deepak wanted
to send an update to the MAINTAINERS file for this (I guess I can do
that too, since he must have forgotten about it), but the main idea is
that I'm there to say no to any driver that someone tries to add there,
unless there are really good reasons why it is actually a good place
to live for that driver.
Can you give me an example of a driver that does belong in drivers/misc?
Not really.
Frankly, I just don't see what's wrong with a repository of various drivers that
don't really belong anywhere else.
The main problem is that for the most part it's a pile of crap that
nobody wants to look at, so drivers getting added there see basically
no real review.
And what about my concern that my driver will be the only one in drivers/virt?
I have no doubt that more of these will come. Chris Metcalf is currently
looking for a home for his tilera hypervisor drivers, and we have the
microsoft hyperv drivers in drivers/staging, so they will hopefully
move to a proper place later. We also have similar drivers in other
places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
or parts of drivers/xen.
Arnd
From: Timur Tabi <hidden> Date: 2011-06-06 16:27:39
Arnd Bergmann wrote:
I have no doubt that more of these will come. Chris Metcalf is currently
looking for a home for his tilera hypervisor drivers, and we have the
microsoft hyperv drivers in drivers/staging, so they will hopefully
move to a proper place later. We also have similar drivers in other
places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
or parts of drivers/xen.
Alright, drivers/virt it is. I'll post a v4 once everyone else has had a chance
to comment on v3.
--
Timur Tabi
Linux kernel developer at Freescale
From: Scott Wood <hidden> Date: 2011-06-06 18:15:28
On Mon, 6 Jun 2011 17:53:09 +0200
Arnd Bergmann [off-list ref] wrote:
On Friday 03 June 2011, Scott Wood wrote:
quoted
On Fri, 3 Jun 2011 17:28:43 +0200
Arnd Bergmann [off-list ref] wrote:
quoted
On Thursday 02 June 2011, Scott Wood wrote:
quoted
I wanted to have the hypervisor take an update dtb (we already have special
meta-properties for things like deletion as part of the hv config
mechanism). But others on the project wanted to keep it simple, and so
get/set property it was. :-/
It's unlikely to change at this point without a real need.
As for a filesystem interface, it's not a good match either.
You can't iterate over anything to read out the full tree from the hv.
kexec iterates over /proc/device-tree to create a dts blob.
That's irrelevant, because we're not talking about that device tree. We're
talking about the device tree of another hypervisor guest.
I understand that it's a different device tree. That doesn't mean we
can't use the same tools.
The kernel does not have the same sort of access to this tree.
quoted
quoted
quoted
You can't delete anything.
rm, rmdir
quoted
You can't create empty nodes.
mkdir
I know how to operate a filesystem. You can't do these operations *on
another guest's device tree through the hv interface*.
Why not?
Because the hypervisor does not support it. It provides only getprop and
setprop. I think you took my "you can't do that" statements to be a
statement about limitations of using a filesystem interfcae -- quite the
opposite, I was saying the hv functionality is too limited to support a
filesystem interface with normal semantics.
quoted
And what would be the benefit of this major restructuring and added
complexity?
I think it would be a slightly better abstraction, and the complexity
is not as big as you make it sound. I'm mainly countering your statement
that it would be a bad interface or that would not possible to do.
I'm not that opposed to having an ioctl interface for your hypervisor
interface, but I am opposed to making design choices based on
a bad representations of facts or not having considered the options
that other people suggest.
I don't really see how a filesystem is a better abstraction for a wrapper
around a procedural interface. A somewhat better argument is that ioctls
are a pain, and Linux doesn't have a better way to expose a procedural
interface, that doesn't require a wrapper program -- though as the wrapper
already exists, and the fs interface would probably be sufficiently awkward
that people would still use a wrapper, that doesn't buy us too much either.
This is not being proposed as any sort of standard kernel API, just a way
for userspace to get access to implementation-specific hcalls.
Implementation-specific backdoor is practically the definition of ioctl. :-)
I would be interested to see a concrete proposal for what this would look
like as a filesystem, though, based on the actual operations that are
available. How would you deal with getting all the parameters in,
performing the operation, and getting the results back? What about when
multiple processes are doing this at the same time? What would the memcpy
hcall look like?
-Scott
On Mon, 6 Jun 2011 17:53:09 +0200
Arnd Bergmann [off-list ref] wrote:
quoted
quoted
quoted
quoted
You can't delete anything.
rm, rmdir
quoted
You can't create empty nodes.
mkdir
I know how to operate a filesystem. You can't do these operations *on
another guest's device tree through the hv interface*.
Why not?
Because the hypervisor does not support it. It provides only getprop and
setprop. I think you took my "you can't do that" statements to be a
statement about limitations of using a filesystem interfcae -- quite the
opposite, I was saying the hv functionality is too limited to support a
filesystem interface with normal semantics.
Ok, sorry for the confusion on my part. It makes a lot more sense now.
quoted
quoted
And what would be the benefit of this major restructuring and added
complexity?
I think it would be a slightly better abstraction, and the complexity
is not as big as you make it sound. I'm mainly countering your statement
that it would be a bad interface or that would not possible to do.
I'm not that opposed to having an ioctl interface for your hypervisor
interface, but I am opposed to making design choices based on
a bad representations of facts or not having considered the options
that other people suggest.
I don't really see how a filesystem is a better abstraction for a wrapper
around a procedural interface. A somewhat better argument is that ioctls
are a pain, and Linux doesn't have a better way to expose a procedural
interface, that doesn't require a wrapper program -- though as the wrapper
already exists, and the fs interface would probably be sufficiently awkward
that people would still use a wrapper, that doesn't buy us too much either.
This is not being proposed as any sort of standard kernel API, just a way
for userspace to get access to implementation-specific hcalls.
Implementation-specific backdoor is practically the definition of ioctl. :-)
I would be interested to see a concrete proposal for what this would look
like as a filesystem, though, based on the actual operations that are
available. How would you deal with getting all the parameters in,
performing the operation, and getting the results back? What about when
multiple processes are doing this at the same time? What would the memcpy
hcall look like?
In fs/libfs.c, we have support for "simple transaction files", where you
write input parameters into a file and then read it back to get the
results. They are very rarely used, but can serve as a way to replace ioctls
with file operations where that makes sense.
For an inter-partition memcpy, a better interface might be a pipe
representation: You have a namespace that is shared between two
partitions, so each side can create directory entries with arbitrary
names in one of the subdirectories of the file system. Then you can
open the file for reading on one side and writing on the other side
and when both sides have started the respective operation, the hypervisor
will copy data. The possible ways to use that functionality are countless.
Similarly, you can mmap a file to get inter-partition shared memory.
Arnd
From: Chris Metcalf <hidden> Date: 2011-06-06 21:01:42
On 6/6/2011 12:24 PM, Arnd Bergmann wrote:
On Monday 06 June 2011, Timur Tabi wrote:.
quoted
And what about my concern that my driver will be the only one in drivers/virt?
I have no doubt that more of these will come. Chris Metcalf is currently
looking for a home for his tilera hypervisor drivers, and we have the
microsoft hyperv drivers in drivers/staging, so they will hopefully
move to a proper place later. We also have similar drivers in other
places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
or parts of drivers/xen.
It might help if someone (Arnd?) wanted to propose a statement of what
drivers/virt was really for. If it's for any Linux driver that upcalls to
a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
well. If it's intended more for drivers that guests running under a
hypervisor can use to talk to the hypervisor itself (e.g. managing
notifications that a hypervisor delivers to a guest to cause it to shut
down or take other actions), then it doesn't seem like the Tilera
paravirtualized device drivers belong there, since they're just using the
Tilera hypervisor synchronously to do I/O or get/set device and driver state.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
From: Konrad Rzeszutek Wilk <hidden> Date: 2011-06-06 21:25:47
On Mon, Jun 06, 2011 at 05:01:36PM -0400, Chris Metcalf wrote:
On 6/6/2011 12:24 PM, Arnd Bergmann wrote:
quoted
On Monday 06 June 2011, Timur Tabi wrote:.
quoted
And what about my concern that my driver will be the only one in drivers/virt?
I have no doubt that more of these will come. Chris Metcalf is currently
looking for a home for his tilera hypervisor drivers, and we have the
microsoft hyperv drivers in drivers/staging, so they will hopefully
move to a proper place later. We also have similar drivers in other
places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
or parts of drivers/xen.
It might help if someone (Arnd?) wanted to propose a statement of what
drivers/virt was really for. If it's for any Linux driver that upcalls to
Was for? I am not seeing it in v3.0-rc2?
a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
well. If it's intended more for drivers that guests running under a
hypervisor can use to talk to the hypervisor itself (e.g. managing
I believe that the code that deals with specific subsystem (so block API
for example) would reside in subsystem directory (so drivers/block would have
your virtualization block driver). This allows the maintainer of block
to make sure your driver is OK.
notifications that a hypervisor delivers to a guest to cause it to shut
down or take other actions), then it doesn't seem like the Tilera
That looks to be arch/<x>/tilera/virt/ candidate?
paravirtualized device drivers belong there, since they're just using the
Tilera hypervisor synchronously to do I/O or get/set device and driver state.
Well, I/O sounds like block API or network API. But then you are also
doing management ioctl - which implies "drivers". "drivers/tilera" does not
work?
From: Chris Metcalf <hidden> Date: 2011-06-06 23:04:47
For context, the most recent patch for the tile driver in question is here:
https://patchwork.kernel.org/patch/843892/
On 6/6/2011 5:23 PM, Konrad Rzeszutek Wilk wrote:
On Mon, Jun 06, 2011 at 05:01:36PM -0400, Chris Metcalf wrote:
quoted
On 6/6/2011 12:24 PM, Arnd Bergmann wrote:
quoted
On Monday 06 June 2011, Timur Tabi wrote:.
quoted
And what about my concern that my driver will be the only one in drivers/virt?
I have no doubt that more of these will come. Chris Metcalf is currently
looking for a home for his tilera hypervisor drivers, and we have the
microsoft hyperv drivers in drivers/staging, so they will hopefully
move to a proper place later. We also have similar drivers in other
places, e.g. drivers/ps3/ps3-sys-manager.c, drivers/s390/char/vmcp.c
or parts of drivers/xen.
It might help if someone (Arnd?) wanted to propose a statement of what
drivers/virt was really for. If it's for any Linux driver that upcalls to
Was for? I am not seeing it in v3.0-rc2?
Sorry, maybe a questionable idiom, but please read past tense in the quoted
text as meaning present tense :-)
quoted
a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
well. If it's intended more for drivers that guests running under a
hypervisor can use to talk to the hypervisor itself (e.g. managing
I believe that the code that deals with specific subsystem (so block API
for example) would reside in subsystem directory (so drivers/block would have
your virtualization block driver). This allows the maintainer of block
to make sure your driver is OK.
Sure, makes sense. The new push (as I understand it) is to group primarily
by function, not by bus or architecture.
quoted
notifications that a hypervisor delivers to a guest to cause it to shut
down or take other actions), then it doesn't seem like the Tilera
That looks to be arch/<x>/tilera/virt/ candidate?
Arnd, among others, has suggested that all drivers live in "drivers"
somewhere, so "arch/tile" may not be the best place. (To be fair, I
originally had this driver in arch/tile/drivers/, so your idea is certainly
reasonable!)
quoted
paravirtualized device drivers belong there, since they're just using the
Tilera hypervisor synchronously to do I/O or get/set device and driver state.
Well, I/O sounds like block API or network API. But then you are also
doing management ioctl - which implies "drivers". "drivers/tilera" does not
work?
There is certainly precedent for drivers that don't fit cleanly into an
existing category to go in drivers/<arch>, e.g. drivers/s390,
drivers/parisc, etc. There is also drivers/platform/x86, though that seems
to be for the bus "platform drivers" rather than just a random character
driver like the one in question.
I don't have a particular opinion here; I'm just hoping to develop enough
consensus that I can ask Linus to pull the driver without generating
controversy :-)
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
On Tuesday 07 June 2011 01:04:40 Chris Metcalf wrote:
For context, the most recent patch for the tile driver in question is here:
https://patchwork.kernel.org/patch/843892/
On 6/6/2011 5:23 PM, Konrad Rzeszutek Wilk wrote:
quoted
On Mon, Jun 06, 2011 at 05:01:36PM -0400, Chris Metcalf wrote:
quoted
quoted
a hypervisor for any reason, then the Tilera paravirtualized drivers fit in
well. If it's intended more for drivers that guests running under a
hypervisor can use to talk to the hypervisor itself (e.g. managing
I believe that the code that deals with specific subsystem (so block API
for example) would reside in subsystem directory (so drivers/block would have
your virtualization block driver). This allows the maintainer of block
to make sure your driver is OK.
Sure, makes sense. The new push (as I understand it) is to group primarily
by function, not by bus or architecture.
Yes.
quoted
quoted
notifications that a hypervisor delivers to a guest to cause it to shut
down or take other actions), then it doesn't seem like the Tilera
That looks to be arch/<x>/tilera/virt/ candidate?
Arnd, among others, has suggested that all drivers live in "drivers"
somewhere, so "arch/tile" may not be the best place. (To be fair, I
originally had this driver in arch/tile/drivers/, so your idea is certainly
reasonable!)
quoted
quoted
paravirtualized device drivers belong there, since they're just using the
Tilera hypervisor synchronously to do I/O or get/set device and driver state.
Well, I/O sounds like block API or network API. But then you are also
doing management ioctl - which implies "drivers". "drivers/tilera" does not
work?
There is certainly precedent for drivers that don't fit cleanly into an
existing category to go in drivers/<arch>, e.g. drivers/s390,
drivers/parisc, etc. There is also drivers/platform/x86, though that seems
to be for the bus "platform drivers" rather than just a random character
driver like the one in question.
I don't have a particular opinion here; I'm just hoping to develop enough
consensus that I can ask Linus to pull the driver without generating
controversy :-)
The drivers/s390 and drivers/parisc directories are from a distant past,
we should not add new ones like them. drivers/platform is controversial,
but I think it's ok for stuff that manages platform specific quirks.
The main problem with that is that it doesn't work for embedded systems,
by extension every ARM specific driver could go into drivers/platform/...
and we don't want that.
You can probably argue that the tile drivers do fit in here as long as
they are specific to the hypervisor and not to some SOC specific hardware.
Arnd
From: Chris Metcalf <hidden> Date: 2011-06-07 16:49:12
On 6/7/2011 3:08 AM, Arnd Bergmann wrote:
On Tuesday 07 June 2011 01:04:40 Chris Metcalf wrote:
quoted
There is certainly precedent for drivers that don't fit cleanly into an
existing category to go in drivers/<arch>, e.g. drivers/s390,
drivers/parisc, etc. There is also drivers/platform/x86, though that seems
to be for the bus "platform drivers" rather than just a random character
driver like the one in question.
The drivers/s390 and drivers/parisc directories are from a distant past,
we should not add new ones like them. drivers/platform is controversial,
but I think it's ok for stuff that manages platform specific quirks.
The main problem with that is that it doesn't work for embedded systems,
by extension every ARM specific driver could go into drivers/platform/...
and we don't want that.
You can probably argue that the tile drivers do fit in here as long as
they are specific to the hypervisor and not to some SOC specific hardware.
Can you clarify that? I think you're contrasting something like an ARM
core that was licensed and put in a SoC by some random vendor, and you
could have an endless stream of drivers for that case. The Tilera core
isn't being licensed; it's sold more like an Intel chip with a fixed set of
interfaces available only from Tilera. The particular interface in
question here is SPI, and the core itself knows how to boot the chip over
SPI by finding an SPI ROM and reading the boot stream out of it directly
after power-up.
So does that match with your model of "drivers/platform/tile"? Maybe we
have a winner! :-)
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
On Tuesday 07 June 2011 18:49:02 Chris Metcalf wrote:
quoted
You can probably argue that the tile drivers do fit in here as long as
they are specific to the hypervisor and not to some SOC specific hardware.
Can you clarify that? I think you're contrasting something like an ARM
core that was licensed and put in a SoC by some random vendor, and you
could have an endless stream of drivers for that case. The Tilera core
isn't being licensed; it's sold more like an Intel chip with a fixed set of
interfaces available only from Tilera. The particular interface in
question here is SPI, and the core itself knows how to boot the chip over
SPI by finding an SPI ROM and reading the boot stream out of it directly
after power-up.
So does that match with your model of "drivers/platform/tile"? Maybe we
have a winner! :-)
I'm not really against drivers/platform/tile for this, the only potential
problem that I see with this is that having more stuff in drivers/platform
might lead to having even more other stuff in there that should really
go into another place.
Obviously, if the device is a raw SPI host, the driver should actually go
into drivers/spi/spi_tile.c rather than drivers/platform/tile/spi.c.
For the spi flash driver that goes through the hypervisor abstraction,
I think drivers/virt/tile would be better than driver/platform/tile,
but we should really have a new "abstract flash character driver" subsystem
for that.
Arnd
From: Timur Tabi <hidden> Date: 2011-06-07 19:20:57
Arnd Bergmann wrote:
For the spi flash driver that goes through the hypervisor abstraction,
I think drivers/virt/tile would be better than driver/platform/tile,
but we should really have a new "abstract flash character driver" subsystem
for that.
Why should it matter that the SPI flash driver goes through the hypervisor
abstration? One of the patches in this patchset is a TTY driver that goes
through the Freescale hypervisor. I put the drivers in drivers/tty.
--
Timur Tabi
Linux kernel developer at Freescale
On Tuesday 07 June 2011 21:20:50 Timur Tabi wrote:
Arnd Bergmann wrote:
quoted
For the spi flash driver that goes through the hypervisor abstraction,
I think drivers/virt/tile would be better than driver/platform/tile,
but we should really have a new "abstract flash character driver" subsystem
for that.
Why should it matter that the SPI flash driver goes through the hypervisor
abstration? One of the patches in this patchset is a TTY driver that goes
through the Freescale hypervisor. I put the drivers in drivers/tty.
The driver in question is for a hypervisor that abstracts the flash memory
using a read/write interface. There is no way you can represent that as
a SPI host driver.
Arnd