Re: [PATCH v2 11/12] hw/vmapple/virtio-blk: Add support for apple virtio-blk

[Philippe Mathieu-Daudé]

On 31/8/23 22:03, Stefan Hajnoczi wrote:
> On Wed, Aug 30, 2023 at 04:14:24PM +0000, Alexander Graf wrote:
> > Apple has its own virtio-blk PCI device ID where it deviates from the
> > official virtio-pci spec slightly: It puts a new "apple type"
> > field at a static offset in config space and introduces a new barrier
> > command.
> >
> > This patch first creates a mechanism for virtio-blk downstream classes to
> > handle unknown commands. It then creates such a downstream class and a new
> > vmapple-virtio-blk-pci class which support the additional apple type config
> > identifier as well as the barrier command.
> >
> > It then exposes 2 subclasses from that that we can use to expose root and
> > aux virtio-blk devices: "vmapple-virtio-root" and "vmapple-virtio-aux".
> >
> > Signed-off-by: Alexander Graf <graf@amazon.com>
>
> Aside from my comments below:
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> > ---
> >
> > v1 -> v2:
> >
> >    - Rework to make all vmapple virtio-blk logic a subclass
> > ---
> >   include/hw/pci/pci_ids.h        |   1 +
> >   include/hw/virtio/virtio-blk.h  |  12 +-
> >   include/hw/vmapple/virtio-blk.h |  39 ++++++
> >   hw/block/virtio-blk.c           |  19 ++-
> >   hw/vmapple/virtio-blk.c         | 212 ++++++++++++++++++++++++++++++++
> >   hw/vmapple/Kconfig              |   3 +
> >   hw/vmapple/meson.build          |   1 +
> >   7 files changed, 282 insertions(+), 5 deletions(-)
> >   create mode 100644 include/hw/vmapple/virtio-blk.h
> >   create mode 100644 hw/vmapple/virtio-blk.c

> > diff --git a/include/hw/vmapple/virtio-blk.h b/include/hw/vmapple/virtio-blk.h
> > new file mode 100644
> > index 0000000000..b23106a3df
> > --- /dev/null
> > +++ b/include/hw/vmapple/virtio-blk.h
> > @@ -0,0 +1,39 @@
> > +/*
> > + * VMApple specific VirtIO Block implementation
> > + *
> > + * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef HW_VMAPPLE_CFG_H
> > +#define HW_VMAPPLE_CFG_H
> > +
> > +#include "hw/sysbus.h"
>
> I'm surprised to see this header since this isn't a SysBus device. Is it
> really needed?
>
> > +#include "qom/object.h"
> > +#include "hw/virtio/virtio-pci.h"

"virtio-pci.h" surprised me too.

> > +#include "hw/virtio/virtio-blk.h"
> > +
> > +#define TYPE_VMAPPLE_VIRTIO_BLK "vmapple-virtio-blk"
> > +#define TYPE_VMAPPLE_VIRTIO_ROOT "vmapple-virtio-root"
> > +#define TYPE_VMAPPLE_VIRTIO_AUX "vmapple-virtio-aux"
> > +
> > +OBJECT_DECLARE_TYPE(VMAppleVirtIOBlk, VMAppleVirtIOBlkClass, 
> > VMAPPLE_VIRTIO_BLK)
> > +
> > +typedef struct VMAppleVirtIOBlkClass {
> > +    /*< private >*/
> > +    VirtIOBlkClass parent;
> > +    /*< public >*/
> > +    void (*get_config)(VirtIODevice *vdev, uint8_t *config);
> > +} VMAppleVirtIOBlkClass;
> > +
> > +typedef struct VMAppleVirtIOBlk {
> > +    /* <private> */
> > +    VirtIOBlock parent_obj;
> > +
> > +    /* <public> */
> > +    uint32_t apple_type;
> > +} VMAppleVirtIOBlk;
> > +
> > +#endif /* HW_VMAPPLE_CFG_H */