Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers

[BALATON Zoltan]

On Thu, 24 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/24/21 7:00 PM, BALATON Zoltan wrote:
> > On Thu, 24 Jun 2021, Philippe Mathieu-Daudé wrote:
> > > On 6/24/21 6:16 PM, Philippe Mathieu-Daudé wrote:
> > > > On 6/24/21 6:01 PM, Philippe Mathieu-Daudé wrote:
> > > > > On 6/24/21 5:46 PM, Philippe Mathieu-Daudé wrote:
> > > > > > Hi Zoltan,
> > > > > >
> > > > > > On 2/21/21 3:34 PM, Philippe Mathieu-Daudé wrote:
> > > > > > > From: BALATON Zoltan <balaton@eik.bme.hu>
> > > > > > >
> > > > > > > The base address of the SMBus io ports and its enabled status is set
> > > > > > > by registers in the PCI config space but this was not correctly
> > > > > > > emulated. Instead the SMBus registers were mapped on realize to the
> > > > > > > base address set by a property to the address expected by fuloong2e
> > > > > > > firmware.
> > > > > > >
> > > > > > > Fix the base and config register handling to more closely model
> > > > > > > hardware which allows to remove the property and allows the guest to
> > > > > > > control this mapping. Do all this in reset instead of realize so it's
> > > > > > > correctly updated on reset.
> > > > > >
> > > > > > This commit broken running PMON on Fuloong2E:
> > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg752605.html
> > > > > > console: PMON2000 MIPS Initializing. Standby...
> > > > > > console: ERRORPC=00000000 CONFIG=00030932
> > > > > > console: PRID=00006302
> > > > > > console: DIMM read
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > console: 000000ff
> > > > > > ...
> > > > > >
> > > > > > From here the console loops displaying this value...
> > > > >
> > > > > Tracing:
>
> > > > pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1
> > >
> > > Offset 93-90 – SMBus I/O Base ....................................... RW
> > > 15-4 I/O Base (16-byte I/O space)................ default = 00h
> > > pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1
> > >
> > > > pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1
> > >
> > > Offset D2 – SMBus Host Configuration ......................... RW
> > > SMBus Host Controller Enable
> > > 0 Disable SMB controller functions ......... default
> > > 1 Enable SMB controller functions
> > > pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1
> > >
> > > Hmm the datasheet indeed document 0xd2... why is the guest accessing
> > > 0xd0 to enable the function? It seems this is the problem, since if
> > > I replace d2 -> d0 PMON boots. See below [*].
>
> > > > > > Expected:
> > > > > >
> > > > > > console: PMON2000 MIPS Initializing. Standby...
> > > > > > console: ERRORPC=00000000 CONFIG=00030932
> > > > > > console: PRID=00006302
> > > > > > console: DIMM read
> > > > > > console: 00000080
> > > > > > console: read memory type
> > > > > > console: read number of rows
> > > > > > ...
>
> > > > > > >  static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t
> > > > > > > val, int len)
> > > > > > >  {
> > > > > > > +    VT686PMState *s = VT82C686B_PM(d);
> > > > > > > +
> > > > > > >      trace_via_pm_write(addr, val, len);
> > > > > > >      pci_default_write_config(d, addr, val, len);
> > > > > > > +    if (ranges_overlap(addr, len, 0x90, 4)) {
> > > > > > > +        uint32_t v = pci_get_long(s->dev.config + 0x90);
> > > > > > > +        pci_set_long(s->dev.config + 0x90, (v & 0xfff0UL) | 1);
> > > > > > > +    }
> > > > > > > +    if (range_covers_byte(addr, len, 0xd2)) {
> > > > > > > +        s->dev.config[0xd2] &= 0xf;
> > > > > > > +        smb_io_space_update(s);
> > >
> > > [*] So the guest writing at 0xd0, this block is skipped, the
> > > I/O region never enabled.
> >
> > Could it be it does word or dword i/o to access multiple addresses at
> > once. Wasn't there a recent change to memory regions that could break
> > this? Is adjusting valid access sizes to the mem region ops needed now
> > to have the memory region handle this?
>
> Do you mean it was buggy earlier, so to accept a guest write at 0xd0
> the code had to handle the 0xd2 address? 0xd2 is the address in the
> datasheet, so I doubt.

No, I meant that instead of writing a byte to 0xd2 the guest might write a 
dword to 0xd0 which also overlaps 0xd2 and would change that but it does 
not reach the device for some reason. But in your trace there was:

> > mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80490 value 
> > 0xeee1 size 4
> > mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe804d2 value 
> > 0x1 size 2
>
> These are:
> pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1
> pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1

Where size is 2 so it would not reach 0xd2 but the address part above is 
0x1fe804d2 which somehow comes out as 0xd0 in the PCI trace so looks like 
something strips the low bits within PCI code and the guest does intend to 
access 0xd2 but it's not passed on to the device as such.

Regards,
BALATON Zoltan