Re: [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 12:22
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 5/7] x86/viridian: add warnings for unimplemented
> hypercalls and MSRs
> 
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > The warnings are rate limited so a malicious guest cannot use them to
> > as a DoS.
> 
> In fact they're debug-build-only ones. Did you perhaps mean gprintk()?
> 

Yes, I did.

> > @@ -534,6 +592,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> >          break;
> >
> >      default:
> > +        if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
> 
> Coding style (also further down).
> 
> > +            gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> > +                     idx);
> 
> Can you please make distinguishing of write from ...
> 
> > @@ -657,6 +719,10 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >      }
> >
> >      default:
> > +        if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
> > +            gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> > +                     idx);
> 
> ... read possible without having to make use of the logged line
> number?
> 

I will.

> > @@ -809,7 +875,15 @@ int viridian_hypercall(struct cpu_user_regs *regs)
> >          break;
> >      }
> >
> > +    case HvGetPartitionId:
> > +    case HvExtCallQueryCapabilities:
> > +        status = HV_STATUS_INVALID_HYPERCALL_CODE;
> > +        break;
> 
> I think a brief comment on why these don't get a message logged
> would be helpful to future readers.

Sure.

> I also think that it would be
> more natural to place the two case labels ...
> 
> >      default:
> > +        gdprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
> > +                 input.call_code);
> > +
> 
> ... here.
> 

Ok. My general preference is for default to be last but given that doing it this way round allows for a fall-through it is indeed neater.

  Paul

> >          status = HV_STATUS_INVALID_HYPERCALL_CODE;
> >          break;
> >      }
> 
> Jan


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