[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] Fix save/restore for HVM domains with viridian=1


  • To: "Tim (Xen.org)" <tim@xxxxxxx>
  • From: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
  • Date: Fri, 25 Nov 2011 15:14:18 +0000
  • Accept-language: en-US
  • Acceptlanguage: en-US
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 25 Nov 2011 15:15:03 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xensource.com>
  • Thread-index: AcyrhOJsnABcN6OrSS2PejfwK/uSKg==
  • Thread-topic: [Xen-devel] [PATCH] Fix save/restore for HVM domains with viridian=1

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: 25 November 2011 15:01
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH] Fix save/restore for HVM domains
> with viridian=1
> 
> At 14:50 +0000 on 25 Nov (1322232658), Paul Durrant wrote:
[snip]
> >
> > +    case XC_SAVE_ID_HVM_VIRIDIAN:
> > +        /* Skip padding 4 bytes then read the acpi ioport
> location.
> > + */
> 
> Ahem.
> ^^^^^^^^^^^^^^^^^^^^
> 

Yes, whoops.

[snip]
> > @@ -206,8 +206,11 @@ int wrmsr_viridian_regs(uint32_t idx, ui
> >      struct vcpu *v = current;
> >      struct domain *d = v->domain;
> >
> > -    if ( !is_viridian_domain(d) )
> > +    if ( !is_viridian_domain(d) ) {
> > +        gdprintk(XENLOG_WARNING, "%s: %d not a viridian
> domain\n", __func__,
> > +                 d->domain_id);
> 
> This should probably be rate-limited; a confused domain could cause
> a _lot_ of these.  Might we want to pause/kill the domain as well,
> or inject a fault?
> 

Yes, true.

> >          return 0;
> > +    }
> >
> >      switch ( idx )
> >      {
> > @@ -271,8 +274,11 @@ int rdmsr_viridian_regs(uint32_t idx, ui
> >      struct vcpu *v = current;
> >      struct domain *d = v->domain;
> >
> > -    if ( !is_viridian_domain(d) )
> > +    if ( !is_viridian_domain(d) ) {
> > +        gdprintk(XENLOG_WARNING, "%s: %d not a viridian
> domain\n", __func__,
> > +                 d->domain_id);
> 
> Likewise.
> 
> >          return 0;
> > +    }
> >
> >      switch ( idx )
> >      {
> > @@ -411,6 +417,8 @@ static int viridian_load_domain_ctxt(str
> >      if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> >          return -EINVAL;
> >
> > +    ASSERT(is_viridian_domain(d));
> > +
> 
> I don't think it's appropriate to crash Xen if the save file is
> bogus.
> 

There's a similar ASSERT in the hypercall function anyway; would you rather I 
turned that into a rate limited warning too?

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.