On Tue, 2 Jan 2024, Bernhard Beschow wrote:
Am 24. Dezember 2023 00:51:53 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
On Tue, 19 Dec 2023, Bernhard Beschow wrote:
Am 19. Dezember 2023 00:26:15 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
On Mon, 18 Dec 2023, Bernhard Beschow wrote:
The VIA south bridges are able to relocate and toggle (enable or disable) their
SuperI/O functions. So far this is hardcoded such that all functions are always
enabled and are located at fixed addresses.
Some PC BIOSes seem to probe for I/O occupancy before activating such a function
and issue an error in case of a conflict. Since the functions are enabled on
reset, conflicts are always detected. Prevent that by implementing relocation
and toggling of the SuperI/O functions.
Note that all SuperI/O functions are now deactivated upon reset (except for
VT82C686B's serial ports where Fuloong 2e's rescue-yl seems to expect them to be
enabled by default). Rely on firmware -- or in case of pegasos2 on board code if
no -bios is given -- to configure the functions accordingly.
Pegasos2 emulates firmware when no -bios is given, this was explained in
previos commit so maybe not needed to be explained it here again so you could
drop the comment between -- -- but I don't mind.
Signed-off-by: Bernhard Beschow <shen...@gmail.com>
---
hw/isa/vt82c686.c | 121 ++++++++++++++++++++++++++++++++++------------
1 file changed, 90 insertions(+), 31 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9c2333a277..be202d23cf 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -15,6 +15,9 @@
#include "qemu/osdep.h"
#include "hw/isa/vt82c686.h"
+#include "hw/block/fdc.h"
+#include "hw/char/parallel-isa.h"
+#include "hw/char/serial.h"
#include "hw/pci/pci.h"
#include "hw/qdev-properties.h"
#include "hw/ide/pci.h"
@@ -343,6 +346,35 @@ static const TypeInfo via_superio_info = {
#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
+static void vt82c686b_superio_update(ViaSuperIOState *s)
+{
+ isa_parallel_set_enabled(s->superio.parallel[0],
+ (s->regs[0xe2] & 0x3) != 3);
+ isa_serial_set_enabled(s->superio.serial[0], s->regs[0xe2] & BIT(2));
+ isa_serial_set_enabled(s->superio.serial[1], s->regs[0xe2] & BIT(3));
+ isa_fdc_set_enabled(s->superio.floppy, s->regs[0xe2] & BIT(4));
+
+ isa_fdc_set_iobase(s->superio.floppy, (s->regs[0xe3] & 0xfc) << 2);
+ isa_parallel_set_iobase(s->superio.parallel[0], s->regs[0xe6] << 2);
+ isa_serial_set_iobase(s->superio.serial[0], (s->regs[0xe7] & 0xfe) << 2);
+ isa_serial_set_iobase(s->superio.serial[1], (s->regs[0xe8] & 0xfe) << 2);
+}
I wonder if some code duplication could be saved by adding a shared
via_superio_update() for this further up in the abstract via-superio class
instead of this method and vt8231_superio_update() below. This common method in
abstract class would need to handle the differences which seem to be reg
addresses offset by 0x10 and VT8231 having only 1 serial port. These could
either be handled by adding function parameters or fields to ViaSuperIOState
for this that the subclasses can set and the method check. (Such as reg
base=0xe2 for vt82c686 and 0xf2 for vt8231 and num_serial or similar for how
many ports are there then can have a for loop for those that would only run
once for vt8231).
Only the enable bits and the parallel port base address line up, the serial
port(s) and the floppy would need special treatment. Not worth it IMO.
Missed this part in previous reply. The serial ports would be taken care of by
a loop for number of ports so only the floppy needs an if which could also use
the number of serial ports for lack of better way to distinguish these cips
easily. Number of ports are in the superio class which you could get with
ISA_SUPERIO_GET_CLASS (see via_superio_realize) then serial.count would be 2
for 686b and 1 for 8231.
I'm not very convinced about telling the device models apart by their number of
sub devices. So let's omit this part for now.
But now I think another way may be better that is to drop the
superio_update function as this updates all functions on writing any
register unnecessarily and put the lines from it in the
vt*_superio_cfg_write() functions under the separate cases. This was
the original intent, that's why the reset function goes through that
write function so it can enable/disable functions. That way you could
apply mask on write so via_superio_cfg_read() would return 0 bits as 0
(although the data sheet is not clear about what real chip does, just
says these must be 0 not that it's enforced but if we enforce that it's
probably better to return the effective value on read as well). Then
when state saving is added in separate patch you can have a similar
function as vt82c686b_superio_reset() (or rename that to update and
make it use regs[xx] instead of constant values and call that from
reset after setting regs values like you did here. But that needs more
thought as the vmstate added by this patch is incomplete and would not
work so you could just drop it for now and add it later with also
adding other necessary state as well. The idea was to implement the
chip first then add state saving so we don't need to bother with
breaking it until we have a good enough implementation. So far the
state saving there is just left over from the old model which never
worked and only left there for reminder but only wanted to fix once the
model is complete enough.
Indeed, the patch obviously does too much if it misses details in vmstate.
Let's omit vmstate handling for now and go with your suggestion.
Any other comments from your side before the next iteration?
Nothing else from me unless Philippe has something to add. You can keep a
common function in the abstract via superclass for handling the enable
bits in the function select register to reduce code duplication as those
match and handle the address setting separately.
So I think for now you could drop vmstate stuff and distribute the
superio_update lines in the superio_cfg_write functions so each reg
only controls the function it should control. Then when vmstate saving
is added later it could reuse superio_reset as an update function
adding a new reset func setting reg values and calling the old
reset/new update function. Does that make sense?
What I don't like about the vt*_superio_cfg_write() being called during
reset is the trace logs they produce. They are hard to tell apart from
guests poking these registers, especially during reboot. So I wonder if
this could be addressed when implementing vmstate handling as you
suggest. Not too big of a deal, though.
An easy way around that is to start qemu with -S then these setup logs
come before qemu stops then logs from guest actions will be printed after
continue|c in monitor.
Regards,
BALATON Zoltan
