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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 6/24/21 8:29 PM, BALATON Zoltan wrote:
> On Thu, 24 Jun 2021, BALATON Zoltan wrote:
>> 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.
> 
> Now I remember I've seen this once:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2020-12/msg06299.html

Pffff... Déjà vu.

Using Jiaxun's patch also allows the following accesses
(which were previously discarded):

pci_cfg_read vt82c686b-isa 05:0 @0x81 -> 0x0
pci_cfg_write vt82c686b-isa 05:0 @0x81 <- 0x80
pci_cfg_write vt82c686b-isa 05:0 @0x83 <- 0x89
pci_cfg_write vt82c686b-isa 05:0 @0x85 <- 0x3
pci_cfg_write vt82c686b-isa 05:0 @0x5a <- 0x7
pci_cfg_write vt82c686b-isa 05:0 @0x85 <- 0x1

Good news is the machine boots.