Re: [PATCH v2 1/2] hw/ide: split bmdma read and write functions from piix.c

[Liav Albani]

On 2/19/22 02:12, BALATON Zoltan wrote:
> On Fri, 18 Feb 2022, Liav Albani wrote:
> > This is a preparation before implementing another PCI IDE controller
> > that relies on these functions, so these can be shared between both
> > implementations.
> >
> > Signed-off-by: Liav Albani <liavalb@gmail.com>
> > ---
> > hw/ide/bmdma.c         | 84 ++++++++++++++++++++++++++++++++++++++++++
> > hw/ide/meson.build     |  2 +-
> > hw/ide/piix.c          | 51 ++-----------------------
> > include/hw/ide/bmdma.h | 34 +++++++++++++++++
> > 4 files changed, 122 insertions(+), 49 deletions(-)
> > create mode 100644 hw/ide/bmdma.c
> > create mode 100644 include/hw/ide/bmdma.h
> >
> > diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
> > new file mode 100644
> > index 0000000000..d12ed730dd
> > --- /dev/null
> > +++ b/hw/ide/bmdma.c
> > @@ -0,0 +1,84 @@
> > +/*
> > + * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 
> > and ICH6/7.
> > + *
> > + * 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/bmdma.h"
> > +#include "hw/ide/pci.h"
> > +#include "trace.h"
>
> Are you sure you need all these includes? I haven't checked but I 
> think there may be some unneeded ones here. On the other hand I'm not 
> sure putting these in a new file is worth it. There are already some 
> bmdma_* functions in hw/ide/pci.c which are declared in 
> include/hw/ide/pci.h so you could just move these there too then no 
> new file, new header nor changes to meson.build is needed in this patch..
Good question, probably not. I'll try to build without them, adding only 
what seems necessary to complete the build. Also, I think adding those 
functions to hw/ide/pci.c is a very good idea as it will make the patch 
smaller and it also makes total sense to me - Bus Master capabilities 
only appear on PCI IDE controllers and not on the ISA IDE controller 
(AFAIK).

It will happen in v3 :)

> > +
> > +uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>
> As I said before these aren't intel specific as the same functions 
> also appear in other ide controllers such as via.c so maybe a better 
> name would be bmdma_default_read and bmdma_default_write or somehting 
> similar so these can be also reused by other non-intel devices too.
Right, I see now that via.c uses the exact same functions... I'll change 
it in v3. The names bmdma_default_read and bmdma_default_write seem like 
a good choice to me.
> Regards,
> BALATON Zoltan
Thanks for the review!