Re: [RFC] CXL: TCG/KVM instruction alignment issue discussion default

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 18/2/23 11:22, Gregory Price wrote:
> 
> Breaking this off into a separate thread for archival sake.
> 
> There's a bug with handling execution of instructions held in CXL
> memory - specifically when an instruction crosses a page boundary.
> 
> The result of this is that type-3 devices cannot use KVM at all at the
> moment, and require the attached patch to run in TCG-only mode.
> 
> 
> CXL memory devices are presently emulated as MMIO, and MMIO has no
> coherency guarantees, so TCG doesn't cache the results of translating
> an instruction, meaning execution is incredibly slow (orders of
> magnitude slower than KVM).
> 
> 
> Request for comments:
> 
> 
> First there's the stability issue:
> 
> 0) TCG cannot handle instructions across a page boundary spanning ram and
>     MMIO. See attached patch for hotfix.  This basically solves the page
>     boundary issue by reverting the entire block to MMIO-mode if the
>     problem is detected.
> 
> 1) KVM needs to be investigated.  It's likely the same/similar issue,
>     but it's not confirmed.
> 
> 
> 
> Second there's the performance issue:
> 
> 0) Do we actually care about performance? How likely are users to
>     attempt to run software out of CXL memory?
> 
> 1) If we do care, is there a potential for converting CXL away from the
>     MMIO design?  The issue is coherency for shared memory. Emulating
>     coherency is a) hard, and b) a ton of work for little gain.
> 
>     Presently marking CXL memory as MMIO basically enforces coherency by
>     preventing caching, though it's unclear how this is enforced
>     by KVM (or if it is, i have to imagine it is).
> 
> 
> 
> It might be nice to solve this for non-shared memory regions, but
> testing functionality >>> performance at this point so it might not
> worth the investment.
> 
> 
> Just tossing this out for discussion
> ~Gregory
> 
> 
> 
> 
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 061519691f..cd383d7125 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -171,8 +171,16 @@ static void *translator_access(CPUArchState *env, DisasContextBase *db,
>           if (host == NULL) {
>               tb_page_addr_t phys_page =
>                   get_page_addr_code_hostp(env, base, &db->host_addr[1]);
> -            /* We cannot handle MMIO as second page. */
> -            assert(phys_page != -1);
> +
> +            /*
> +            * If the second page is MMIO, treat as if the first page
> +            * was MMIO as well, so that we do not cache the TB.
> +            */
> +            if (unlikely(phys_page == -1)) {
> +                tb_set_page_addr0(tb, -1);
> +                return NULL;
> +            }
> +
>               tb_set_page_addr1(tb, phys_page);
>   #ifdef CONFIG_USER_ONLY
>               page_protect(end);
> 

This is:
https://lore.kernel.org/qemu-devel/20230222020023.904232-2-richard.henderson@linaro.org/