Re: [Xen-devel] [PATCH v4 1/9] xen/arm: Implement hvm save and restore

[Jaeyong Yoo <jaeyong.yoo@xxxxxxxxxxx>]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Tuesday, October 08, 2013 6:06 PM
> To: Ian Campbell
> Cc: jaeyong.yoo@xxxxxxxxxxx; Julien Grall; xen-devel@xxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel