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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.17 at 10:57, <paul.durrant@xxxxxxxxxx> 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()?

> @@ -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?

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

>          status = HV_STATUS_INVALID_HYPERCALL_CODE;
>          break;
>      }

Jan


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