[PATCH 0/1] accel/tcg: Allow the second page of an instruction to be MMIO

[PATCH 0/1] accel/tcg: Allow the second page of an instruction to be MMIO

[Richard Henderson]

Curious but true: two independent reports of the same issue within
24 hours, one with an x86 guest and one with an arm guest.

Neither report included instructions for reproduction (and both seem
to be with complex setup), therefore this is untested, but seems simple
enough to be the proper fix.  It matches up with

    /*
     * If the TB is not associated with a physical RAM page then it must be
     * a temporary one-insn TB, and we have nothing left to do. Return early
     * before attempting to link to other TBs or add to the lookup table.
     */
    if (tb_page_addr0(tb) == -1) {
        return tb;
    }

in tb_gen_code().


r~


Richard Henderson (1):
  accel/tcg: Allow the second page of an instruction to be MMIO

 accel/tcg/translator.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/1] accel/tcg: Allow the second page of an instruction to be MMIO

[Richard Henderson]

If an instruction straddles a page boundary, and the first page
was ram, but the second page was MMIO, we would abort.  Handle
this as if both pages are MMIO, by setting the ram_addr_t for
the first page to -1.

Reported-by: Sid Manning <sidneym@quicinc.com>
Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/translator.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index ef5193c67e..1cf404ced0 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -176,8 +176,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);
-- 
2.34.1

Re: [PATCH 1/1] accel/tcg: Allow the second page of an instruction to be MMIO

[Philippe Mathieu-Daudé]

On 6/2/23 20:38, Richard Henderson wrote:
> If an instruction straddles a page boundary, and the first page
> was ram, but the second page was MMIO, we would abort.  Handle
> this as if both pages are MMIO, by setting the ram_addr_t for
> the first page to -1.
> 
> Reported-by: Sid Manning <sidneym@quicinc.com>
> Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/translator.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index ef5193c67e..1cf404ced0 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -176,8 +176,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);

Nice trick! I'm tempted to log it at CPU_LOG_EXEC (or
CPU_LOG_TB_CPU) level.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +                return NULL;
> +            }
> +
>               tb_set_page_addr1(tb, phys_page);
>   #ifdef CONFIG_USER_ONLY
>               page_protect(end);

Re: [PATCH 1/1] accel/tcg: Allow the second page of an instruction to be MMIO

[Jørgen Hansen]

On 2/6/23 20:38, Richard Henderson wrote:
> If an instruction straddles a page boundary, and the first page
> was ram, but the second page was MMIO, we would abort.  Handle
> this as if both pages are MMIO, by setting the ram_addr_t for
> the first page to -1.
> 
> Reported-by: Sid Manning <sidneym@quicinc.com>
> Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/translator.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index ef5193c67e..1cf404ced0 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -176,8 +176,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);
> --
> 2.34.1
> 

Thanks a lot for the quick turnaround. I've verified that the patch 
resolves the issue we experienced.

RE: [PATCH 1/1] accel/tcg: Allow the second page of an instruction to be MMIO

[Sid Manning]

> -----Original Message-----
> From: Jørgen Hansen <Jorgen.Hansen@wdc.com>
> Sent: Tuesday, February 7, 2023 9:03 AM
> To: Richard Henderson <richard.henderson@linaro.org>; qemu-
> devel@nongnu.org
> Cc: Sid Manning <sidneym@quicinc.com>; Mark Burton
> <mburton@qti.qualcomm.com>; Brian Cain <bcain@quicinc.com>; Matheus
> Bernardino <mathbern@qti.qualcomm.com>; Ajay Joshi
> <Ajay.Joshi@wdc.com>
> Subject: Re: [PATCH 1/1] accel/tcg: Allow the second page of an instruction to
> be MMIO
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On 2/6/23 20:38, Richard Henderson wrote:
> > If an instruction straddles a page boundary, and the first page was
> > ram, but the second page was MMIO, we would abort.  Handle this as if
> > both pages are MMIO, by setting the ram_addr_t for the first page to
> > -1.
> >
> > Reported-by: Sid Manning <sidneym@quicinc.com>
> > Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   accel/tcg/translator.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index
> > ef5193c67e..1cf404ced0 100644
> > --- a/accel/tcg/translator.c
> > +++ b/accel/tcg/translator.c
> > @@ -176,8 +176,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);
> > --
> > 2.34.1
> >
> 
> Thanks a lot for the quick turnaround. I've verified that the patch resolves
> the issue we experienced.

I also verified this patch fixed my issue.  Thanks!

Re: [PATCH 0/1] accel/tcg: Allow the second page of an instruction to be MMIO

[Richard Henderson]

On 2/6/23 09:38, Richard Henderson wrote:
> Curious but true: two independent reports of the same issue within
> 24 hours, one with an x86 guest and one with an arm guest.
> 
> Neither report included instructions for reproduction (and both seem
> to be with complex setup), therefore this is untested, but seems simple
> enough to be the proper fix.  It matches up with
> 
>      /*
>       * If the TB is not associated with a physical RAM page then it must be
>       * a temporary one-insn TB, and we have nothing left to do. Return early
>       * before attempting to link to other TBs or add to the lookup table.
>       */
>      if (tb_page_addr0(tb) == -1) {
>          return tb;
>      }
> 
> in tb_gen_code().
> 
> 
> r~
> 
> 
> Richard Henderson (1):
>    accel/tcg: Allow the second page of an instruction to be MMIO
> 
>   accel/tcg/translator.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 

Queued to tcg-next.


r~