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

[Philippe Mathieu-Daudé]

On 6/24/21 8:01 PM, 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.

Oh, good eyes :)

Indeed I see:

static uint32_t bonito_sbridge_pciaddr(void *opaque, hwaddr addr)
{
    ...
    regno = (cfgaddr & BONITO_PCICONF_REG_MASK) >>
BONITO_PCICONF_REG_OFFSET;
    ...

Having:

#define BONITO_PCICONF_REG_MASK        0xFC
#define BONITO_PCICONF_REG_OFFSET      0

I'll look at what I have on Bonito.