Re: [Xen-devel] [PATCH RESEND v5 1/6] xen/arm: Implement hvm save and restore

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-11-13 at 12:00 +0400, Eugene Fedotov wrote:

> > [...]
> >> +static int vgic_irq_rank_save(struct vgic_rank *ext,
> >> +                               struct vgic_irq_rank *rank)
> >> +{
> >> +    spin_lock(&rank->lock);
> > This is probably wise, but the domain must be paused at this point,
> > right?
> I think this lock can be removed, thanks.

I'd be happy for it to stay as a belt-and-braces/good practice thing.

> >> +        return -EINVAL;
> >> +    }
> >> +    memcpy(ext->ipriority, rank->ipriority, sizeof(rank->ipriority));
> >> +    /* ITARGETS */
> >> +    if ( sizeof(rank->itargets) != sizeof (ext->itargets) )
> >> +    {
> >> +        dprintk(XENLOG_G_ERR, "hvm_hw_gic: check itargets dumping 
> >> space\n");
> >> +        return -EINVAL;
> >> +    }
> >> +    memcpy(ext->itargets, rank->itargets, sizeof(rank->itargets));
> >> +    spin_unlock(&rank->lock);
> >> +    return 0;
> >> +}
> >> +
> >> +static int gic_save(struct domain *d, hvm_domain_context_t *h)
> >> +{
> >> +    struct hvm_hw_gic ctxt;
> >> +    struct vcpu *v;
> >> +
> >> +    /* Save the state of GICs */
> >> +    for_each_vcpu( d, v )
> > Where is the GICD state saved then?
> The only GICD structure we save for guest domain is struct 
> vgic_irq_rank, it includes: IENABLE, IACTIVE, IPEND,  PENDSGI, ICFG, 
> IPRIORITY, ITARGETS registers. We create the same structure inside hvm : 
> vgic_rank (that is no guaranteed to be the same as struct vgic_irq_rank) 
> and save it calling vgic_irq_rank_save routine below in gic_save.

I can only see one call to vgic_irq_rank_save which is the one to save
PPI state within the vcpu loop. What about the (per-cpu) SGI and
(global) SPIs? I can't see where either of those are saved.

> >> +        ctxt.x0 = c.x0;
> > Would it make sense to include a field of type vcpu_guest_core_regs in
> > the save struct instead of this big list?
> Yes, I think vcpu_guest_core_regs is already common definition for ARM32 
> and ARM64.

Correct.

> >> diff --git a/xen/include/asm-arm/hvm/support.h 
> >> b/xen/include/asm-arm/hvm/support.h
> >> new file mode 100644
> >> index 0000000..8311f2f
> >> --- /dev/null
> >> +++ b/xen/include/asm-arm/hvm/support.h
> >> @@ -0,0 +1,29 @@
> >> +/*
> >> + * support.h: HVM support routines used by ARMv7 VE.
> > Not just on Vexpress nor just on v7 I expect?
> VE means Virtualization Extentions here.
> OK, we should support ARM v8 in future,so can be "HVM support routines 
> used by ARMv7/v8 with Virtualization Extensions" a good comment?

I think just "...used by ARM" would be sufficient.

> >> + *
> >> + * Copyright (c) 2012, Citrix Systems
> > Really?
> OK
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> >> for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License 
> >> along with
> >> + * this program; if not, write to the Free Software Foundation, Inc., 59 
> >> Temple
> >> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> >> + */
> >> +
> >> +#ifndef __ASM_ARM_HVM_SUPPORT_H__
> >> +#define __ASM_ARM_HVM_SUPPORT_H__
> >> +
> >> +#include <xen/types.h>
> >> +#include <public/hvm/ioreq.h>
> >> +#include <xen/sched.h>
> >> +#include <xen/hvm/save.h>
> >> +#include <asm/processor.h>
> > Just some #includes here?
> Yes, I remove unnecessary includes.

Oh, I see now this header required by common code, that's OK then.

But please do minimise what is needed here.

Ian.


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