Re: [GIT preview] for-6.3/cxl-ram-region

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 1 Feb 2023 16:16:34 -0500
Gregory Price <gregory.price@memverge.com> wrote:

> On Wed, Feb 01, 2023 at 12:29:50AM -0500, Gregory Price wrote:
> > > Are you using single root port configuration? If yes, the following
> > > patch should have fixed the issue,
> > > https://lore.kernel.org/linux-cxl/20221215170909.2650271-1-fan.ni@samsung.com/
> > >   
> > > > [   97.476231] RSP: 0018:ffffa30000d23d20 EFLAGS: 00010292                      
> > 
> > I did not have this patch.  This should definitely make its way up.
> > 
> > ~Gregory  
> 
> 
> This fixed up the stack trace for me, but memregion is still failing to
> successfully complete.
> 
> It looks like configuration and decoder commit completes, but then
> cxl-cli bails out because writing
> 
> echo region0 > /sys/bus/cxl/drivers/cxl_region/bind
> 
> results in "Failed to synchronize CPU cache state"
> 
> I presume this is because of either a logic error or because the memory
> just isn't actually hooked up yet, but this is the relevant code:
> 
> 
> static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
> {
>   if (!test_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags))
>     return 0;
> 
>   if (!cpu_cache_has_invalidate_memregion()) {
>     if (IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)) {
>       dev_warn_once(
>         &cxlr->dev,
>         "Bypassing cpu_cache_invalidate_memregion() for testing!\n");
>       clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
>       return 0;
>     } else {
>       dev_err(&cxlr->dev,
>         "Failed to synchronize CPU cache state\n");
>       return -ENXIO;
>     }
>   }
> 
>   cpu_cache_invalidate_memregion(IORES_DESC_CXL);
>   clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
>   return 0;
> }
> 
> 
> 
> Looks like i can bypass this with CONFIG_CXL_REGION_INVALIDATION_TEST
> but just wanted to report back incase this is not intended.
> 
> On x86, this invalidate_memregion() call maps to not having the
> hypervisor bit set:
> 
> bool cpu_cache_has_invalidate_memregion(void)
> {
>   return !cpu_feature_enabled(X86_FEATURE_HYPERVISOR);
> }
> EXPORT_SYMBOL_NS_GPL(cpu_cache_has_invalidate_memregion, DEVMEM);

Yup. I don't think we currently support this properly even when
doing TCG mode on QEMU rather than KVM.

Note that there is another QEMU issue that needs resolving if you intend
to use this as normal memory and it's worse under KVM. Effects the
corner case where an instruction crosses the boundary from normal memory
into CXL memory.

Thanks to the various QEMU folk who are helping us figure out what to do
about this for the explanations that follow!

We currently handle the region as MMIO - in QEMU terms, no actual relationship
to what the OS sees (as need to mess with the address
mappings on each access for interleaving). That's a problem for KVM
(which may not cope with sub page granularity remapping under the hood).

https://lore.kernel.org/qemu-devel/ff3f25ee-1c98-242b-905e-0b01d9f0948d@linaro.org/#r

Also a problem in TCG because the handling of executing out of MMIO takes
a shortcut.  It is fine (though very low performance as using a fall back path)
for fully in MMIO regions, but not the corner case where the start of the instruction
is in normal RAM (with all the related fast paths and instruction caching) and
the end of the instruction is in the CXL MMIO region (a CFMWS window).

Currently looks like the fix will be to use the slow path for this case.
Patches welcome!

Anyhow, in meantime beware.

Jonathan


> 
> 
> 
> I presume if i enable the invalidate_test bit in my config this will
> work, but if anyone can validate that this is expected behavior without
> it, that would be great.
> 
> Thanks!
> ~Gregory