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

[Tim Deegan <tim@xen.org>]

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).

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