Re: [Xen-devel] [PATCH] x86: Control CR0 TS behavior using dev_na_ts_allowed

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.14 at 04:30, Sarah Newman <srn@xxxxxxxxx> wrote:

Not being convinced at all that this is the right approach (in
particular it remains unclear how an affected guest should deal with
running on a hypervisor not supporting the new interface), a couple
of mechanical comments nevertheless:

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1256,6 +1256,12 @@ long arch_do_domctl(
>      }
>      break;
>  
> +    case XEN_DOMCTL_dev_na_ts_allowed:
> +    {
> +        d->arch.pv_domain.dev_na_ts_allowed = 
> domctl->u.dev_na_ts_allowed.allowed;
> +    }
> +    break;

Pointless braces. Also you're converting a 32-bit flags value to
bool_t here, which you ought to do more carefully: Either any non-
zero value of the input means "boolean true", or bit 0 of it is the
intended designator. The decision here may also make necessary
an adjustment to the interface definition further down.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -713,8 +713,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t 
> sub_idx,
>              *ebx = 0x40000200;
>          *ecx = 0;          /* Features 1 */
>          *edx = 0;          /* Features 2 */
> -        if ( is_pv_vcpu(current) )
> +        if ( is_pv_vcpu(current) ) {
>              *ecx |= XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD;
> +
> +            if (current->domain->arch.pv_domain.dev_na_ts_allowed)
> +                *ecx |= XEN_CPUID_FEAT1_DEV_NA_TS_ALLOWED;
> +        }

Coding style.

> @@ -3269,8 +3273,13 @@ void do_device_not_available(struct cpu_user_regs 
> *regs)
>  
>      if ( curr->arch.pv_vcpu.ctrlreg[0] & X86_CR0_TS )
>      {
> -        do_guest_trap(TRAP_no_device, regs, 0);
> -        curr->arch.pv_vcpu.ctrlreg[0] &= ~X86_CR0_TS;
> +        if (!current->domain->arch.pv_domain.dev_na_ts_allowed) {
> +            do_guest_trap(TRAP_no_device, regs, 0);
> +            curr->arch.pv_vcpu.ctrlreg[0] &= ~X86_CR0_TS;
> +        } else {
> +            stts();
> +            do_guest_trap(TRAP_no_device, regs, 0);
> +        }

No need to call do_guest_trap() separately in both branches -
leave the call where it was, and only make the other operation
conditional.

Also coding style again.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -232,6 +232,8 @@ struct pv_domain
>       * unmask the event channel */
>      bool_t auto_unmask;
>  
> +    bool_t dev_na_ts_allowed;

The naming of this (here and elsewhere) is rather odd: I assume
you mean "dev_na" to be an abbreviation of "device not available",
but I'd much prefer the CPU exception abbreviation (#NM) to be
used in such a case. However, in the case here this still wouldn't
be a precise description of the behavior you establish: TS being
set isn't allowed, but required to be set upon guest #NM. I'd
therefore suggest naming this along the lines of "nm_enforce_ts".

Jan


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