Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Laurent Vivier]

Le 09/09/2019 à 18:11, Peter Maydell a écrit :
> On Mon, 9 Sep 2019 at 17:05, Laurent Vivier  wrote:
>>
>> Co-developed-by: Mark Cave-Ayland 
>> Signed-off-by: Mark Cave-Ayland 
>> Signed-off-by: Laurent Vivier 
>> Reviewed-by: Hervé Poussineau 
> 
>> +static void sysbus_swim_class_init(ObjectClass *oc, void *data)
>> +{
>> +DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +dc->realize = sysbus_swim_realize;
>> +}
> 
> Missing reset and VMState for migration. These should be
> baseline requirements for adding new device models to the
> tree, because in an ideal world every device would support
> both -- we should be gradually fixing the existing devices
> which are missing these, and not letting new devices in,
> so the situation gets gradually better, not worse.

OK, I'm going to add reset and VMState.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Aleksandar Markovic]

09.09.2019. 18.32, "Aleksandar Markovic"  је
написао/ла:
>
>
> 09.09.2019. 18.03, "Laurent Vivier"  је написао/ла:
> >
> > Co-developed-by: Mark Cave-Ayland 
> > Signed-off-by: Mark Cave-Ayland 
> > Signed-off-by: Laurent Vivier 
> > Reviewed-by: Hervé Poussineau 
> > ---
>
> Laurent, hi!
>
> I am not sure how "Co-developed-by:" fits in our workflow. There was some
recent talk on restricting those marks to only a handful of them, and
preventing new ones from introducing (the starter example was something
like "Regression-tested-by:"). Perhaps a final sentence "This patch was
co-developed with Mark..." would be better?
>
> Aleksandar

Laurent,

I noticed I overlooked that there is a paragraph on "Co-developed-by:" in
the Linux kernel "how to submit a patch", so it is fine.

Sorry about noise.

Aleksandar

> >  hw/block/Kconfig|   3 +
> >  hw/block/Makefile.objs  |   1 +
> >  hw/block/swim.c | 421 
> >  include/hw/block/swim.h |  76 
> >  4 files changed, 501 insertions(+)
> >  create mode 100644 hw/block/swim.c
> >  create mode 100644 include/hw/block/swim.h
> >
> > diff --git a/hw/block/Kconfig b/hw/block/Kconfig
> > index df96dc5dcc..2d17f481ad 100644
> > --- a/hw/block/Kconfig
> > +++ b/hw/block/Kconfig
> > @@ -37,3 +37,6 @@ config VHOST_USER_BLK
> >  # Only PCI devices are provided for now
> >  default y if VIRTIO_PCI
> >  depends on VIRTIO && VHOST_USER && LINUX
> > +
> > +config SWIM
> > +bool
> > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> > index f5f643f0cc..28c2495a00 100644
> > --- a/hw/block/Makefile.objs
> > +++ b/hw/block/Makefile.objs
> > @@ -8,6 +8,7 @@ common-obj-$(CONFIG_XEN) += xen-block.o
> >  common-obj-$(CONFIG_ECC) += ecc.o
> >  common-obj-$(CONFIG_ONENAND) += onenand.o
> >  common-obj-$(CONFIG_NVME_PCI) += nvme.o
> > +common-obj-$(CONFIG_SWIM) += swim.o
> >
> >  obj-$(CONFIG_SH4) += tc58128.o
> >
> > diff --git a/hw/block/swim.c b/hw/block/swim.c
> > new file mode 100644
> > index 00..6e26915238
> > --- /dev/null
> > +++ b/hw/block/swim.c
> > @@ -0,0 +1,421 @@
> > +/*
> > + * QEMU Macintosh floppy disk controller emulator (SWIM)
> > + *
> > + * Copyright (c) 2014-2018 Laurent Vivier 
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.
See
> > + * the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> > +#include "qapi/error.h"
> > +#include "sysemu/block-backend.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/block/block.h"
> > +#include "hw/block/swim.h"
> > +#include "hw/qdev-properties.h"
> > +
> > +/* IWM registers */
> > +
> > +#define IWM_PH0L0
> > +#define IWM_PH0H1
> > +#define IWM_PH1L2
> > +#define IWM_PH1H3
> > +#define IWM_PH2L4
> > +#define IWM_PH2H5
> > +#define IWM_PH3L6
> > +#define IWM_PH3H7
> > +#define IWM_MTROFF  8
> > +#define IWM_MTRON   9
> > +#define IWM_INTDRIVE10
> > +#define IWM_EXTDRIVE11
> > +#define IWM_Q6L 12
> > +#define IWM_Q6H 13
> > +#define IWM_Q7L 14
> > +#define IWM_Q7H 15
> > +
> > +/* SWIM registers */
> > +
> > +#define SWIM_WRITE_DATA 0
> > +#define SWIM_WRITE_MARK 1
> > +#define SWIM_WRITE_CRC  2
> > +#define SWIM_WRITE_PARAMETER3
> > +#define SWIM_WRITE_PHASE4
> > +#define SWIM_WRITE_SETUP5
> > +#define SWIM_WRITE_MODE06
> > +#define SWIM_WRITE_MODE17
> > +
> > +#define SWIM_READ_DATA  8
> > +#define SWIM_READ_MARK  9
> > +#define SWIM_READ_ERROR 10
> > +#define SWIM_READ_PARAMETER 11
> > +#define SWIM_READ_PHASE 12
> > +#define SWIM_READ_SETUP 13
> > +#define SWIM_READ_STATUS14
> > +#define SWIM_READ_HANDSHAKE 15
> > +
> > +#define REG_SHIFT   9
> > +
> > +#define SWIM_MODE_IWM  0
> > +#define SWIM_MODE_SWIM 1
> > +
> > +/* bits in phase register */
> > +
> > +#define SWIM_SEEK_NEGATIVE   0x074
> > +#define SWIM_STEP0x071
> > +#define SWIM_MOTOR_ON0x072
> > +#define SWIM_MOTOR_OFF   0x076
> > +#define SWIM_INDEX   0x073
> > +#define SWIM_EJECT   0x077
> > +#define SWIM_SETMFM  0x171
> > +#define SWIM_SETGCR  0x175
> > +#define SWIM_RELAX   0x033
> > +#define SWIM_LSTRB   0x008
> > +#define SWIM_CA_MASK 0x077
> > +
> > +/* Select values for swim_select and swim_readbit */
> > +
> > +#define SWIM_READ_DATA_0 0x074
> > +#define SWIM_TWOMEG_DRIVE0x075
> > +#define SWIM_SINGLE_SIDED0x076
> > +#define SWIM_DRIVE_PRESENT   0x077
> > +#define SWIM_DISK_IN 0x170
> > +#define SWIM_WRITE_PROT  0x171
> > +#define SWIM_TRACK_ZERO  0x172
> > +#define SWIM_TACHO   0x173

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Laurent Vivier]

Le 09/09/2019 à 18:32, Aleksandar Markovic a écrit :
> 
> 09.09.2019. 18.03, "Laurent Vivier"  > је написао/ла:
>>
>> Co-developed-by: Mark Cave-Ayland  >
>> Signed-off-by: Mark Cave-Ayland  >
>> Signed-off-by: Laurent Vivier  >
>> Reviewed-by: Hervé Poussineau  >
>> ---
> 
> Laurent, hi!
> 
> I am not sure how "Co-developed-by:" fits in our workflow. There was
> some recent talk on restricting those marks to only a handful of them,
> and preventing new ones from introducing (the starter example was
> something like "Regression-tested-by:"). Perhaps a final sentence "This
> patch was co-developed with Mark..." would be better?
> 

"Co-developed-by:" is described in
linux/Documentation/process/submitting-patches.rst [1] whereas
"Regression-tested-by:" is not.

So as I'm not aware of the discussions you point out, are we in the same
situation here?

Thanks,
Laurent

[1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Laurent Vivier]

Le 09/09/2019 à 18:18, Dr. David Alan Gilbert a écrit :
> * Peter Maydell (peter.mayd...@linaro.org) wrote:
>> On Mon, 9 Sep 2019 at 17:05, Laurent Vivier  wrote:
>>>
>>> Co-developed-by: Mark Cave-Ayland 
>>> Signed-off-by: Mark Cave-Ayland 
>>> Signed-off-by: Laurent Vivier 
>>> Reviewed-by: Hervé Poussineau 
>>
>>> +static void sysbus_swim_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +DeviceClass *dc = DEVICE_CLASS(oc);
>>> +
>>> +dc->realize = sysbus_swim_realize;
>>> +}
>>
>> Missing reset and VMState for migration. These should be
>> baseline requirements for adding new device models to the
>> tree, because in an ideal world every device would support
>> both -- we should be gradually fixing the existing devices
>> which are missing these, and not letting new devices in,
>> so the situation gets gradually better, not worse.
> 
> I'm happy to see things marked unmigratable instead for obscure
> devices; I don't see the need to worry about migration for stuff
> that's not going to be migrated.
> 

I think in this case it's better to mark it unmigratable because the
structure will change in the future to really manage floppy and it
avoids to introduces a new version of the migration data stream.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Aleksandar Markovic]

09.09.2019. 18.03, "Laurent Vivier"  је написао/ла:
>
> Co-developed-by: Mark Cave-Ayland 
> Signed-off-by: Mark Cave-Ayland 
> Signed-off-by: Laurent Vivier 
> Reviewed-by: Hervé Poussineau 
> ---

Laurent, hi!

I am not sure how "Co-developed-by:" fits in our workflow. There was some
recent talk on restricting those marks to only a handful of them, and
preventing new ones from introducing (the starter example was something
like "Regression-tested-by:"). Perhaps a final sentence "This patch was
co-developed with Mark..." would be better?

Aleksandar

>  hw/block/Kconfig|   3 +
>  hw/block/Makefile.objs  |   1 +
>  hw/block/swim.c | 421 
>  include/hw/block/swim.h |  76 
>  4 files changed, 501 insertions(+)
>  create mode 100644 hw/block/swim.c
>  create mode 100644 include/hw/block/swim.h
>
> diff --git a/hw/block/Kconfig b/hw/block/Kconfig
> index df96dc5dcc..2d17f481ad 100644
> --- a/hw/block/Kconfig
> +++ b/hw/block/Kconfig
> @@ -37,3 +37,6 @@ config VHOST_USER_BLK
>  # Only PCI devices are provided for now
>  default y if VIRTIO_PCI
>  depends on VIRTIO && VHOST_USER && LINUX
> +
> +config SWIM
> +bool
> diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> index f5f643f0cc..28c2495a00 100644
> --- a/hw/block/Makefile.objs
> +++ b/hw/block/Makefile.objs
> @@ -8,6 +8,7 @@ common-obj-$(CONFIG_XEN) += xen-block.o
>  common-obj-$(CONFIG_ECC) += ecc.o
>  common-obj-$(CONFIG_ONENAND) += onenand.o
>  common-obj-$(CONFIG_NVME_PCI) += nvme.o
> +common-obj-$(CONFIG_SWIM) += swim.o
>
>  obj-$(CONFIG_SH4) += tc58128.o
>
> diff --git a/hw/block/swim.c b/hw/block/swim.c
> new file mode 100644
> index 00..6e26915238
> --- /dev/null
> +++ b/hw/block/swim.c
> @@ -0,0 +1,421 @@
> +/*
> + * QEMU Macintosh floppy disk controller emulator (SWIM)
> + *
> + * Copyright (c) 2014-2018 Laurent Vivier 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "qapi/error.h"
> +#include "sysemu/block-backend.h"
> +#include "hw/sysbus.h"
> +#include "hw/block/block.h"
> +#include "hw/block/swim.h"
> +#include "hw/qdev-properties.h"
> +
> +/* IWM registers */
> +
> +#define IWM_PH0L0
> +#define IWM_PH0H1
> +#define IWM_PH1L2
> +#define IWM_PH1H3
> +#define IWM_PH2L4
> +#define IWM_PH2H5
> +#define IWM_PH3L6
> +#define IWM_PH3H7
> +#define IWM_MTROFF  8
> +#define IWM_MTRON   9
> +#define IWM_INTDRIVE10
> +#define IWM_EXTDRIVE11
> +#define IWM_Q6L 12
> +#define IWM_Q6H 13
> +#define IWM_Q7L 14
> +#define IWM_Q7H 15
> +
> +/* SWIM registers */
> +
> +#define SWIM_WRITE_DATA 0
> +#define SWIM_WRITE_MARK 1
> +#define SWIM_WRITE_CRC  2
> +#define SWIM_WRITE_PARAMETER3
> +#define SWIM_WRITE_PHASE4
> +#define SWIM_WRITE_SETUP5
> +#define SWIM_WRITE_MODE06
> +#define SWIM_WRITE_MODE17
> +
> +#define SWIM_READ_DATA  8
> +#define SWIM_READ_MARK  9
> +#define SWIM_READ_ERROR 10
> +#define SWIM_READ_PARAMETER 11
> +#define SWIM_READ_PHASE 12
> +#define SWIM_READ_SETUP 13
> +#define SWIM_READ_STATUS14
> +#define SWIM_READ_HANDSHAKE 15
> +
> +#define REG_SHIFT   9
> +
> +#define SWIM_MODE_IWM  0
> +#define SWIM_MODE_SWIM 1
> +
> +/* bits in phase register */
> +
> +#define SWIM_SEEK_NEGATIVE   0x074
> +#define SWIM_STEP0x071
> +#define SWIM_MOTOR_ON0x072
> +#define SWIM_MOTOR_OFF   0x076
> +#define SWIM_INDEX   0x073
> +#define SWIM_EJECT   0x077
> +#define SWIM_SETMFM  0x171
> +#define SWIM_SETGCR  0x175
> +#define SWIM_RELAX   0x033
> +#define SWIM_LSTRB   0x008
> +#define SWIM_CA_MASK 0x077
> +
> +/* Select values for swim_select and swim_readbit */
> +
> +#define SWIM_READ_DATA_0 0x074
> +#define SWIM_TWOMEG_DRIVE0x075
> +#define SWIM_SINGLE_SIDED0x076
> +#define SWIM_DRIVE_PRESENT   0x077
> +#define SWIM_DISK_IN 0x170
> +#define SWIM_WRITE_PROT  0x171
> +#define SWIM_TRACK_ZERO  0x172
> +#define SWIM_TACHO   0x173
> +#define SWIM_READ_DATA_1 0x174
> +#define SWIM_MFM_MODE0x175
> +#define SWIM_SEEK_COMPLETE   0x176
> +#define SWIM_ONEMEG_MEDIA0x177
> +
> +/* Bits in handshake register */
> +
> +#define SWIM_MARK_BYTE   0x01
> +#define SWIM_CRC_ZERO0x02
> +#define SWIM_RDDATA  0x04
> +#define SWIM_SENSE   0x08
> +#define SWIM_MOTEN   0x10
> +#define SWIM_ERROR   0x20
> +#define SWIM_DAT2BYTE0x40
> +#define SWIM_DAT1BYTE0x80
> +
> +/* 

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Peter Maydell]

On Mon, 9 Sep 2019 at 17:18, Dr. David Alan Gilbert  wrote:
>
> * Peter Maydell (peter.mayd...@linaro.org) wrote:
> > Missing reset and VMState for migration. These should be
> > baseline requirements for adding new device models to the
> > tree, because in an ideal world every device would support
> > both -- we should be gradually fixing the existing devices
> > which are missing these, and not letting new devices in,
> > so the situation gets gradually better, not worse.
>
> I'm happy to see things marked unmigratable instead for obscure
> devices; I don't see the need to worry about migration for stuff
> that's not going to be migrated.

VM snapshot save/restore is a useful debug tool. But yes,
"mark as nonmigratable explicitly" is OK too -- at least
that way if the user tries it they get a clear error message,
and we can later on look through to find what devices
are missing the capability. (Personally I find it's just
as easy to just implement the vmstate struct, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On Mon, 9 Sep 2019 at 17:05, Laurent Vivier  wrote:
> >
> > Co-developed-by: Mark Cave-Ayland 
> > Signed-off-by: Mark Cave-Ayland 
> > Signed-off-by: Laurent Vivier 
> > Reviewed-by: Hervé Poussineau 
> 
> > +static void sysbus_swim_class_init(ObjectClass *oc, void *data)
> > +{
> > +DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +dc->realize = sysbus_swim_realize;
> > +}
> 
> Missing reset and VMState for migration. These should be
> baseline requirements for adding new device models to the
> tree, because in an ideal world every device would support
> both -- we should be gradually fixing the existing devices
> which are missing these, and not letting new devices in,
> so the situation gets gradually better, not worse.

I'm happy to see things marked unmigratable instead for obscure
devices; I don't see the need to worry about migration for stuff
that's not going to be migrated.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v9 8/9] hw/m68k: add a dummy SWIM floppy controller

[Peter Maydell]

On Mon, 9 Sep 2019 at 17:05, Laurent Vivier  wrote:
>
> Co-developed-by: Mark Cave-Ayland 
> Signed-off-by: Mark Cave-Ayland 
> Signed-off-by: Laurent Vivier 
> Reviewed-by: Hervé Poussineau 

> +static void sysbus_swim_class_init(ObjectClass *oc, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +dc->realize = sysbus_swim_realize;
> +}

Missing reset and VMState for migration. These should be
baseline requirements for adding new device models to the
tree, because in an ideal world every device would support
both -- we should be gradually fixing the existing devices
which are missing these, and not letting new devices in,
so the situation gets gradually better, not worse.

thanks
-- PMM