Re: [Xen-devel] [PATCH 08/38] arm: allocate and setup a guest vcpu.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2012-06-06 at 14:46 +0100, Stefano Stabellini wrote:
> On Fri, 1 Jun 2012, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/domain.c         |   68 
> > +++++++++++++++++++++++++++++++++++++++++
> >  xen/arch/arm/dummy.S          |    3 --
> >  xen/include/public/arch-arm.h |    9 -----
> >  3 files changed, 68 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 9339a11..62a2f3a 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -144,6 +144,17 @@ void free_vcpu_struct(struct vcpu *v)
> >      free_xenheap_page(v);
> >  }
> >  
> > +struct vcpu_guest_context *alloc_vcpu_guest_context(void)
> > +{
> > +    return xmalloc(struct vcpu_guest_context);
> > +
> > +}
> > +
> > +void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
> > +{
> > +    xfree(vgc);
> > +}
> > +
> >  int vcpu_initialise(struct vcpu *v)
> >  {
> >      int rc = 0;
> > @@ -182,6 +193,9 @@ int arch_domain_create(struct domain *d, unsigned int 
> > domcr_flags)
> >      if ( (rc = p2m_init(d)) != 0 )
> >          goto fail;
> >  
> > +    if ( (rc = domain_vgic_init(d)) != 0 )
> > +        goto fail;
> > +
> 
> there is a call to domain_vgic_init already in arch_domain_create

So there is!

I notice while checking that a bunch of stuff can/should be pushed under
the !idle_domain conditional, or better the idle domain case should bail
early.

> > +int arch_set_info_guest(
> > +    struct vcpu *v, vcpu_guest_context_u c)
> > +{
> > +    struct cpu_user_regs *regs = &c.nat->user_regs;
> > +
> > +    if ( !is_guest_psr(regs->cpsr) )
> > +        return -EINVAL;
> > +
> > +    if ( regs->spsr_svc && !is_guest_psr(regs->spsr_svc) )
> > +        return -EINVAL;
> > +    if ( regs->spsr_abt && !is_guest_psr(regs->spsr_abt) )
> > +        return -EINVAL;
> > +    if ( regs->spsr_und && !is_guest_psr(regs->spsr_und) )
> > +        return -EINVAL;
> > +    if ( regs->spsr_irq && !is_guest_psr(regs->spsr_irq) )
> > +        return -EINVAL;
> > +    if ( regs->spsr_fiq && !is_guest_psr(regs->spsr_fiq) )
> > +        return -EINVAL;
> > +
> > +    v->arch.cpu_info->guest_cpu_user_regs = *regs;
> > +
> > +    /* XXX other state:
> > +     * - SCTLR
> > +     * - TTBR0/1
> > +     * - TTBCR
> > +     */
> > +
> > +    //if ( flags & VGCF_online )
> > +        clear_bit(_VPF_down, &v->pause_flags);
> > +    //else
> > +    //    set_bit(_VPF_down, &v->pause_flags);
> > +
> > +    return 0;
> > +}
> 
> Do we really need to add commented out code like this?

Yeah, you're right, I copied from x86 which has this but we haven't
implemented it for ARM yet. I suppose an XXX would be better. Or maybe I
should just implement the flags...

> Also arch_set_info_guest could benefit by a couple of lines of comments
> to explain what it is supposed to do.

I'll add something.



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