| Message ID | 20250327153627.307040-1-rakeshjb010@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci-host/gt64120.c: Fix PCI host bridge endianness handling | expand |
On Thu, 27 Mar 2025, rakeshj wrote: > The GT-64120 PCI controller requires special handling where: > 1. Host bridge (device 0) must use native endianness > 2. Other devices follow MByteSwap bit in GT_PCI0_CMD > > Previous implementation accidentally swapped all accesses, breaking > host bridge detection (lspci -d 11ab:4620). This fix: > > - Adds device filtering via (phb->config_reg & 0x00FFF800) > - Preserves native endianness for host bridge > - Maintains swapping for other devices in big-endian mode > > Fixes: 145e2198 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2826 > Signed-off-by: rakeshj <rakeshjb010@gmail.com> > --- > hw/pci-host/gt64120.c | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c > index d5c13a89b6..098f8e5988 100644 > --- a/hw/pci-host/gt64120.c > +++ b/hw/pci-host/gt64120.c > @@ -320,11 +320,46 @@ static void gt64120_isd_mapping(GT64120State *s) > memory_region_transaction_commit(); > } > > +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, unsigned size) > +{ > + GT64120State *s = opaque; > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > + uint32_t val = pci_data_read(phb->bus, phb->config_reg, size); > + > + /* Only swap for non-bridge devices in big-endian mode */ > + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { > + val = bswap32(val); I don't know if this is the best way to fix this issue as I don't know what the issue is in the first place (isn't PCI usually little endian?) but I think you can't just use bswap here because it also needs to take into account the endianness of the host QEMU is running on. So most likely you need le32_to_cpu or be32_to_cpu or similar here as you're converting a value with known endianness to a variable that is in host endian and the *_to_cpu functions are for that. But I could be wrong, these endianness issues are quite confusing. It's best if you can test on both LE and BE host. Regards, BALATON Zoltan > + } > + return val; > +} > + > +static void gt64120_pci_data_write(void *opaque, hwaddr addr, > + uint64_t val, unsigned size) > +{ > + GT64120State *s = opaque; > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { > + val = bswap32(val); > + } > + if (phb->config_reg & (1u << 31)) > + pci_data_write(phb->bus, phb->config_reg | (addr & 3), val, size); > +} > + > +static const MemoryRegionOps gt64120_pci_data_ops = { > + .read = gt64120_pci_data_read, > + .write = gt64120_pci_data_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid = { > + .min_access_size = 1, > + .max_access_size = 4, > + }, > +}; > + > static void gt64120_update_pci_cfgdata_mapping(GT64120State *s) > { > /* Indexed on MByteSwap bit, see Table 158: PCI_0 Command, Offset: 0xc00 */ > static const MemoryRegionOps *pci_host_data_ops[] = { > - &pci_host_data_be_ops, &pci_host_data_le_ops > + >64120_pci_data_ops, &pci_host_data_le_ops > }; > PCIHostState *phb = PCI_HOST_BRIDGE(s); > >
On 3/27/25 21:50, BALATON Zoltan wrote: > On Thu, 27 Mar 2025, rakeshj wrote: >> The GT-64120 PCI controller requires special handling where: >> 1. Host bridge (device 0) must use native endianness >> 2. Other devices follow MByteSwap bit in GT_PCI0_CMD >> >> Previous implementation accidentally swapped all accesses, breaking >> host bridge detection (lspci -d 11ab:4620). This fix: >> >> - Adds device filtering via (phb->config_reg & 0x00FFF800) >> - Preserves native endianness for host bridge >> - Maintains swapping for other devices in big-endian mode >> >> Fixes: 145e2198 ("hw/mips/gt64xxx_pci: Endian-swap using >> PCI_HOST_BRIDGE MemoryRegionOps") >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2826 >> Signed-off-by: rakeshj <rakeshjb010@gmail.com> >> --- >> hw/pci-host/gt64120.c | 37 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 36 insertions(+), 1 deletion(-) >> >> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c >> index d5c13a89b6..098f8e5988 100644 >> --- a/hw/pci-host/gt64120.c >> +++ b/hw/pci-host/gt64120.c >> @@ -320,11 +320,46 @@ static void gt64120_isd_mapping(GT64120State *s) >> memory_region_transaction_commit(); >> } >> >> +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, >> unsigned size) >> +{ >> + GT64120State *s = opaque; >> + PCIHostState *phb = PCI_HOST_BRIDGE(s); >> + uint32_t val = pci_data_read(phb->bus, phb->config_reg, size); >> + >> + /* Only swap for non-bridge devices in big-endian mode */ >> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { >> + val = bswap32(val); > > I don't know if this is the best way to fix this issue as I don't know > what the issue is in the first place (isn't PCI usually little endian?) Yes but this particular PCI host bridge is the exception, as it is the only user of pci_host_data_be_ops. > but I think you can't just use bswap here because it also needs to take > into account the endianness of the host QEMU is running on. It should be fine. You should take into account: - the endianness produced by pci_data_read/pci_data_write (always little endian) - the endianness expected by the guest (big endian under the conditions in the patch) - the endianness expected by memory.c (always little endian, as specified in gt64120_pci_data_ops) Because there is either zero or one mismatch, bswap32 is fine. *However*, there is some extra complication that is unnecessary after this patch: - right now the !(s->regs[GT_PCI0_CMD] & 1) that you have added is dead code: if it was ever 1, gt64120_update_pci_cfgdata_mapping() would pick pci_host_data_ops[1] and gt64120_pci_data_read/write would not run at all! - but alternatively you could keep that conditional, and get rid completely of gt64120_update_pci_cfgdata_mapping(). Just keep the initialization code memory_region_init_io(&phb->data_mem, OBJECT(phb), >64120_pci_data_ops, s, "pci-conf-data", 4); memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGDATA << 2, &phb->data_mem, 1); at the end of gt64120_realize() where the sister region phb->conf_mem is already being initialized. This would actually make me happy. Either way, pci_host_data_be_ops is dead after this patch and you can remove it; and since you are at it, you may also want to remove the wrong comment /* * PCI_CONFIG_ADDR is parent_obj.config_reg, via pci_host_conf_be_ops, * so that we can map PCI_CONFIG_DATA to pci_host_data_be_ops. */ in include/hw/pci-host/dino.h: DINO emulation has *never* used pci_host_data_be_ops. This has snowballed a bit, but it should not be a problem. :) Paolo
On Fri, 28 Mar 2025, Paolo Bonzini wrote: > On 3/27/25 21:50, BALATON Zoltan wrote: >> On Thu, 27 Mar 2025, rakeshj wrote: >>> The GT-64120 PCI controller requires special handling where: >>> 1. Host bridge (device 0) must use native endianness >>> 2. Other devices follow MByteSwap bit in GT_PCI0_CMD >>> >>> Previous implementation accidentally swapped all accesses, breaking >>> host bridge detection (lspci -d 11ab:4620). This fix: >>> >>> - Adds device filtering via (phb->config_reg & 0x00FFF800) >>> - Preserves native endianness for host bridge >>> - Maintains swapping for other devices in big-endian mode >>> >>> Fixes: 145e2198 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE >>> MemoryRegionOps") >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2826 >>> Signed-off-by: rakeshj <rakeshjb010@gmail.com> >>> --- >>> hw/pci-host/gt64120.c | 37 ++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 36 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c >>> index d5c13a89b6..098f8e5988 100644 >>> --- a/hw/pci-host/gt64120.c >>> +++ b/hw/pci-host/gt64120.c >>> @@ -320,11 +320,46 @@ static void gt64120_isd_mapping(GT64120State *s) >>> memory_region_transaction_commit(); >>> } >>> >>> +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, unsigned >>> size) >>> +{ >>> + GT64120State *s = opaque; >>> + PCIHostState *phb = PCI_HOST_BRIDGE(s); >>> + uint32_t val = pci_data_read(phb->bus, phb->config_reg, size); >>> + >>> + /* Only swap for non-bridge devices in big-endian mode */ >>> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { >>> + val = bswap32(val); >> >> I don't know if this is the best way to fix this issue as I don't know what >> the issue is in the first place (isn't PCI usually little endian?) > > Yes but this particular PCI host bridge is the exception, as it is the only > user of pci_host_data_be_ops. > >> but I think you can't just use bswap here because it also needs to take >> into account the endianness of the host QEMU is running on. > > It should be fine. You should take into account: > > - the endianness produced by pci_data_read/pci_data_write (always little > endian) > > - the endianness expected by the guest (big endian under the conditions in > the patch) > > - the endianness expected by memory.c (always little endian, as specified in > gt64120_pci_data_ops) > > Because there is either zero or one mismatch, bswap32 is fine. This may worth a comment but I'm still not convinced this works on big endian host because I think pci_data_read returns val in host endianness and if you want big endian then you only need to bswap on LE host not on BE host. Was this tested on BE host and confirmed it works? Regards, BALATON Zoltan > *However*, there is some extra complication that is unnecessary after this > patch: > > - right now the !(s->regs[GT_PCI0_CMD] & 1) that you have added is dead code: > if it was ever 1, gt64120_update_pci_cfgdata_mapping() would pick > pci_host_data_ops[1] and gt64120_pci_data_read/write would not run at all! > > - but alternatively you could keep that conditional, and get rid completely > of gt64120_update_pci_cfgdata_mapping(). Just keep the initialization code > > memory_region_init_io(&phb->data_mem, OBJECT(phb), > >64120_pci_data_ops, > s, "pci-conf-data", 4); > memory_region_add_subregion_overlap(&s->ISD_mem, GT_PCI0_CFGDATA << 2, > &phb->data_mem, 1); > > at the end of gt64120_realize() where the sister region phb->conf_mem is > already being initialized. This would actually make me happy. > > Either way, pci_host_data_be_ops is dead after this patch and you can remove > it; and since you are at it, you may also want to remove the wrong comment > > /* > * PCI_CONFIG_ADDR is parent_obj.config_reg, via pci_host_conf_be_ops, > * so that we can map PCI_CONFIG_DATA to pci_host_data_be_ops. > */ > > in include/hw/pci-host/dino.h: DINO emulation has *never* used > pci_host_data_be_ops. > > This has snowballed a bit, but it should not be a problem. :) > > Paolo > >
On Fri, Mar 28, 2025 at 3:16 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > It should be fine. You should take into account: > > > > - the endianness produced by pci_data_read/pci_data_write (always little > > endian) > > > > - the endianness expected by the guest (big endian under the conditions in > > the patch) > > > > - the endianness expected by memory.c (always little endian, as specified in > > gt64120_pci_data_ops) > > > > Because there is either zero or one mismatch, bswap32 is fine. > > This may worth a comment but I'm still not convinced this works on big > endian host because I think pci_data_read returns val in host endianness > and if you want big endian then you only need to bswap on LE host not on > BE host. Was this tested on BE host and confirmed it works? Looking again at the code, there is definitely one problem: since you have + .min_access_size = 1, + .max_access_size = 4, the bswap can also be bswap16 if size == 2 (and bswap32 only if size == 4). Also WRT to what Zoltan is saying, I think it's safe to just add an extra swap, as long as it's of the correct size. The swap changes the combined action of ops->read and adjust_endianness() to have 1 or 2 swaps instead of 0 and 1, and that is the same as inverting the result of devend_memop(). The loops in access_with_adjusted_size() could be a problem but they do not matter here, because split accesses are never needed (impl.*_access_size are the same as valid.*_access_size). Thanks, Paolo Paolo
On 28/03/2025 15.16, BALATON Zoltan wrote: > On Fri, 28 Mar 2025, Paolo Bonzini wrote: >> On 3/27/25 21:50, BALATON Zoltan wrote: >>> On Thu, 27 Mar 2025, rakeshj wrote: >>>> The GT-64120 PCI controller requires special handling where: >>>> 1. Host bridge (device 0) must use native endianness >>>> 2. Other devices follow MByteSwap bit in GT_PCI0_CMD >>>> >>>> Previous implementation accidentally swapped all accesses, breaking >>>> host bridge detection (lspci -d 11ab:4620). This fix: >>>> >>>> - Adds device filtering via (phb->config_reg & 0x00FFF800) >>>> - Preserves native endianness for host bridge >>>> - Maintains swapping for other devices in big-endian mode >>>> >>>> Fixes: 145e2198 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE >>>> MemoryRegionOps") >>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2826 >>>> Signed-off-by: rakeshj <rakeshjb010@gmail.com> >>>> --- >>>> hw/pci-host/gt64120.c | 37 ++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 36 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c >>>> index d5c13a89b6..098f8e5988 100644 >>>> --- a/hw/pci-host/gt64120.c >>>> +++ b/hw/pci-host/gt64120.c >>>> @@ -320,11 +320,46 @@ static void gt64120_isd_mapping(GT64120State *s) >>>> memory_region_transaction_commit(); >>>> } >>>> >>>> +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, >>>> unsigned size) >>>> +{ >>>> + GT64120State *s = opaque; >>>> + PCIHostState *phb = PCI_HOST_BRIDGE(s); >>>> + uint32_t val = pci_data_read(phb->bus, phb->config_reg, size); >>>> + >>>> + /* Only swap for non-bridge devices in big-endian mode */ >>>> + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { >>>> + val = bswap32(val); >>> >>> I don't know if this is the best way to fix this issue as I don't know >>> what the issue is in the first place (isn't PCI usually little endian?) >> >> Yes but this particular PCI host bridge is the exception, as it is the >> only user of pci_host_data_be_ops. >> >>> but I think you can't just use bswap here because it also needs to take >>> into account the endianness of the host QEMU is running on. >> >> It should be fine. You should take into account: >> >> - the endianness produced by pci_data_read/pci_data_write (always little >> endian) >> >> - the endianness expected by the guest (big endian under the conditions in >> the patch) >> >> - the endianness expected by memory.c (always little endian, as specified >> in gt64120_pci_data_ops) >> >> Because there is either zero or one mismatch, bswap32 is fine. > > This may worth a comment but I'm still not convinced this works on big > endian host because I think pci_data_read returns val in host endianness and > if you want big endian then you only need to bswap on LE host not on BE > host. Was this tested on BE host and confirmed it works? FWIW, I just checked the patch on a big endian host, and it seems to work as expected, with both qemu-system-mips and qemu-system-mipsel the "lspci -d 11ab:4620" was working correctly there. Thomas
On 28/3/25 18:34, Paolo Bonzini wrote: > On Fri, Mar 28, 2025 at 3:16 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> It should be fine. You should take into account: >>> >>> - the endianness produced by pci_data_read/pci_data_write (always little >>> endian) >>> >>> - the endianness expected by the guest (big endian under the conditions in >>> the patch) >>> >>> - the endianness expected by memory.c (always little endian, as specified in >>> gt64120_pci_data_ops) >>> >>> Because there is either zero or one mismatch, bswap32 is fine. >> >> This may worth a comment but I'm still not convinced this works on big >> endian host because I think pci_data_read returns val in host endianness >> and if you want big endian then you only need to bswap on LE host not on >> BE host. Was this tested on BE host and confirmed it works? > > Looking again at the code, there is definitely one problem: since you have > > + .min_access_size = 1, > + .max_access_size = 4, > > the bswap can also be bswap16 if size == 2 (and bswap32 only if size == 4). Per 'PCI LOCAL BUS SPECIFICATION, REV. 3.0': ''' 3.2.2.3.2. Software Generation of Configuration Transactions Anytime a host bridge sees a full DWORD I/O write from the host to CONFIG_ADDRESS, the bridge must latch the data into its CONFIG_ADDRESS register. On full DWORD I/O reads to CONFIG_ADDRESS, the bridge must return the data in CONFIG_ADDRESS. Any other types of accesses to this address (non-DWORD) have no effect on CONFIG_ADDRESS and are executed as normal I/O transactions on the PCI bus. Therefore, the only I/O Space consumed by this register is a DWORD at the given address. I/O devices that share the same address but use BYTE or WORD registers are not affected because their transactions will pass through the host bridge unchanged. ''' IIUC we don't need the bswap16.
Thanks for feedback on [PATCH v1]! I've posted v2 incorporating the suggestions:ve posted v2 incorporating your suggestions Paolo: You pointed out the size issue with .min_access_size = 1 and .max_access_size = 4, where bswap32 was wrong for 2-byte accesses. I’ve fixed this with size-appropriate swaps (bswap16 for 2-byte, bswap32 for 4-byte). On the extra swap idea, I stuck with a single swap since it aligns PCI LE with guest BE expectations without overcomplicating it—let me know if I misunderstood. I’ve sent [PATCH v2] incorporating changes: 1.Removed gt64120_update_pci_cfgdata_mapping() and moved initialization code to gt64120_realize() for a simpler MByteSwap check. 2.Removed unused pci_host_data_be_ops and a misleading comment in dino.h 3.Size-specific swaps (bswap16 and bswap32) I included bswap16 for 2-byte accesses in v2—should this be restricted to 4-byte only (bswap32) per the spec, or does GT-64120 expect 2-byte config swaps too? It’s a minor tweak, so I left it in v2 for now—happy to adjust in a v3 if needed. The new patch is available at: https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html Could you take a look at [PATCH v2] and let me know your thoughts, especially on the 2-byte swap question? Thanks! On Sat, Mar 29, 2025 at 4:05 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 28/3/25 18:34, Paolo Bonzini wrote: > > On Fri, Mar 28, 2025 at 3:16 PM BALATON Zoltan <balaton@eik.bme.hu> > wrote: > >>> It should be fine. You should take into account: > >>> > >>> - the endianness produced by pci_data_read/pci_data_write (always > little > >>> endian) > >>> > >>> - the endianness expected by the guest (big endian under the > conditions in > >>> the patch) > >>> > >>> - the endianness expected by memory.c (always little endian, as > specified in > >>> gt64120_pci_data_ops) > >>> > >>> Because there is either zero or one mismatch, bswap32 is fine. > >> > >> This may worth a comment but I'm still not convinced this works on big > >> endian host because I think pci_data_read returns val in host endianness > >> and if you want big endian then you only need to bswap on LE host not on > >> BE host. Was this tested on BE host and confirmed it works? > > > > Looking again at the code, there is definitely one problem: since you > have > > > > + .min_access_size = 1, > > + .max_access_size = 4, > > > > the bswap can also be bswap16 if size == 2 (and bswap32 only if size == > 4). > > Per 'PCI LOCAL BUS SPECIFICATION, REV. 3.0': > > ''' > 3.2.2.3.2. Software Generation of Configuration Transactions > > Anytime a host bridge sees a full DWORD I/O write from the host > to CONFIG_ADDRESS, the bridge must latch the data into its > CONFIG_ADDRESS register. On full DWORD I/O reads to CONFIG_ADDRESS, > the bridge must return the data in CONFIG_ADDRESS. Any other types > of accesses to this address (non-DWORD) have no effect on CONFIG_ADDRESS > and are executed as normal I/O transactions on the PCI bus. Therefore, > the only I/O Space consumed by this register is a DWORD at the given > address. I/O devices that share the same address but use BYTE or WORD > registers are not affected because their transactions will pass through > the host bridge unchanged. > ''' > > IIUC we don't need the bswap16. > >
On Sat, Mar 29, 2025 at 12:31 PM Rakesh J <rakeshjb010@gmail.com> wrote: > Paolo: You pointed out the size issue with .min_access_size = 1 and .max_access_size = 4, where bswap32 was wrong for 2-byte accesses. I’ve fixed this with size-appropriate swaps (bswap16 for 2-byte, bswap32 for 4-byte). On the extra swap idea, I stuck with a single swap since it aligns PCI LE with guest BE expectations without overcomplicating it—let me know if I misunderstood. The extra swap (compared to what the "regular" PCI data ops do) is exactly what you were doing. > I’ve sent [PATCH v2] incorporating changes: > 1.Removed gt64120_update_pci_cfgdata_mapping() and moved initialization code > to gt64120_realize() for a simpler MByteSwap check. > 2.Removed unused pci_host_data_be_ops and a misleading comment in dino.h > > 3.Size-specific swaps (bswap16 and bswap32) > I included bswap16 for 2-byte accesses in v2—should this be restricted to 4-byte only (bswap32) per the spec, or does GT-64120 expect 2-byte config swaps too? It’s a minor tweak, so I left it in v2 for now—happy to adjust in a v3 if needed. Which swap to use is not really related to what the GT-64120 does, but to the interface between memory.c and the MemoryRegionOps. When access_size == 2, QEMU wants the result in bits 0..15 so you need to use bswap16. With bswap32, the result would be in bits 16..31. Paolo
On 29/3/25 12:30, Rakesh J wrote: > Thanks for feedback on [PATCH v1]! > > I've posted v2 incorporating the suggestions:ve posted v2 incorporating > your suggestions > > Paolo: You pointed out the size issue with .min_access_size = 1 > and .max_access_size = 4, where bswap32 was wrong for 2-byte accesses. > I’ve fixed this with size-appropriate swaps (bswap16 for 2-byte, bswap32 > for 4-byte). On the extra swap idea, I stuck with a single swap since it > aligns PCI LE with guest BE expectations without overcomplicating it—let > me know if I misunderstood. > > I’ve sent [PATCH v2] incorporating changes: > 1.Removed gt64120_update_pci_cfgdata_mapping() and moved initialization code > to gt64120_realize() for a simpler MByteSwap check. > 2.Removed unused pci_host_data_be_ops and a misleading comment in dino.h > > 3.Size-specific swaps (bswap16 and bswap32) > I included bswap16 for 2-byte accesses in v2—should this be restricted > to 4-byte only (bswap32) per the spec, or does GT-64120 expect 2-byte > config swaps too? It’s a minor tweak, so I left it in v2 for now—happy > to adjust in a v3 if needed. > > The new patch is available at: > https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html > <https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html> > Could you take a look at [PATCH v2] and let me know your thoughts, > especially on the 2-byte swap question? Thanks! > > On Sat, Mar 29, 2025 at 4:05 PM Philippe Mathieu-Daudé > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > On 28/3/25 18:34, Paolo Bonzini wrote: > > On Fri, Mar 28, 2025 at 3:16 PM BALATON Zoltan > <balaton@eik.bme.hu <mailto:balaton@eik.bme.hu>> wrote: > >>> It should be fine. You should take into account: > >>> > >>> - the endianness produced by pci_data_read/pci_data_write > (always little > >>> endian) > >>> > >>> - the endianness expected by the guest (big endian under the > conditions in > >>> the patch) > >>> > >>> - the endianness expected by memory.c (always little endian, as > specified in > >>> gt64120_pci_data_ops) > >>> > >>> Because there is either zero or one mismatch, bswap32 is fine. > >> > >> This may worth a comment but I'm still not convinced this works > on big > >> endian host because I think pci_data_read returns val in host > endianness > >> and if you want big endian then you only need to bswap on LE > host not on > >> BE host. Was this tested on BE host and confirmed it works? > > > > Looking again at the code, there is definitely one problem: since > you have > > > > + .min_access_size = 1, > > + .max_access_size = 4, > > > > the bswap can also be bswap16 if size == 2 (and bswap32 only if > size == 4). > > Per 'PCI LOCAL BUS SPECIFICATION, REV. 3.0': > > ''' > 3.2.2.3.2. Software Generation of Configuration Transactions > > Anytime a host bridge sees a full DWORD I/O write from the host > to CONFIG_ADDRESS, the bridge must latch the data into its > CONFIG_ADDRESS register. On full DWORD I/O reads to CONFIG_ADDRESS, > the bridge must return the data in CONFIG_ADDRESS. Any other types > of accesses to this address (non-DWORD) have no effect on > CONFIG_ADDRESS > and are executed as normal I/O transactions on the PCI bus. Therefore, > the only I/O Space consumed by this register is a DWORD at the given > address. I/O devices that share the same address but use BYTE or WORD > registers are not affected because their transactions will pass through > the host bridge unchanged. > ''' > > IIUC we don't need the bswap16. What I'm questioning is why we need .min_access_size = 1 and not .min_access_size = 4.
re-examining the PCI spec (3.2.2.3.2) and QEMU's PCI host bridge implementations, I agree .min_access_size = 4 is actually more correct I'll update the patch(v4) to: -Set .min_access_size = 4 in MemoryRegionOps -Remove the now-unnecessary bswap16 handling On Mon, Mar 31, 2025 at 5:04 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 29/3/25 12:30, Rakesh J wrote: > > Thanks for feedback on [PATCH v1]! > > > > I've posted v2 incorporating the suggestions:ve posted v2 incorporating > > your suggestions > > > > Paolo: You pointed out the size issue with .min_access_size = 1 > > and .max_access_size = 4, where bswap32 was wrong for 2-byte accesses. > > I’ve fixed this with size-appropriate swaps (bswap16 for 2-byte, bswap32 > > for 4-byte). On the extra swap idea, I stuck with a single swap since it > > aligns PCI LE with guest BE expectations without overcomplicating it—let > > me know if I misunderstood. > > > > I’ve sent [PATCH v2] incorporating changes: > > 1.Removed gt64120_update_pci_cfgdata_mapping() and moved initialization > code > > to gt64120_realize() for a simpler MByteSwap check. > > 2.Removed unused pci_host_data_be_ops and a misleading comment in dino.h > > > > 3.Size-specific swaps (bswap16 and bswap32) > > I included bswap16 for 2-byte accesses in v2—should this be restricted > > to 4-byte only (bswap32) per the spec, or does GT-64120 expect 2-byte > > config swaps too? It’s a minor tweak, so I left it in v2 for now—happy > > to adjust in a v3 if needed. > > > > The new patch is available at: > > https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html > > <https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06884.html> > > Could you take a look at [PATCH v2] and let me know your thoughts, > > especially on the 2-byte swap question? Thanks! > > > > On Sat, Mar 29, 2025 at 4:05 PM Philippe Mathieu-Daudé > > <philmd@linaro.org <mailto:philmd@linaro.org>> wrote: > > > > On 28/3/25 18:34, Paolo Bonzini wrote: > > > On Fri, Mar 28, 2025 at 3:16 PM BALATON Zoltan > > <balaton@eik.bme.hu <mailto:balaton@eik.bme.hu>> wrote: > > >>> It should be fine. You should take into account: > > >>> > > >>> - the endianness produced by pci_data_read/pci_data_write > > (always little > > >>> endian) > > >>> > > >>> - the endianness expected by the guest (big endian under the > > conditions in > > >>> the patch) > > >>> > > >>> - the endianness expected by memory.c (always little endian, as > > specified in > > >>> gt64120_pci_data_ops) > > >>> > > >>> Because there is either zero or one mismatch, bswap32 is fine. > > >> > > >> This may worth a comment but I'm still not convinced this works > > on big > > >> endian host because I think pci_data_read returns val in host > > endianness > > >> and if you want big endian then you only need to bswap on LE > > host not on > > >> BE host. Was this tested on BE host and confirmed it works? > > > > > > Looking again at the code, there is definitely one problem: since > > you have > > > > > > + .min_access_size = 1, > > > + .max_access_size = 4, > > > > > > the bswap can also be bswap16 if size == 2 (and bswap32 only if > > size == 4). > > > > Per 'PCI LOCAL BUS SPECIFICATION, REV. 3.0': > > > > ''' > > 3.2.2.3.2. Software Generation of Configuration Transactions > > > > Anytime a host bridge sees a full DWORD I/O write from the host > > to CONFIG_ADDRESS, the bridge must latch the data into its > > CONFIG_ADDRESS register. On full DWORD I/O reads to CONFIG_ADDRESS, > > the bridge must return the data in CONFIG_ADDRESS. Any other types > > of accesses to this address (non-DWORD) have no effect on > > CONFIG_ADDRESS > > and are executed as normal I/O transactions on the PCI bus. > Therefore, > > the only I/O Space consumed by this register is a DWORD at the given > > address. I/O devices that share the same address but use BYTE or WORD > > registers are not affected because their transactions will pass > through > > the host bridge unchanged. > > ''' > > > > IIUC we don't need the bswap16. > > What I'm questioning is why we need .min_access_size = 1 > and not .min_access_size = 4. >
diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c index d5c13a89b6..098f8e5988 100644 --- a/hw/pci-host/gt64120.c +++ b/hw/pci-host/gt64120.c @@ -320,11 +320,46 @@ static void gt64120_isd_mapping(GT64120State *s) memory_region_transaction_commit(); } +static uint64_t gt64120_pci_data_read(void *opaque, hwaddr addr, unsigned size) +{ + GT64120State *s = opaque; + PCIHostState *phb = PCI_HOST_BRIDGE(s); + uint32_t val = pci_data_read(phb->bus, phb->config_reg, size); + + /* Only swap for non-bridge devices in big-endian mode */ + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { + val = bswap32(val); + } + return val; +} + +static void gt64120_pci_data_write(void *opaque, hwaddr addr, + uint64_t val, unsigned size) +{ + GT64120State *s = opaque; + PCIHostState *phb = PCI_HOST_BRIDGE(s); + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { + val = bswap32(val); + } + if (phb->config_reg & (1u << 31)) + pci_data_write(phb->bus, phb->config_reg | (addr & 3), val, size); +} + +static const MemoryRegionOps gt64120_pci_data_ops = { + .read = gt64120_pci_data_read, + .write = gt64120_pci_data_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 4, + }, +}; + static void gt64120_update_pci_cfgdata_mapping(GT64120State *s) { /* Indexed on MByteSwap bit, see Table 158: PCI_0 Command, Offset: 0xc00 */ static const MemoryRegionOps *pci_host_data_ops[] = { - &pci_host_data_be_ops, &pci_host_data_le_ops + >64120_pci_data_ops, &pci_host_data_le_ops }; PCIHostState *phb = PCI_HOST_BRIDGE(s);
The GT-64120 PCI controller requires special handling where: 1. Host bridge (device 0) must use native endianness 2. Other devices follow MByteSwap bit in GT_PCI0_CMD Previous implementation accidentally swapped all accesses, breaking host bridge detection (lspci -d 11ab:4620). This fix: - Adds device filtering via (phb->config_reg & 0x00FFF800) - Preserves native endianness for host bridge - Maintains swapping for other devices in big-endian mode Fixes: 145e2198 ("hw/mips/gt64xxx_pci: Endian-swap using PCI_HOST_BRIDGE MemoryRegionOps") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2826 Signed-off-by: rakeshj <rakeshjb010@gmail.com> --- hw/pci-host/gt64120.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-)