Re: [PATCH] x86/pci: Correct ECS handling with CF8/CFC emulation

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

On Tue, Apr 04, 2023 at 02:27:36PM +0100, Andrew Cooper wrote:
> On 03/04/2023 2:26 pm, Roger Pau Monné wrote:
> > On Mon, Apr 03, 2023 at 11:16:52AM +0100, Andrew Cooper wrote:
> >> On 03/04/2023 9:57 am, Roger Pau Monné wrote:
> >> (Quick tangent...  Our PCI handling is currently very dumb. 
> >> pci_mmcfg_read() returns its value by pointer but the callers never
> >> check.  Swapping it to return by value would improve code gen quite a
> >> lot.  Also, when MMCFG is active we still pass BCS accesses to IO ports.)
> > I wonder if it's really preferred to access registers below 255 using
> > the IO ports, as Linux seems to do the same (prefer IO port access if
> > possible).
> 
> And see how many attempts there have been to change this, only blocked
> on untangling the IO port mess on other architectures (a problem Xen
> doesn't have to contend with).
> 
> MMCFG, when available is strictly preferable to IO ports.
> 
> An MMCFG access is a single UC read or write, whereas IO ports are a
> pair of UC accesses *and* a global spinlock.

Right, I know it's better from a performance PoV, but didn't know
whether there was some known glitches for not using MMCFG when
accessing regs < 255.

> >>>>  
> >>>>  #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \
> >>>>                          || id == 0x01268086 || id == 0x01028086 \
> >>>> diff --git a/xen/arch/x86/pv/emul-priv-op.c 
> >>>> b/xen/arch/x86/pv/emul-priv-op.c
> >>>> index 5da00e24e4ff..008367195c78 100644
> >>>> --- a/xen/arch/x86/pv/emul-priv-op.c
> >>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
> >>>> @@ -245,19 +245,7 @@ static bool pci_cfg_ok(struct domain *currd, 
> >>>> unsigned int start,
> >>>>          if ( ro_map && test_bit(machine_bdf, ro_map) )
> >>>>              return false;
> >>>>      }
> >>>> -    start |= CF8_ADDR_LO(currd->arch.pci_cf8);
> >>>> -    /* AMD extended configuration space access? */
> >>>> -    if ( CF8_ADDR_HI(currd->arch.pci_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) )
> >>>> -            return false;
> >>>> -        if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) )
> >>>> -            start |= CF8_ADDR_HI(currd->arch.pci_cf8);
> >>>> -    }
> >>>> +    start |= CF8_REG(currd->arch.pci_cf8);
> >>>>  
> >>>>      return !write ?
> >>>>             xsm_pci_config_permission(XSM_HOOK, currd, machine_bdf,
> >>>> @@ -1104,6 +1092,11 @@ static int cf_check write_msr(
> >>>>          if ( !is_hwdom_pinned_vcpu(curr) )
> >>>>              return X86EMUL_OKAY;
> >>>>          if ( (rdmsr_safe(MSR_AMD64_NB_CFG, temp) != 0) ||
> >>>> +             /*
> >>>> +              * TODO: this is broken.  What happens when dom0 is pinned 
> >>>> but
> >>>> +              * can't see the full system?  CF8_EXT probably ought to 
> >>>> be a
> >>>> +              * Xen-owned setting, and made symmetric across the system.
> >>>> +              */
> >>> I would assume CF8_EXT would be symmetric across the system, specially
> >>> given that it seems to be phased out and only used in older AMD
> >>> families that where all symmetric?
> >> The CF8_EXT bit has been phased out.  The IO ECS functionality still 
> >> exists.
> >>
> >> But yes, the more I think about letting dom0 play with this, the more I
> >> think it is a fundamentally broken idea...  I bet it was from the very
> >> early AMD Fam10h days where dom0 knew how to turn it on, and Xen was
> >> trying to pretend it didn't have to touch any PCI devices.
> > It seems to me Xen should set CF8_EXT on all threads (when available)
> > and expose it to dom0, so that accesses using pci_{conf,write}_read()
> > work as expected?
> 
> It's per northbridge in the system, not per thread.  Hence needing the
> spinlock protecting the CF8/CFC IO port pair access, and why MMCFG is
> strictly preferable.

So just setting CF8_EXT_ENABLE on MSR_AMD64_NB_CFG by the BSP should
be enough to have it enabled?  I expect all other threads will see the
bit as set in the MSR then.

Thanks, Roger.