From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: Re: [PATCH] xen: reduce severity of message about using v1 grant tables. Date: Thu, 03 Dec 2009 12:13:58 -0800 Message-ID: <4B181C06.7040107@goop.org> References: <1259782098-32180-1-git-send-email-Ian.Campbell@citrix.com> <4B16C103.9070105@goop.org> <1259783682.31045.22.camel@localhost.localdomain> <1259836124.23698.10393.camel@zakaz.uk.xensource.com> <20091203120116.GA15460@weybridge.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20091203120116.GA15460@weybridge.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Steven Smith Cc: Ian Campbell , Steven Smith , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 12/03/09 04:01, Steven Smith wrote: >> Oh, I see what you meant... in the proper resume case (as opposed to the >> cancelled suspend/checkpoint case I was thinking of) there should be no >> grant tables in use at this point so most devices should, in theory, be >> able to reconnect using v1 grants, any drivers which require v2 grant >> tables need to check for them in their resume hook as well as at start >> of day. >> >> Unfortunately frontend devices tear down their grant entries after the >> resume rather than before the suspend (I presume this has to do with >> faster checkpointing?) which means they could be trying to clear an >> entry of the wrong layout, leading the unbounded badness that the >> comment refers to. >> > Actually, I think that'd be okay. Drivers should be clearing grant > table entries through the gnttab_end_* functions, which always check > grant_table_version and do the right thing. > gnttab_grant_foreign_access_ref_{subpage,trans} would BUG(), but that > could be turned into an error return easily enough. Handling it > everywhere would be a pain, though. > > The only other potential subtlety is handling a suspend while some > other vcpu is doing grant table operations, but I think the > stop_machine() is sufficient protection against that. > Yes. We already have to be careful to prevent pte updates from being preempted by suspend, so grant operations are similar. (When CONFIG_PREEMPT is enabled, it is freeze/thaw which provides this guarantee.) > BUG_ON(flags & (GTF_accept_transfer | GTF_reading | > GTF_writing | GTF_sub_page | GTF_permit_access)); > - BUG_ON(grant_table_version == 1); > + if (grant_table_version == 1) > Rather than having all these version tests, would it make sense to have a grant_ops vector? J