Re: [Xen-devel] [PATCH] x86: synchronize PCI config space access decoding

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/03/15 16:08, Jan Beulich wrote:
> Both PV and HVM logic have similar but not similar enough code here.
> Synchronize the two so that
> - in the HVM case we don't unconditionally try to access extended
>   config space
> - in the PV case we pass a correct range to the XSM hook
> - in the PV case we don't needlessly deny access when the operation
>   isn't really on PCI config space
> All this along with sharing the macros HVM already had here.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2383,11 +2383,6 @@ void hvm_vcpu_down(struct vcpu *v)
>  static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
>                                                          ioreq_t *p)
>  {
> -#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
> -#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
> -#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
> -#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
> -
>      struct hvm_ioreq_server *s;
>      uint32_t cf8;
>      uint8_t type;
> @@ -2416,9 +2411,19 @@ static struct hvm_ioreq_server *hvm_sele
>  
>          type = IOREQ_TYPE_PCI_CONFIG;
>          addr = ((uint64_t)sbdf << 32) |
> -               CF8_ADDR_HI(cf8) |
>                 CF8_ADDR_LO(cf8) |
>                 (p->addr & 3);
> +        /* AMD extended configuration space access? */
> +        if ( CF8_ADDR_HI(cf8) &&
> +             boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> +             boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
> +        {
> +            uint64_t msr_val;
> +
> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> +                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> +                addr |= CF8_ADDR_HI(cf8);

This is another example of host state which leaks into guests across
migrate, but in this case is also problematic at the host level.

As far as the host goes, MSR_AMD64_NB_CFG is a per-node msr and Xen
should verify that the AMD64_NB_CFG_CF8_EXT_ENABLE_BIT is consistent
across the system, or bits of emulate_privileged_op() are liable to
execute differently depending on which pcpu a vcpu happens to be scheduled.

Beyond that, for now there should be a __read_mostly bool_t based on the
system verification, which is used in preference to reading the MSR each
time a guest does a cf8 access.

Someone with many tuits can see about making the entire guest MSR state
move in the migration stream, or I will probably get to it after getting
the cpuid work done.

~Andrew


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