From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaeyong Yoo Subject: Re: [PATCH v4 1/9] xen/arm: Implement hvm save and restore Date: Wed, 09 Oct 2013 21:34:39 +0900 Message-ID: <005201cec4eb$ed922660$c8b67320$%yoo@samsung.com> References: <2631824.259101381213851451.JavaMail.weblogic@epml25> <1381222420.9920.9.camel@kazak.uk.xensource.com> <20131008090617.GA40780@ocelot.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <20131008090617.GA40780@ocelot.phlegethon.org> Content-language: ko List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: 'Tim Deegan' , 'Ian Campbell' Cc: 'Julien Grall' , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Tim Deegan [mailto:tim@xen.org] > Sent: Tuesday, October 08, 2013 6:06 PM > To: Ian Campbell > Cc: jaeyong.yoo@samsung.com; Julien Grall; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH v4 1/9] xen/arm: Implement hvm save and > restore > > At 09:53 +0100 on 08 Oct (1381226020), Ian Campbell wrote: > > On Tue, 2013-10-08 at 06:30 +0000, Jaeyong Yoo wrote: > > > > > >> +static void vgic_irq_rank_save(struct vgic_rank *ext, > > > >> + struct vgic_irq_rank *rank) { > > > >> + spin_lock(&rank->lock); > > > >> + /* Some of VGIC registers are not used yet, it is for a future > usage */ > > > >> + /* IENABLE, IACTIVE, IPEND, PENDSGI registers */ > > > >> + ext->ienable = rank->ienable; > > > >> + ext->iactive = rank->iactive; > > > >> + ext->ipend = rank->ipend; > > > >> + ext->pendsgi = rank->pendsgi; > > > >> + /* ICFG */ > > > >> + ext->icfg[0] = rank->icfg[0]; > > > >> + ext->icfg[1] = rank->icfg[1]; > > > >Can you use memcpy? > > > > Why? > > > > > OK. > > > > If ext->icfg[0] rank->icfg[0] are the same type then I think using > > assignment is better, since it is more type safe and it will > > effectively get turned into a memcpy by the compiler anyway. > > +1. memcpy is dangerous here. > > On x86 we avoided (much of) this kind of thing by just using the API- > defined structs directly in the internal ones. Then there's no need for > any copying at all in this kind of function, just save the struct out. > > Of course, you have to be careful to separate architectural state (which > goes in the save record) from Xen implementation detail (which must not). Sounds good to me. I will prepare next version with this idea. Thanks, Jaeyong > > Tim. > > > If they are different types then using memcpy just runs the risk that > > one of them will silently change and break things. > > > > With that in mind can these not be proper assignments too: > > > > > >> + /* IPRIORITY */ > > > >> + memcpy(ext->ipriority, rank->ipriority, sizeof(rank- > >ipriority)); > > > >> + /* ITARGETS */ > > > >> + memcpy(ext->itargets, rank->itargets, > > > >> + sizeof(rank->itargets)); > > > > Or if not then copy field by field would actually be preferable IMHO. > > A helper macro can make it a bit tidier sometimes: > > #define C(F) ext->#F = rank->#F > > > > then > > C(itargets.foo); > > C(itergets.bar); > > #undef C > > etc. > > > > Ian. > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel