| Message ID | 20230206193809.1153124-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Allow the second page of an instruction to be MMIO | expand |
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);
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.
> -----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!
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);
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(-)