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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/06/15 07:29, Jan Beulich wrote:
>>>> On 16.06.15 at 20:26, <andrew.cooper3@citrix.com> wrote:
>> On 16/06/15 09:09, Jan Beulich wrote:
>>>>>> On 15.06.15 at 17:32, <andrew.cooper3@citrix.com> 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.
>> Sorry - I had completely forgotten that this was a v2, and had already
>> asked this question.
>>
>> However, hvm_select_ioreq_server() it not a rare function to call, and I
>> am still concerned with the overhead.
> And I can only repeat that the MSR isn't being accessed unless an
> apparent extended access is being seen.

Ok - I suppose that isn't too bad.  Combined with below, it should never
actually happen.

>
>> It turns out that MSR_AMD64_NB_CFG is unconditionally RAZ and has all
>> writes discarded, so no HVM guest will ever be in a position to
>> legitimately use AMD extended configuration access.
> Where have you found that? The register (named NB_CFG1 in
> newer families' BKGDs) is clearly r/w.

It is implemented as RAZ/write discard in the hvm msr intercept code,
and appears to exist only to prevent the guest blowing up in a
cross-vendor case.

~Andrew