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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.03.15 at 19:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Yes, but cross-vendor migration has (iirc) many more issues like this
(and considering the wide family range the risk of this breaking for
migration between AMD systems seems marginal).

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

I think this goes too far in mistrusting Dom0.

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

But it is part of the change to _not_ do the MSR access on each
CF8 one: We first check whether this at all looks like an extended
config space access. I.e. I considered eliminating the rdmsr, but
didn't consider it worthwhile for the change here.

Jan


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