Re: [Xen-devel] [PATCH 5/6] x86/hvm: Handle x2apic MSRs the new guest_{rd, wr}msr() infrastructure

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 26, 2018 at 05:35:18PM +0000, Andrew Cooper wrote:
> Dispatch from the guest_{rd,wr}msr() functions.  The read side should be safe
> outside of current context, but the write side is definitely not.  As the
> toolstack has plausible reason to access the APIC registers via this interface
> (not least because whether they are accessible at all depends on guest
> settings), unilaterally reject access attempts outside of current context.
> 
> Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
> to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE.  The previous
> callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
> interfere with the fallback to legacy MSR handling.
> 
> While altering guest_rdmsr_x2apic() make a couple of minor improvements.
> Reformat the initialiser for readable[] so it indents in a more natural way,
> and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
> path.
> 
> Observant people might notice that we now don't let PV guests read the x2apic
> MSRs.  They should never have been able to in the first place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> @@ -1054,13 +1056,18 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int 
> msr, uint64_t msr_content)
>      case APIC_EOI:
>      case APIC_ESR:
>          if ( msr_content )
> +        {
>      default:
> -            return X86EMUL_UNHANDLEABLE;

Why not add a gp_fault label here and just return X86EMUL_EXCEPTION?
That seems better than adding a gp_fault label below.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,6 +139,7 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> +    const struct vcpu *curr = current;
>      const struct domain *d = v->domain;
>      const struct cpuid_policy *cp = d->arch.cpuid;
>      const struct msr_domain_policy *dp = d->arch.msr;
> @@ -175,6 +176,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
> +        if ( !is_hvm_domain(d) || v != curr )
> +            goto gp_fault;
> +
> +        ret = guest_rdmsr_x2apic(v, msr, val);
> +        goto out;
> +
>      case 0x40000000 ... 0x400001ff:
>          if ( is_viridian_domain(d) )
>          {
> @@ -270,6 +278,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>          break;
>      }
>  
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
> +        if ( !is_hvm_domain(d) || v != curr )
> +            goto gp_fault;
> +
> +        ret = guest_wrmsr_x2apic(v, msr, val);
> +        goto out;

AFAICT you could just use a break here?

Thanks, Roger.

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