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

[Jan Beulich]

>>> On 16.04.18 at 18:44,  wrote:
> On 16/04/18 13:32, Jan Beulich wrote:
> On 16.04.18 at 14:18,  wrote:
>>> On 13/04/18 12:39, Jan Beulich wrote:
>>> On 13.04.18 at 13:17,  wrote:
> On 13/04/18 09:31, Jan Beulich wrote:
> On 12.04.18 at 18:55,  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].
> That is what the "needs further plumbing" refers to, as well as needing
> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>
> However, we are gaining an increasing amount of common x86 code which
> needs to read control register values, and I've got a plan to refactor
> across the board to v->arch.cr4 (and similar).  There is no point having
> identical information in different parts of sub-unions.
 I agree.

>> 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.
> Hmm - doing this, while probably the better long temr course of action,
> would require passing the ops structures down into the callbacks.
 That doesn't sound like a problem, though - the hypercall path would
 pass NULL there as well.
>> This ...
>>
>>> +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?
> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
 Oh, right you are.
>>> So, are your comments suitably addressed?  It is unclear whether you
>>> want any changes to be made.
>> ... is what I'd prefer to be taken care of without delaying to the time when
>> we make this work for HVM as well. Unless you feel really strongly about it
>> being better the way you have it, in which case you may feel free to add
>> my ack.
> 
> In all PV cases (hypercall and emulation), the current code functions
> correctly, because DE is active in context.
> 
> In principle, the emulation case would be better if it used the
> read_cr() hook, but that is invasive to arrange (which is why I chose
> not to at this point), and still needs special casing for the hypercall
> case anyway.

Well, okay then:
Acked-by: Jan Beulich 

Jan



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

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

[Andrew Cooper]

On 16/04/18 13:32, Jan Beulich wrote:
 On 16.04.18 at 14:18,  wrote:
>> On 13/04/18 12:39, Jan Beulich wrote:
>> On 13.04.18 at 13:17,  wrote:
 On 13/04/18 09:31, Jan Beulich wrote:
 On 12.04.18 at 18:55,  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].
 That is what the "needs further plumbing" refers to, as well as needing
 hooks to get/modify %dr6/7 from the VMCB/VMCS.

 However, we are gaining an increasing amount of common x86 code which
 needs to read control register values, and I've got a plan to refactor
 across the board to v->arch.cr4 (and similar).  There is no point having
 identical information in different parts of sub-unions.
>>> I agree.
>>>
> 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.
 Hmm - doing this, while probably the better long temr course of action,
 would require passing the ops structures down into the callbacks.
>>> That doesn't sound like a problem, though - the hypercall path would
>>> pass NULL there as well.
> This ...
>
>> +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?
 [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
 patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
>>> Oh, right you are.
>> So, are your comments suitably addressed?  It is unclear whether you
>> want any changes to be made.
> ... is what I'd prefer to be taken care of without delaying to the time when
> we make this work for HVM as well. Unless you feel really strongly about it
> being better the way you have it, in which case you may feel free to add
> my ack.

In all PV cases (hypercall and emulation), the current code functions
correctly, because DE is active in context.

In principle, the emulation case would be better if it used the
read_cr() hook, but that is invasive to arrange (which is why I chose
not to at this point), and still needs special casing for the hypercall
case anyway.

~Andrew

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

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

[Jan Beulich]

>>> On 16.04.18 at 14:18,  wrote:
> On 13/04/18 12:39, Jan Beulich wrote:
> On 13.04.18 at 13:17,  wrote:
>>> On 13/04/18 09:31, Jan Beulich wrote:
>>> On 12.04.18 at 18:55,  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].
>>> That is what the "needs further plumbing" refers to, as well as needing
>>> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>>>
>>> However, we are gaining an increasing amount of common x86 code which
>>> needs to read control register values, and I've got a plan to refactor
>>> across the board to v->arch.cr4 (and similar).  There is no point having
>>> identical information in different parts of sub-unions.
>> I agree.
>>
 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.
>>> Hmm - doing this, while probably the better long temr course of action,
>>> would require passing the ops structures down into the callbacks.
>> That doesn't sound like a problem, though - the hypercall path would
>> pass NULL there as well.

This ...

> +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?
>>> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
>>> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
>> Oh, right you are.
> 
> So, are your comments suitably addressed?  It is unclear whether you
> want any changes to be made.

... is what I'd prefer to be taken care of without delaying to the time when
we make this work for HVM as well. Unless you feel really strongly about it
being better the way you have it, in which case you may feel free to add
my ack.

Jan



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

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

[Andrew Cooper]

On 13/04/18 12:39, Jan Beulich wrote:
 On 13.04.18 at 13:17,  wrote:
>> On 13/04/18 09:31, Jan Beulich wrote:
>> On 12.04.18 at 18:55,  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].
>> That is what the "needs further plumbing" refers to, as well as needing
>> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>>
>> However, we are gaining an increasing amount of common x86 code which
>> needs to read control register values, and I've got a plan to refactor
>> across the board to v->arch.cr4 (and similar).  There is no point having
>> identical information in different parts of sub-unions.
> I agree.
>
>>> 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.
>> Hmm - doing this, while probably the better long temr course of action,
>> would require passing the ops structures down into the callbacks.
> That doesn't sound like a problem, though - the hypercall path would
> pass NULL there as well.
>
 +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?
>> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
>> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
> Oh, right you are.

So, are your comments suitably addressed?  It is unclear whether you
want any changes to be made.

~Andrew

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

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

[Jan Beulich]

>>> On 13.04.18 at 13:17,  wrote:
> On 13/04/18 09:31, Jan Beulich wrote:
> On 12.04.18 at 18:55,  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].
> 
> That is what the "needs further plumbing" refers to, as well as needing
> hooks to get/modify %dr6/7 from the VMCB/VMCS.
> 
> However, we are gaining an increasing amount of common x86 code which
> needs to read control register values, and I've got a plan to refactor
> across the board to v->arch.cr4 (and similar).  There is no point having
> identical information in different parts of sub-unions.

I agree.

>> 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.
> 
> Hmm - doing this, while probably the better long temr course of action,
> would require passing the ops structures down into the callbacks.

That doesn't sound like a problem, though - the hypercall path would
pass NULL there as well.

>>> +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?
> 
> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.

Oh, right you are.

Jan



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

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

[Andrew Cooper]

On 13/04/18 09:31, Jan Beulich wrote:
 On 12.04.18 at 18:55,  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].

That is what the "needs further plumbing" refers to, as well as needing
hooks to get/modify %dr6/7 from the VMCB/VMCS.

However, we are gaining an increasing amount of common x86 code which
needs to read control register values, and I've got a plan to refactor
across the board to v->arch.cr4 (and similar).  There is no point having
identical information in different parts of sub-unions.

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

Hmm - doing this, while probably the better long temr course of action,
would require passing the ops structures down into the callbacks.

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

[5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
patch), as IO breakpoints are only valid to use when %cr4.de is enabled.

I haven't yet investigated the native behaviour of selecting an IO
breakpoint, then disabling Debug Extensions.

That said, one of my TODO items is to allow PV guests to use General
Detect (because its trivial to implement), at which point [5] will turn
from an IO breakpoint shadow, into a more general %dr7 shadow, and ORing
it in here will matter.

~Andrew

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

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

[Jan Beulich]

>>> On 12.04.18 at 18:55,  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel