Hello BALATON,
Thank you for helping keeping this patch noticeable to everyone :)
I tried to reach out to John via a private email last Saturday (two days
ago) so I don't "spam" the mailing list for no good reason.
It might be that I should actually refrain from doing so and talk to the
maintainer directly on the mailing list once the patch
has been submitted to the mailing list.
I've not yet seen any response from John so I assume it's a matter of
days before he can take care of this.
Best regards,
Liav
On 2/14/22 14:26, BALATON Zoltan wrote:
On Sat, 5 Feb 2022, BALATON Zoltan wrote:
Hello,
Ping? John, do you agree with my comments? Should Liav proceed to send
a v2?
Thanks,
BALATON Zoltan
On Sat, 5 Feb 2022, Liav Albani wrote:
On 2/5/22 17:48, BALATON Zoltan wrote:
On Sat, 5 Feb 2022, Liav Albani wrote:
This type of IDE controller has support for relocating the IO
ports and
doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the
controller.
I haven't looked at in detail so only a few comments I've got while
reading it. What machine needs this? In QEMU I think we only have
piix and ich9 emulated for pc and q35 machines but maybe ich6 is
also used by some machine I don't know about. Otherwise it looks
odd to have ide part of ich6 but not the other parts of this chip.
Hi BALATON,
This is my first patch to QEMU and the first time I send patches
over the mail. I sent my github tree to John Snow (the maintainer of
the IDE code in QEMU) for advice if I should send them here and I
was encouraged to do that.
Welcome and thanks a lot for taking time to contribute and share your
results. In case you're not yet aware, these docs should explain how
patches are handled on the list:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html
For the next time patch I'll put a note on writing a descriptive
cover letter as it could have put more valuable details on why I
sent this patch.
There's no such machine type emulating the ICH6 chipset in QEMU.
However, I wrote this emulation component as a test for the
SerenityOS kernel because I have a machine from 2009 which has
an ICH7 southbridge, so, I wanted to emulate such device with QEMU
to ease development on it.
I found out that Linux with libata was using the controller without
any noticeable problems, but the SerenityOS kernel struggled to use
this device, so I decided that
I should send this patch to get it merged and then I can use it
locally and maybe other people will benefit from it.
In regard to other components of the ICH6 chipset - I don't think
it's worth anybody's time to actually implement them as the ICH9
chipset is quite close to what the ICH6 chipset offers as far as I
can tell.
The idea of implementing ich6-ide controller was to enable the
option of people like me and other OS developers to ensure their
kernels operate correctly on such type of device,
which is legacy-free device in the aspect of PCI bus resource
management but still is a legacy device which belongs to chipsets of
late 2000s.
That's OK, maybe a short mention (just one sentence) in the commit
message explaining this would help to understand why this device
model was added.
Signed-off-by: Liav Albani <liav...@gmail.com>
---
hw/i386/Kconfig | 2 +
hw/ide/Kconfig | 5 +
hw/ide/bmdma.c | 83 +++++++++++++++
hw/ide/ich6.c | 211
+++++++++++++++++++++++++++++++++++++++
hw/ide/meson.build | 3 +-
hw/ide/piix.c | 50 +---------
include/hw/ide/pci.h | 5 +
include/hw/pci/pci_ids.h | 1 +
8 files changed, 311 insertions(+), 49 deletions(-)
create mode 100644 hw/ide/bmdma.c
create mode 100644 hw/ide/ich6.c
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..a18de2d962 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -75,6 +75,7 @@ config I440FX
select PCI_I440FX
select PIIX3
select IDE_PIIX
+ select IDE_ICH6
select DIMM
select SMBIOS
select FW_CFG_DMA
@@ -101,6 +102,7 @@ config Q35
select PCI_EXPRESS_Q35
select LPC_ICH9
select AHCI_ICH9
+ select IDE_ICH6
select DIMM
select SMBIOS
select FW_CFG_DMA
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index dd85fa3619..63304325a5 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -38,6 +38,11 @@ config IDE_VIA
select IDE_PCI
select IDE_QDEV
+config IDE_ICH6
+ bool
+ select IDE_PCI
+ select IDE_QDEV
+
config MICRODRIVE
bool
select IDE_QDEV
diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
new file mode 100644
index 0000000000..979f5974fd
--- /dev/null
+++ b/hw/ide/bmdma.c
@@ -0,0 +1,83 @@
+/*
+ * QEMU IDE Emulation: PCI PIIX3/4 support.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2006 Openedhand Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person
obtaining a copy
+ * of this software and associated documentation files (the
"Software"), to deal
+ * in the Software without restriction, including without
limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense,
and/or sell
+ * copies of the Software, and to permit persons to whom the
Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+
+#include "hw/ide/pci.h"
+#include "trace.h"
+
+uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
Moving these functions to avoid duplication is a good idea but a
couple of points:
- Maybe this should be a separate patch just for moving these out
then another patch adding ich6 for easier review of separate changes.
I don't mind splitting this patch into a couple of "commits" (or
series of diffs? I don't understand the terminology yet) so one
"commit" will be a preparation - extract the
functions to a separate file, and the next diff can implement the
ich6-ide controller.
See the docs, which should explain how to split up patches and what
should be in one patch. This would be a patch series then. As to
where to move these functions or how to proceed with it wait for John
Snow's reply as he's the maintainer of this part so what he prefers
is the way to go. What I wrote is my opinion but others may have
different views so give them a chance to reply too then once we see
what to do you can send a v2 with the changes that seems to be the
consensus on how to best proceed.
- There are already several bmdma_* functions in pci.c and you add
these to pci.h so maybe these should be moved near there in pci.c
instead. (Or more of those bmdma_* functions moved in this new file
and add its own header?)
I'm not sure what is the best approach but maybe the latter
suggestion (new file and its own header) seems quite good to me.
- This is not piix specific so the name probably should not say
that. Maybe somthing along the lines of pci_default_read_config so
bmdma_default_read or similar. In fact it also appears in via.c and
a more complex version in cmd646.c which could still reuse
functions like these like we do with pci config write. Converting
those other two to use the newly split off functions instead of
duplicating it could be done in a follow up patch, if you don't
want to do that I may look at via-ide but a patch is welcome if you
have time for that, unless others think otherwise and we'll take a
different route.
I want to create the best possible patch so if it's desirable to
rename it to something more generic, that's OK for me.
I'd like to address some of the issues you mentioned (such as
avoiding duplicates in how we use bus master DMA in the ide code) in
future patches though, so if possible, let's keep it simple now :)
Sure, this could be done afterwards too just noted it when noticed
there are other places that use similar functions so this is a
possible clean up that should be easy to do either as an additional
patch in this series or a separate one later.
Regards,
BALATON Zoltan
+static uint32_t ich6_pci_config_read(PCIDevice *d,
+ uint32_t address, int len)
+{
+ return pci_default_read_config(d, address, len);
+}
Why do you override this if you have nothing to do in it? Just use
pci_default_read_config and only override ich6_pci_config_write
where you actually has something to add to the default.
You're right, I should not override this function. It was probably
me trying (I uploaded this patch to GitHub about 2 weeks ago so I
don't remember precisely) to override it for the sake of returning a
value of 0x8000 when the guest OS tries to figure out if the IDE
channel is disabled or not in the 0x40 and 0x42 registers in the PCI
config space of the device.
Maybe also wait for a few days for other's comments (especially the
maintainer's opinion on this) before sending a v2 so you get all
comments and see what to do.
Thank you very much for putting time into reviewing this! :)
Best regards,
Liav
