Recent changes to pcie_host corrected size of its internal region to
match what it expects - only the low 28 bits are ever decoded. Previous
code just ignored bit 29 (if size was 1 << 29) in the address which does
not make much sense. We are now asserting on size > 1 << 28 instead,
but it so happened that ppc actually allows guest to configure as large
a size as it wants to, and current firmware set it to 1 << 29.
With just qemu-system-ppc -M sam460ex this triggers an assert which
seems to happen when the guest (board firmware?) writes a value to
CFGMSK reg:
(gdb) bt
This is done in the board firmware here:
https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD
when trying to map config space.
Note that what firmware does matches
https://www.hardware.com.br/comunidade/switch-cisco/1128380/
So it's not clear what the proper fix should be.
However, allowing guest to trigger an assert in qemu is not good practice anyway.
For now let's just force the mask to 256MB on guest write, this way
anything outside the expected address range is ignored.
Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX")
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Affected system is orphan so I guess I will merge the patch unless
someone objects.
hw/ppc/ppc440_uc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 993e3ba955..a1ecf6dd1c 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val)
case PEGPL_CFGMSK:
s->cfg_mask = val;
size = ~(val & 0xfffffffe) + 1;
+ /*
+ * Firmware sets this register to E0000001. Why we are not sure,
+ * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is
+ * ignored.
+ */
+ if (size > PCIE_MMCFG_SIZE_MAX) {
+ size = PCIE_MMCFG_SIZE_MAX;
+ }
pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size);
break;
case PEGPL_MSGBAH:
--
MST
Michael, I'll queue this patch with the commit msg proposed by Zoltan as follows: Author: Michael S. Tsirkin <mst@redhat.com> Date: Thu May 26 18:43:43 2022 -0400 ppc: fix boot with sam460ex Recent changes to pcie_host corrected size of its internal region to match what it expects: only the low 28 bits are ever decoded. Previous code just ignored bit 29 (if size was 1 << 29) in the address which does not make much sense. We are now asserting on size > 1 << 28 instead, but PPC 4xx actually allows guest to configure different sizes, and some firmwares seem to set it to 1 << 29. This caused e.g. qemu-system-ppc -M sam460ex to exit with an assert when the guest writes a value to CFGMSK register when trying to map config space. This is done in the board firmware in ppc4xx_init_pcie_port() in roms/u-boot-sam460ex/arch/powerpc/cpu/ppc4xx/4xx_pcie.c It's not clear what the proper fix should be but for now let's force the size to 256MB, so anything outside the expected address range is ignored. Is that ok with you? Thanks, Daniel On 5/26/22 19:43, Michael S. Tsirkin wrote: > Recent changes to pcie_host corrected size of its internal region to > match what it expects - only the low 28 bits are ever decoded. Previous > code just ignored bit 29 (if size was 1 << 29) in the address which does > not make much sense. We are now asserting on size > 1 << 28 instead, > but it so happened that ppc actually allows guest to configure as large > a size as it wants to, and current firmware set it to 1 << 29. > > With just qemu-system-ppc -M sam460ex this triggers an assert which > seems to happen when the guest (board firmware?) writes a value to > CFGMSK reg: > > (gdb) bt > > This is done in the board firmware here: > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD > > when trying to map config space. > > Note that what firmware does matches > https://www.hardware.com.br/comunidade/switch-cisco/1128380/ > > So it's not clear what the proper fix should be. > > However, allowing guest to trigger an assert in qemu is not good practice anyway. > > For now let's just force the mask to 256MB on guest write, this way > anything outside the expected address range is ignored. > > Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Affected system is orphan so I guess I will merge the patch unless > someone objects. > > hw/ppc/ppc440_uc.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 993e3ba955..a1ecf6dd1c 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) > case PEGPL_CFGMSK: > s->cfg_mask = val; > size = ~(val & 0xfffffffe) + 1; > + /* > + * Firmware sets this register to E0000001. Why we are not sure, > + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is > + * ignored. > + */ > + if (size > PCIE_MMCFG_SIZE_MAX) { > + size = PCIE_MMCFG_SIZE_MAX; > + } > pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); > break; > case PEGPL_MSGBAH:
On Mon, Jun 06, 2022 at 10:51:23AM -0300, Daniel Henrique Barboza wrote: > Michael, > > > I'll queue this patch with the commit msg proposed by Zoltan as follows: > > > Author: Michael S. Tsirkin <mst@redhat.com> > Date: Thu May 26 18:43:43 2022 -0400 > > ppc: fix boot with sam460ex > Recent changes to pcie_host corrected size of its internal region to > match what it expects: only the low 28 bits are ever decoded. Previous > code just ignored bit 29 (if size was 1 << 29) in the address which does > not make much sense. We are now asserting on size > 1 << 28 instead, > but PPC 4xx actually allows guest to configure different sizes, and some > firmwares seem to set it to 1 << 29. > This caused e.g. qemu-system-ppc -M sam460ex to exit with an assert when > the guest writes a value to CFGMSK register when trying to map config > space. This is done in the board firmware in ppc4xx_init_pcie_port() in > roms/u-boot-sam460ex/arch/powerpc/cpu/ppc4xx/4xx_pcie.c > It's not clear what the proper fix should be but for now let's force the > size to 256MB, so anything outside the expected address range is > ignored. > > > Is that ok with you? > > > Thanks, > > > Daniel ACK > > On 5/26/22 19:43, Michael S. Tsirkin wrote: > > Recent changes to pcie_host corrected size of its internal region to > > match what it expects - only the low 28 bits are ever decoded. Previous > > code just ignored bit 29 (if size was 1 << 29) in the address which does > > not make much sense. We are now asserting on size > 1 << 28 instead, > > but it so happened that ppc actually allows guest to configure as large > > a size as it wants to, and current firmware set it to 1 << 29. > > > > With just qemu-system-ppc -M sam460ex this triggers an assert which > > seems to happen when the guest (board firmware?) writes a value to > > CFGMSK reg: > > > > (gdb) bt > > > > This is done in the board firmware here: > > > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD > > > > when trying to map config space. > > > > Note that what firmware does matches > > https://www.hardware.com.br/comunidade/switch-cisco/1128380/ > > > > So it's not clear what the proper fix should be. > > > > However, allowing guest to trigger an assert in qemu is not good practice anyway. > > > > For now let's just force the mask to 256MB on guest write, this way > > anything outside the expected address range is ignored. > > > > Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") > > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Affected system is orphan so I guess I will merge the patch unless > > someone objects. > > > > hw/ppc/ppc440_uc.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > > index 993e3ba955..a1ecf6dd1c 100644 > > --- a/hw/ppc/ppc440_uc.c > > +++ b/hw/ppc/ppc440_uc.c > > @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) > > case PEGPL_CFGMSK: > > s->cfg_mask = val; > > size = ~(val & 0xfffffffe) + 1; > > + /* > > + * Firmware sets this register to E0000001. Why we are not sure, > > + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is > > + * ignored. > > + */ > > + if (size > PCIE_MMCFG_SIZE_MAX) { > > + size = PCIE_MMCFG_SIZE_MAX; > > + } > > pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); > > break; > > case PEGPL_MSGBAH:
[ resend with a reply-all ] On 5/27/22 00:43, Michael S. Tsirkin wrote: > Recent changes to pcie_host corrected size of its internal region to > match what it expects - only the low 28 bits are ever decoded. Previous > code just ignored bit 29 (if size was 1 << 29) in the address which does > not make much sense. We are now asserting on size > 1 << 28 instead, > but it so happened that ppc actually allows guest to configure as large > a size as it wants to, and current firmware set it to 1 << 29. > > With just qemu-system-ppc -M sam460ex this triggers an assert which > seems to happen when the guest (board firmware?) writes a value to > CFGMSK reg: > > (gdb) bt > > This is done in the board firmware here: > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD > > when trying to map config space. > > Note that what firmware does matches > https://www.hardware.com.br/comunidade/switch-cisco/1128380/ > > So it's not clear what the proper fix should be. > > However, allowing guest to trigger an assert in qemu is not good practice anyway. > > For now let's just force the mask to 256MB on guest write, this way > anything outside the expected address range is ignored. > > Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Affected system is orphan so I guess I will merge the patch unless > someone objects. Fine with me. Acked-by: Cédric Le Goater <clg@kaod.org> On the orphan status, MAINTAINERS file says the sam460ex machine is maintained. I understand that these files : hw/ppc/ppc440_uc.c hw/ppc/ppc440.h hw/ppc/ppc440_pcix.c hw/ppc/ppc4xx_devs.c include/hw/ppc/ppc4xx.h and these include/hw/i2c/ppc4xx_i2c.h hw/i2c/ppc4xx_i2c.c hw/intc/ppc-uic.c include/hw/intc/ppc-uic.h should be under the same entry since sam460ex depends on it. The ppc440 support is a bit of a mess to be honest. We have two 440 machines bamboo and sam460ex which have a lot in common a part from the PCI host bridge. Thanks, C. > > hw/ppc/ppc440_uc.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 993e3ba955..a1ecf6dd1c 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) > case PEGPL_CFGMSK: > s->cfg_mask = val; > size = ~(val & 0xfffffffe) + 1; > + /* > + * Firmware sets this register to E0000001. Why we are not sure, > + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is > + * ignored. > + */ > + if (size > PCIE_MMCFG_SIZE_MAX) { > + size = PCIE_MMCFG_SIZE_MAX; > + } > pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); > break; > case PEGPL_MSGBAH:
On Mon, 30 May 2022, Cédric Le Goater wrote: > [ resend with a reply-all ] > > On 5/27/22 00:43, Michael S. Tsirkin wrote: >> Recent changes to pcie_host corrected size of its internal region to >> match what it expects - only the low 28 bits are ever decoded. Previous >> code just ignored bit 29 (if size was 1 << 29) in the address which does >> not make much sense. We are now asserting on size > 1 << 28 instead, >> but it so happened that ppc actually allows guest to configure as large >> a size as it wants to, and current firmware set it to 1 << 29. >> >> With just qemu-system-ppc -M sam460ex this triggers an assert which >> seems to happen when the guest (board firmware?) writes a value to >> CFGMSK reg: >> >> (gdb) bt >> >> This is done in the board firmware here: >> >> https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD >> >> when trying to map config space. >> >> Note that what firmware does matches >> https://www.hardware.com.br/comunidade/switch-cisco/1128380/ >> >> So it's not clear what the proper fix should be. >> >> However, allowing guest to trigger an assert in qemu is not good practice >> anyway. >> >> For now let's just force the mask to 256MB on guest write, this way >> anything outside the expected address range is ignored. >> >> Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct >> PCIE_MMCFG_SIZE_MAX") >> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> >> Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> >> Affected system is orphan so I guess I will merge the patch unless >> someone objects. > > Fine with me. > > Acked-by: Cédric Le Goater <clg@kaod.org> > > > On the orphan status, > > MAINTAINERS file says the sam460ex machine is maintained. I understand that I'm maintaining sam460ex but I don't know much about other parts so don't want to take up all the PPC4xx emulation so only files I've written or made extensive changes so I know them are listed. Others are part of pre-existing PPC4xx emulation which may be orphan. > these files : > > hw/ppc/ppc440_uc.c > hw/ppc/ppc440.h > hw/ppc/ppc440_pcix.c The ppc440_pcix is already listed, the rest aren't something I know much about so wouldn't volunteer to maintain those. They are shared with other PPC 4xx machines. Regards, BALATON Zoltan > hw/ppc/ppc4xx_devs.c > include/hw/ppc/ppc4xx.h > > and these > > include/hw/i2c/ppc4xx_i2c.h > hw/i2c/ppc4xx_i2c.c > hw/intc/ppc-uic.c > include/hw/intc/ppc-uic.h > > should be under the same entry since sam460ex depends on it. > > The ppc440 support is a bit of a mess to be honest. We have two 440 > machines bamboo and sam460ex which have a lot in common a part from > the PCI host bridge. > > Thanks, > > C. > >> >> hw/ppc/ppc440_uc.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c >> index 993e3ba955..a1ecf6dd1c 100644 >> --- a/hw/ppc/ppc440_uc.c >> +++ b/hw/ppc/ppc440_uc.c >> @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, >> uint32_t val) >> case PEGPL_CFGMSK: >> s->cfg_mask = val; >> size = ~(val & 0xfffffffe) + 1; >> + /* >> + * Firmware sets this register to E0000001. Why we are not sure, >> + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is >> + * ignored. >> + */ >> + if (size > PCIE_MMCFG_SIZE_MAX) { >> + size = PCIE_MMCFG_SIZE_MAX; >> + } >> pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, >> size); >> break; >> case PEGPL_MSGBAH: > >
On 5/27/22 00:43, Michael S. Tsirkin wrote: > Recent changes to pcie_host corrected size of its internal region to > match what it expects - only the low 28 bits are ever decoded. Previous > code just ignored bit 29 (if size was 1 << 29) in the address which does > not make much sense. We are now asserting on size > 1 << 28 instead, > but it so happened that ppc actually allows guest to configure as large > a size as it wants to, and current firmware set it to 1 << 29. > > With just qemu-system-ppc -M sam460ex this triggers an assert which > seems to happen when the guest (board firmware?) writes a value to > CFGMSK reg: > > (gdb) bt > > This is done in the board firmware here: > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD > > when trying to map config space. > > Note that what firmware does matches > https://www.hardware.com.br/comunidade/switch-cisco/1128380/ > > So it's not clear what the proper fix should be. > > However, allowing guest to trigger an assert in qemu is not good practice anyway. > > For now let's just force the mask to 256MB on guest write, this way > anything outside the expected address range is ignored. > > Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Affected system is orphan so I guess I will merge the patch unless > someone objects. Fine with me. Acked-by: Cédric Le Goater <clg@kaod.org> On the orphan status, MAINTAINERS file says the sam460ex machine is maintained. I understand that these files : hw/ppc/ppc440_uc.c hw/ppc/ppc440.h hw/ppc/ppc440_pcix.c hw/ppc/ppc4xx_devs.c include/hw/ppc/ppc4xx.h and these include/hw/i2c/ppc4xx_i2c.h hw/i2c/ppc4xx_i2c.c hw/intc/ppc-uic.c include/hw/intc/ppc-uic.h should be under the same entry since sam460ex depends on it. The ppc440 support is a bit of a mess to be honest. We have two 440 machines bamboo and sam460ex which have a lot in common a part from the PCI host bridge. Thanks, C. > > hw/ppc/ppc440_uc.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 993e3ba955..a1ecf6dd1c 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) > case PEGPL_CFGMSK: > s->cfg_mask = val; > size = ~(val & 0xfffffffe) + 1; > + /* > + * Firmware sets this register to E0000001. Why we are not sure, > + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is > + * ignored. > + */ > + if (size > PCIE_MMCFG_SIZE_MAX) { > + size = PCIE_MMCFG_SIZE_MAX; > + } > pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); > break; > case PEGPL_MSGBAH:
Hello, Some changes to commit message (patch is OK). On Thu, 26 May 2022, Michael S. Tsirkin wrote: > Recent changes to pcie_host corrected size of its internal region to > match what it expects - only the low 28 bits are ever decoded. Previous > code just ignored bit 29 (if size was 1 << 29) in the address which does > not make much sense. We are now asserting on size > 1 << 28 instead, > but it so happened that ppc actually allows guest to configure as large > a size as it wants to, and current firmware set it to 1 << 29. > > With just qemu-system-ppc -M sam460ex this triggers an assert which > seems to happen when the guest (board firmware?) writes a value to > CFGMSK reg: > > (gdb) bt Backtrace is missing but you could just drop this line and replace : with . at end of previous line as we probably don't need the full backtrace, the commit message is already too long in my opinion. > This is done in the board firmware here: > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD > > when trying to map config space. > > Note that what firmware does matches > https://www.hardware.com.br/comunidade/switch-cisco/1128380/ That's not it. It's a different hardware and firmware, just quoted it as an example that this value seems to be common to that SoC even on different hardware/OS/firmware (probably comes from reference hardware/devel board?). The sam460ex is here https://www.acube-systems.biz/index.php?page=hardware&pid=5 the U-Boot in above repo is matching the firmware from the acube page but I had to fix some bugs in it to make it compile and work. Otherwise this should be OK. Regards, BALATON Zoltan > So it's not clear what the proper fix should be. > > However, allowing guest to trigger an assert in qemu is not good practice anyway. > > For now let's just force the mask to 256MB on guest write, this way > anything outside the expected address range is ignored. > > Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Affected system is orphan so I guess I will merge the patch unless > someone objects. > > hw/ppc/ppc440_uc.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 993e3ba955..a1ecf6dd1c 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) > case PEGPL_CFGMSK: > s->cfg_mask = val; > size = ~(val & 0xfffffffe) + 1; > + /* > + * Firmware sets this register to E0000001. Why we are not sure, > + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is > + * ignored. > + */ > + if (size > PCIE_MMCFG_SIZE_MAX) { > + size = PCIE_MMCFG_SIZE_MAX; > + } > pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); > break; > case PEGPL_MSGBAH: >
On Fri, May 27, 2022 at 12:46:57PM +0200, BALATON Zoltan wrote: > Hello, > > Some changes to commit message (patch is OK). Want to write the commit message for me then? > On Thu, 26 May 2022, Michael S. Tsirkin wrote: > > Recent changes to pcie_host corrected size of its internal region to > > match what it expects - only the low 28 bits are ever decoded. Previous > > code just ignored bit 29 (if size was 1 << 29) in the address which does > > not make much sense. We are now asserting on size > 1 << 28 instead, > > but it so happened that ppc actually allows guest to configure as large > > a size as it wants to, and current firmware set it to 1 << 29. > > > > With just qemu-system-ppc -M sam460ex this triggers an assert which > > seems to happen when the guest (board firmware?) writes a value to > > CFGMSK reg: > > > > (gdb) bt > > Backtrace is missing but you could just drop this line and replace : with . > at end of previous line as we probably don't need the full backtrace, the > commit message is already too long in my opinion. > > > This is done in the board firmware here: > > > > https://git.qemu.org/?p=u-boot-sam460ex.git;a=blob;f=arch/powerpc/cpu/ppc4xx/4xx_pcie.c;h=13348be93dccc74c13ea043d6635a7f8ece4b5f0;hb=HEAD > > > > when trying to map config space. > > > > Note that what firmware does matches > > https://www.hardware.com.br/comunidade/switch-cisco/1128380/ > > That's not it. It's a different hardware and firmware, just quoted it as an > example that this value seems to be common to that SoC even on different > hardware/OS/firmware (probably comes from reference hardware/devel board?). > The sam460ex is here > > https://www.acube-systems.biz/index.php?page=hardware&pid=5 > > the U-Boot in above repo is matching the firmware from the acube page but I > had to fix some bugs in it to make it compile and work. > > Otherwise this should be OK. > > Regards, > BALATON Zoltan > > > So it's not clear what the proper fix should be. > > > > However, allowing guest to trigger an assert in qemu is not good practice anyway. > > > > For now let's just force the mask to 256MB on guest write, this way > > anything outside the expected address range is ignored. > > > > Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") > > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Affected system is orphan so I guess I will merge the patch unless > > someone objects. > > > > hw/ppc/ppc440_uc.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > > index 993e3ba955..a1ecf6dd1c 100644 > > --- a/hw/ppc/ppc440_uc.c > > +++ b/hw/ppc/ppc440_uc.c > > @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) > > case PEGPL_CFGMSK: > > s->cfg_mask = val; > > size = ~(val & 0xfffffffe) + 1; > > + /* > > + * Firmware sets this register to E0000001. Why we are not sure, > > + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is > > + * ignored. > > + */ > > + if (size > PCIE_MMCFG_SIZE_MAX) { > > + size = PCIE_MMCFG_SIZE_MAX; > > + } > > pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); > > break; > > case PEGPL_MSGBAH: > >
On Fri, 27 May 2022, Michael S. Tsirkin wrote: > On Fri, May 27, 2022 at 12:46:57PM +0200, BALATON Zoltan wrote: >> Hello, >> >> Some changes to commit message (patch is OK). > > Want to write the commit message for me then? How about: Recent changes to pcie_host corrected size of its internal region to match what it expects: only the low 28 bits are ever decoded. Previous code just ignored bit 29 (if size was 1 << 29) in the address which does not make much sense. We are now asserting on size > 1 << 28 instead, but PPC 4xx actually allows guest to configure different sizes, and some firmwares seem to set it to 1 << 29. This caused e.g. qemu-system-ppc -M sam460ex to exit with an assert when the guest writes a value to CFGMSK register when trying to map config space. This is done in the board firmware in ppc4xx_init_pcie_port() in roms/u-boot-sam460ex/arch/powerpc/cpu/ppc4xx/4xx_pcie.c It's not clear what the proper fix should be but for now let's force the size to 256MB, so anything outside the expected address range is ignored. >>> Fixes: commit 1f1a7b2269 ("include/hw/pci/pcie_host: Correct PCIE_MMCFG_SIZE_MAX") >>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> >>> Affected system is orphan so I guess I will merge the patch unless >>> someone objects. >>> >>> hw/ppc/ppc440_uc.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c >>> index 993e3ba955..a1ecf6dd1c 100644 >>> --- a/hw/ppc/ppc440_uc.c >>> +++ b/hw/ppc/ppc440_uc.c >>> @@ -1180,6 +1180,14 @@ static void dcr_write_pcie(void *opaque, int dcrn, uint32_t val) >>> case PEGPL_CFGMSK: >>> s->cfg_mask = val; >>> size = ~(val & 0xfffffffe) + 1; >>> + /* >>> + * Firmware sets this register to E0000001. Why we are not sure, >>> + * but the current guess is anything above PCIE_MMCFG_SIZE_MAX is >>> + * ignored. >>> + */ >>> + if (size > PCIE_MMCFG_SIZE_MAX) { >>> + size = PCIE_MMCFG_SIZE_MAX; >>> + } >>> pcie_host_mmcfg_update(PCIE_HOST_BRIDGE(s), val & 1, s->cfg_base, size); >>> break; >>> case PEGPL_MSGBAH: >>> > >
© 2016 - 2024 Red Hat, Inc.