Re: [Xen-devel] [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.04.18 at 18:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> do_get_debugreg() has several bugs:
> 
>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>    %cr4.de is disabled.
>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>    with zero.
>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>    near the top of the address space.
> 
> Introduce a common x86emul_read_dr() handler (as we will eventually want to
> add HVM support) which separates its success/failure indication from the data
> value, and have do_get_debugreg() call into the handler.

The HVM part here is sort of questionable because of your use of
curr->arch.pv_vcpu.ctrlreg[4]. This is appropriate for the NULL ctxt case,
but it's already a layering violation for the use of the function in
priv_op_ops, where the read_cr() hook should be used instead.

> The ABI of do_get_debugreg() remains broken, but switches from -EINVAL to
> -ENODEV for compatibility with the changes in the following patch.
> 
> Take the opportunity to add a missing local variable block to x86_emulate.c

I don't think such a block can ever be "missing" - it shouldn't really be a 
requirement
for one to be there; note how ./CODING_STYLE says "is permitted". Of course I
don't mind its addition here.

> +int x86emul_read_dr(unsigned int reg, unsigned long *val,
> +                    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct vcpu *curr = current;
> +
> +    /* HVM support requires a bit more plumbing before it will work. */
> +    ASSERT(is_pv_vcpu(curr));
> +
> +    switch ( reg )
> +    {
> +    case 0 ... 3:
> +    case 6:
> +        *val = curr->arch.debugreg[reg];
> +        break;
> +
> +    case 7:
> +        *val = (curr->arch.debugreg[7] |
> +                curr->arch.debugreg[5]);
> +        break;
> +
> +    case 4 ... 5:
> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
> +        {
> +            *val = curr->arch.debugreg[reg + 2];
> +            break;

Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 
(really
DR5) value here?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel