Re: [PATCH v3 1/6] vt82c686: Implement control of serial port io ranges via config regs - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: BALATON Zoltan <balaton@eik.bme.hu>,
	David Gibson <david@gibson.dropbear.id.au>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Huacai Chen <chenhuacai@kernel.org>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3 1/6] vt82c686: Implement control of serial port io ranges via config regs
Date: Tue, 23 Feb 2021 10:27:17 +0100	[thread overview]
Message-ID: <f9f7e196-45b3-32f8-2455-0f4f52645e6e@amsat.org> (raw)
In-Reply-To: <845e85da-aa5e-4ac4-7449-7026eaf3b350@eik.bme.hu>

On 2/23/21 10:18 AM, BALATON Zoltan wrote:
> On Tue, 23 Feb 2021, David Gibson wrote:
>> On Mon, Feb 22, 2021 at 04:22:06PM +0100, BALATON Zoltan wrote:
>>> In VIA super south bridge the io ranges of superio components
>>> (parallel and serial ports and FDC) can be controlled by superio
>>> config registers to set their base address and enable/disable them.
>>> This is not easy to implement in QEMU because ISA emulation is only
>>> designed to set io base address once on creating the device and io
>>> ranges are registered at creation and cannot easily be disabled or
>>> moved later.
>>>
>>> In this patch we hack around that but only for serial ports because
>>> those have a single io range at port base that's relatively easy to
>>> handle and it's what guests actually use and set address different
>>> than the default.
>>>
>>> We do not attempt to handle controlling the parallel and FDC regions
>>> because those have multiple io ranges so handling them would be messy
>>> and guests either don't change their deafult or don't care. We could
>>> even get away with disabling and not emulating them, but since they
>>> are already there, this patch leaves them mapped at their default
>>> address just in case this could be useful for a guest in the future.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>
>> The maintainers of the hw/isa/vt82c686.c should probably be CCed on this.
> 
> He is (but may not be obvious because the accent in Phil's name is not
> handled well by my mail host so to avoid misspelling his name I've just
> omitted it).

We are 3 (other now Cc'ed):

$ ./scripts/get_maintainer.pl -f hw/isa/vt82c686.c
Huacai Chen <chenhuacai@kernel.org> (odd fixer:Fuloong 2E)
"Philippe Mathieu-Daudé" <f4bug@amsat.org> (odd fixer:Fuloong 2E)
Jiaxun Yang <jiaxun.yang@flygoat.com> (reviewer:Fuloong 2E)

> And he also said these should go in via some other tree:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06551.html
> 
> He may still review or ack these but I guess he'd need some time.

I took a bunch of Zoltan's patches:

$ git log --oneline e551455f1e7..00d8ba9e0d6 hw/isa/vt82c686.c
cc2b4550115 vt82c686: Fix superio_cfg_{read,write}() functions
2c4c556e061 vt82c686: Log superio_cfg unimplemented accesses
b7741b77425 vt82c686: Simplify by returning earlier
2b98dca9571 vt82c686: Reduce indentation by returning early
c953bf71182 vt82c686: Remove index field of SuperIOConfig
3dc31cb8490 vt82c686: Move creation of ISA devices to the ISA bridge
9859ad1c4b6 vt82c686: Simplify vt82c686b_realize()
e1a69736e59 vt82c686: Make vt82c686b-pm an abstract base class and add
vt8231-pm based on it
084bf4b41d4 vt82c686: Set user_creatable=false for VT82C686B_PM
3ab1eea6bce vt82c686: Fix up power management io base and config
9af8e529b91 vt82c686: Correctly reset all registers to default values on
reset
40a0bba1e3f vt82c686: Correct vt82c686-pm I/O size
35e360ed674 vt82c686: Make vt82c686-pm an I/O tracing region
911629e6d37 vt82c686: Fix SMBus IO base and configuration registers
94349bffda0 vt82c686: Reorganise code
6be6e4bc769 vt82c686: Move superio memory region to SuperIOConfig struct
7886a674f13 vt82c686: Rename superio config related parts
007b3103a39 vt82c686: Use shorter name for local variable holding object
state
9b0fbae2cbf vt82c686: Remove unneeded includes and defines
ff413a1f7f6 vt82c686: Convert debug printf to trace points
dc66439542c vt82c686: Remove legacy vt82c686b_pm_init() function
0bfda9a225b vt82c686: Remove legacy vt82c686b_isa_init() function
657fae258f9 vt82c686: Split off via-[am]c97 into separate file in hw/audio
07c6832cb2c vt82c686: Remove vt82c686b_[am]c97_init() functions
0f79846147f vt82c686: Rename VT82C686B to VT82C686B_ISA
e6340505441 vt82c686: Remove unnecessary _DEVICE suffix from type macros
5a4856ed78e vt82c686: Rename AC97/MC97 parts from VT82C686B to VIA

But with this one I raised a question for find_subregion() ISA use,
and deferred to Paolo for this particular question, and to PPC for
the new VT8231 device model.

Regards,

Phil.