| Message ID | 20210224124756.12491-1-patrice.chotard@foss.st.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | firmware: scmi: Replace memcpy_from/toio() by memcpy() in scmi_read/write_resp_from_smt() | expand |
> From: Patrice Chotard <patrice.chotard@foss.st.com> > Date: Wed, 24 Feb 2021 13:47:55 +0100 > > To avoid "synchronous abort" in AARCH64 in case the "from" address > is not aligned, replace memcpy_toio() and memcpy_fromio() by memcpy(). > > Signed-off-by: Patrice Chotard <patrice.chotard@st.com> > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > --- > > drivers/firmware/scmi/smt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This doesnt really make sense. There is no guarantee that memcpy() doesn't do an unaligned load or store either. Unaligned loads and stores from/to normal memory should just work on arm64, and memcpy_toio() and memcpy_fromio() make sure they don't do unaligned loads and stores from/to "IO" memory (in this case the shared memory buffer). > diff --git a/drivers/firmware/scmi/smt.c b/drivers/firmware/scmi/smt.c > index d25478796a..b001163b62 100644 > --- a/drivers/firmware/scmi/smt.c > +++ b/drivers/firmware/scmi/smt.c > @@ -93,7 +93,7 @@ int scmi_write_msg_to_smt(struct udevice *dev, struct scmi_smt *smt, > SMT_HEADER_PROTOCOL_ID(msg->protocol_id) | > SMT_HEADER_MESSAGE_ID(msg->message_id); > > - memcpy_toio(hdr->msg_payload, msg->in_msg, msg->in_msg_sz); > + memcpy(hdr->msg_payload, msg->in_msg, msg->in_msg_sz); > > return 0; > } > @@ -124,7 +124,7 @@ int scmi_read_resp_from_smt(struct udevice *dev, struct scmi_smt *smt, > > /* Get the data */ > msg->out_msg_sz = hdr->length - sizeof(hdr->msg_header); > - memcpy_fromio(msg->out_msg, hdr->msg_payload, msg->out_msg_sz); > + memcpy(msg->out_msg, hdr->msg_payload, msg->out_msg_sz); > > return 0; > } > -- > 2.17.1 > >
Hi Mark On 2/24/21 3:05 PM, Mark Kettenis wrote: >> From: Patrice Chotard <patrice.chotard@foss.st.com> >> Date: Wed, 24 Feb 2021 13:47:55 +0100 >> >> To avoid "synchronous abort" in AARCH64 in case the "from" address >> is not aligned, replace memcpy_toio() and memcpy_fromio() by memcpy(). >> >> Signed-off-by: Patrice Chotard <patrice.chotard@st.com> >> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> >> --- >> >> drivers/firmware/scmi/smt.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > This doesnt really make sense. There is no guarantee that memcpy() > doesn't do an unaligned load or store either. > > Unaligned loads and stores from/to normal memory should just work on > arm64, and memcpy_toio() and memcpy_fromio() make sure they don't do > unaligned loads and stores from/to "IO" memory (in this case the > shared memory buffer). You are right. After further analysis, i understood why the "synchronous abort" occurs. At early stage, before U-Boot relocation, MMU is disabled and then all the "normal" memory space is not yet configured with MT_NORMAL type. In this situation, all 64bits accesses to a none 64 bits aligned pointer in "normal memory" triggers a synchronous abort. Same accesses, with MMU configured (after U-Boot relocation), are OK. I will propose a new patch which will update memcpy_from/toio() and forbid 64bits accesses to non aligned 64bits pointer in case MMU is not yet enabled. Thanks Patrice > >> diff --git a/drivers/firmware/scmi/smt.c b/drivers/firmware/scmi/smt.c >> index d25478796a..b001163b62 100644 >> --- a/drivers/firmware/scmi/smt.c >> +++ b/drivers/firmware/scmi/smt.c >> @@ -93,7 +93,7 @@ int scmi_write_msg_to_smt(struct udevice *dev, struct scmi_smt *smt, >> SMT_HEADER_PROTOCOL_ID(msg->protocol_id) | >> SMT_HEADER_MESSAGE_ID(msg->message_id); >> >> - memcpy_toio(hdr->msg_payload, msg->in_msg, msg->in_msg_sz); >> + memcpy(hdr->msg_payload, msg->in_msg, msg->in_msg_sz); >> >> return 0; >> } >> @@ -124,7 +124,7 @@ int scmi_read_resp_from_smt(struct udevice *dev, struct scmi_smt *smt, >> >> /* Get the data */ >> msg->out_msg_sz = hdr->length - sizeof(hdr->msg_header); >> - memcpy_fromio(msg->out_msg, hdr->msg_payload, msg->out_msg_sz); >> + memcpy(msg->out_msg, hdr->msg_payload, msg->out_msg_sz); >> >> return 0; >> } >> -- >> 2.17.1 >> >>
diff --git a/drivers/firmware/scmi/smt.c b/drivers/firmware/scmi/smt.c index d25478796a..b001163b62 100644 --- a/drivers/firmware/scmi/smt.c +++ b/drivers/firmware/scmi/smt.c @@ -93,7 +93,7 @@ int scmi_write_msg_to_smt(struct udevice *dev, struct scmi_smt *smt, SMT_HEADER_PROTOCOL_ID(msg->protocol_id) | SMT_HEADER_MESSAGE_ID(msg->message_id); - memcpy_toio(hdr->msg_payload, msg->in_msg, msg->in_msg_sz); + memcpy(hdr->msg_payload, msg->in_msg, msg->in_msg_sz); return 0; } @@ -124,7 +124,7 @@ int scmi_read_resp_from_smt(struct udevice *dev, struct scmi_smt *smt, /* Get the data */ msg->out_msg_sz = hdr->length - sizeof(hdr->msg_header); - memcpy_fromio(msg->out_msg, hdr->msg_payload, msg->out_msg_sz); + memcpy(msg->out_msg, hdr->msg_payload, msg->out_msg_sz); return 0; }