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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.06.15 at 17:32, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/06/15 15:30, Jan Beulich wrote:
>> @@ -2439,9 +2434,19 @@ struct hvm_ioreq_server *hvm_select_iore
>>  
>>          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) &&
>> +             d->arch.x86_vendor == X86_VENDOR_AMD &&
>> +             d->arch.x86 >= 0x10 && d->arch.x86 <= 0x17 )
>> +        {
>> +            uint64_t msr_val;
>> +
>> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> 
> We now have several common paths which read this MSR looking for CF8_EXT.
> 
> I think it would make sense to probe this on boot and have a
> cpu_has_amd_cf8_ext rather than repeatedly sampling an off-cpu MSR,
> although this would require synchronising it across all northbridges in
> emulate privileged op.
> 
> Alternatively, it might just be better to unconditionally enable it
> during startup (as Linux does) and prevent dom0 from playing, which
> would avoid the need to synchronise updates to it.

You just repeat what you said for v1, without taking into
consideration my reply thereto: Us not using this method
ourselves, we should honor and play by what Dom0 does.

>> @@ -1787,9 +1790,9 @@ static bool_t pci_cfg_ok(struct domain *
>>          if ( ro_map && test_bit(machine_bdf, ro_map) )
>>              return 0;
>>      }
>> -    start = currd->arch.pci_cf8 & 0xFF;
>> +    start |= CF8_ADDR_LO(currd->arch.pci_cf8);
> 
> This, combined with the change to the callers, looks suspect.
> 
> The callers are both accesses at cfc, with port&3 being the offset at
> the port.  This logical or here is combining the base offset to cfc with
> the destination address requested via the setting in cf8.
> 
> Is this intentional, and ifso, why?

It is: First of all you need to consider what start is being used for -
solely the call to xsm_pci_config_permission(). And there we want
the precise range of config space fields being accessed, not some
rough estimate thereof (i.e. the current code is broken in this
regard, and the fix is even spelled out in the commit message).

>> --- a/xen/include/asm-x86/pci.h
>> +++ b/xen/include/asm-x86/pci.h
>> @@ -1,6 +1,11 @@
>>  #ifndef __X86_PCI_H__
>>  #define __X86_PCI_H__
>>  
>> +#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))
> 
> As you are moving these, would you mind lining the '(cf8) &' bits
> vertically, which will make them far easier to read.

Can do, sure.

Jan


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