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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2023 19:57, Andrew Cooper wrote:
> This started by noticing the dubious Fam17h check in
> arch_ioreq_server_get_type_addr(), and then realising that checking the host
> CF8_EXT setting is utterly bogus for the guest <-> qemu emulation path.
> 
> What should be consulted here is the guest's MSR_AMD64_NB_CFG setting, but a)
> there isn't one, and b) the vestigial remnants of the cross-vendor migration
> logic cause MSR_AMD64_NB_CFG to be unconditionally read-as-zero, making the
> CF8_EXT path unused by any suitably-written OS in the first place.
> 
> MSR_AMD64_NB_CFG really has been removed on Fam17h (it's now just a read-zero,
> write-discard stub), and the ECS extension is unconditionally active, meaning
> it is not correct for Xen to ignore the ExtRegNo field on newer AMD CPUs.

I don't buy "unconditionally active". Whether to enable this by default is
up to firmware; I view it as not unlikely that some firmware actually have
a setup control for it. Fam17 controls it through D1[8-F]F4x044:0, while
Fam19 has D1[8-F]F0xC00:0 for this purpose. (I've had a work item for quite
some time to actually make use of this bit to enable ECS.)

> It turns out that Xen did even have this behaviour in 4.5 and earlier, with
> this problematic CF8_EXT checking being added in 4.6.  Therefore, revert back
> to Xen's older behaviour - it is objectively less wrong than the current
> logic.
> 
> While fixing this, get rid of hvm_pci_decode_addr() - it is more complicated
> to follow (and to call) than using the CF8* macros in the calling context.
> Rename CF8_ADDR() to CF8_REG() to better describe what it does, and write a
> comment explaining all about CF8/CFC accesses.
> 
> There's one rare case when CF8_EXT is visible to guests, and that is for a
> pinned hwdom.  Right now, we permit such a dom0 to modify the CF8_EXT bit, but
> this seems like a very unwise idea.  Leave a TODO for people to consider.
> 
> Fixes: e0fbf3bf9871 ("x86/AMD: correct certain Fam17 checks")

Therefore I'm not convinced this Fixes: tag is warranted.

> Fixes: 2d67a7a4d37a ("x86: synchronize PCI config space access decoding")

I'm also curious which particular aspect of that change you are considering
to be fixed here.

Your claim about behavior being reserved is perhaps okay as far as hardware
is concerned, but by removing the checks you e.g. misguide
xsm_pci_config_permission() in case it handles certain register ranges
specially. Dealing with that was, according to the description, one of the
purposes of the commit above (it's been long enough, so I can only go from
the description in git).

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -248,20 +248,6 @@ void register_g2m_portio_handler(struct domain *d)
>      handler->ops = &g2m_portio_ops;
>  }
>  
> -unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
> -                                 pci_sbdf_t *sbdf)
> -{
> -    ASSERT(CF8_ENABLED(cf8));
> -
> -    sbdf->bdf = CF8_BDF(cf8);
> -    sbdf->seg = 0;
> -    /*
> -     * NB: the lower 2 bits of the register address are fetched from the
> -     * offset into the 0xcfc register when reading/writing to it.
> -     */
> -    return CF8_ADDR_LO(cf8) | (addr & 3);
> -}

I have to admit that I'm surprised that you fold replacing of this
function (purely mechanical afaict) into a change with a significant
functional change. Wouldn't you agree that this may better be split off?

> @@ -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.
> +              */
>               ((val ^ temp) & ~(1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
>              goto invalid;
>          if ( wrmsr_safe(MSR_AMD64_NB_CFG, val) == 0 )

What do you mean by "can't see the full system"? Originally Linux used
only the MSR view of the bit to enable ECS, but this was specifically
extended to prefer the PCI config space view instead (24d9b70b8c67
"x86: Use PCI method for enabling AMD extended config space before MSR
method").

Since here we're dealing with the MSR flavor, and since Linux shouldn't
be using this anymore (due to checking whether the bit is clear before
trying to set it), we may be okay with simply purging this code if we
don't care about very old Linux Dom0 anymore (or if we use your
argument of it not reliably affecting the entire system).

As to the setting becoming Xen-owned: Dom0 being responsible for
(almost) all of PCI, it would be somewhat odd to take away from it this
level of control.

Jan