On Sat, 5 Oct 2024 at 07:47, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
> On 2024/09/28 17:57, Phil Dennis-Jordan wrote:
> > From: Alexander Graf <g...@amazon.com>
> >
> > 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 <g...@amazon.com>
> > Signed-off-by: Phil Dennis-Jordan <p...@philjordan.eu>
> > ---
> > hw/block/virtio-blk.c | 19 ++-
> > hw/vmapple/Kconfig | 3 +
> > hw/vmapple/meson.build | 1 +
> > hw/vmapple/virtio-blk.c | 212 ++++++++++++++++++++++++++++++++
> > include/hw/pci/pci_ids.h | 1 +
> > include/hw/virtio/virtio-blk.h | 12 +-
> > include/hw/vmapple/virtio-blk.h | 39 ++++++
> > 7 files changed, 282 insertions(+), 5 deletions(-)
> > create mode 100644 hw/vmapple/virtio-blk.c
> > create mode 100644 include/hw/vmapple/virtio-blk.h
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 115795392c4..cecc4cef9e4 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -50,12 +50,12 @@ static void virtio_blk_init_request(VirtIOBlock *s,
> VirtQueue *vq,
> > req->mr_next = NULL;
> > }
> >
> > -static void virtio_blk_free_request(VirtIOBlockReq *req)
> > +void virtio_blk_free_request(VirtIOBlockReq *req)
> > {
> > g_free(req);
> > }
>
> This function is identical with g_free(). Perhaps it's better to remove
> it instead of updating it.
>
I'm not sure that's something I should be doing in such a tangential patch
series, and it's very much a matter of taste - other operations on
VirtIOBlockReq have similar, consistent naming, and this function seemingly
hasn't been touched for 7 years.
Perhaps virtio-blk maintainer Stefan Hajnoczi could weigh in? (tagged in
To:)
> > -static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char
> status)
> > +void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
> > {
> > VirtIOBlock *s = req->dev;
> > VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > @@ -966,8 +966,18 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> > break;
> > }
> > default:
> > - virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> > - virtio_blk_free_request(req);
> > + {
> > + /*
> > + * Give subclasses a chance to handle unknown requests. This
> way the
> > + * class lookup is not in the hot path.
> > + */
> > + VirtIOBlkClass *vbk = VIRTIO_BLK_GET_CLASS(s);
> > + if (!vbk->handle_unknown_request ||
> > + !vbk->handle_unknown_request(req, mrb, type)) {
> > + virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> > + virtio_blk_free_request(req);
> > + }
> > + }
> > }
> > return 0;
> > }
> > @@ -2044,6 +2054,7 @@ static const TypeInfo virtio_blk_info = {
> > .instance_size = sizeof(VirtIOBlock),
> > .instance_init = virtio_blk_instance_init,
> > .class_init = virtio_blk_class_init,
> > + .class_size = sizeof(VirtIOBlkClass),
> > };
> >
> > static void virtio_register_types(void)
> > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
> > index 8bbeb9a9237..bcd1be63e3c 100644
> > --- a/hw/vmapple/Kconfig
> > +++ b/hw/vmapple/Kconfig
> > @@ -7,3 +7,6 @@ config VMAPPLE_BDIF
> > config VMAPPLE_CFG
> > bool
> >
> > +config VMAPPLE_VIRTIO_BLK
> > + bool
> > +
> > diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build
> > index 64b78693a31..bf17cf906c9 100644
> > --- a/hw/vmapple/meson.build
> > +++ b/hw/vmapple/meson.build
> > @@ -1,3 +1,4 @@
> > system_ss.add(when: 'CONFIG_VMAPPLE_AES', if_true: files('aes.c'))
> > system_ss.add(when: 'CONFIG_VMAPPLE_BDIF', if_true: files('bdif.c'))
> > system_ss.add(when: 'CONFIG_VMAPPLE_CFG', if_true: files('cfg.c'))
> > +system_ss.add(when: 'CONFIG_VMAPPLE_VIRTIO_BLK', if_true:
> files('virtio-blk.c'))
> > diff --git a/hw/vmapple/virtio-blk.c b/hw/vmapple/virtio-blk.c
> > new file mode 100644
> > index 00000000000..720eaa61a86
> > --- /dev/null
> > +++ b/hw/vmapple/virtio-blk.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * 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.
> > + *
> > + * VMApple uses almost standard VirtIO Block, but with a few key
> differences:
> > + *
> > + * - Different PCI device/vendor ID
> > + * - An additional "type" identifier to differentiate AUX and Root
> volumes
> > + * - An additional BARRIER command
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/vmapple/virtio-blk.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "qapi/error.h"
> > +
> > +#define VIRTIO_BLK_T_APPLE_BARRIER 0x10000
> > +
> > +#define VIRTIO_APPLE_TYPE_ROOT 1
> > +#define VIRTIO_APPLE_TYPE_AUX 2
> > +
> > +static bool vmapple_virtio_blk_handle_unknown_request(VirtIOBlockReq
> *req,
> > + MultiReqBuffer
> *mrb,
> > + uint32_t type)
> > +{
> > + switch (type) {
> > + case VIRTIO_BLK_T_APPLE_BARRIER:
> > + /* We ignore barriers for now. YOLO. */
>
> It should be LOG_UNIMP instead of a mere comment.
>
Fixed in next patch version.
> > + virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> > + virtio_blk_free_request(req);
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +/*
> > + * VMApple virtio-blk uses the same config format as normal virtio,
> with one
> > + * exception: It adds an "apple type" specififer at the same location
> that
> > + * the spec reserves for max_secure_erase_sectors. Let's hook into the
> > + * get_config code path here, run it as usual and then patch in the
> apple type.
> > + */
> > +static void vmapple_virtio_blk_get_config(VirtIODevice *vdev, uint8_t
> *config)
> > +{
> > + VMAppleVirtIOBlk *dev = VMAPPLE_VIRTIO_BLK(vdev);
> > + VMAppleVirtIOBlkClass *vvbk = VMAPPLE_VIRTIO_BLK_GET_CLASS(dev);
> > + struct virtio_blk_config *blkcfg = (struct virtio_blk_config
> *)config;
> > +
> > + vvbk->get_config(vdev, config);
> > +
> > + g_assert(dev->parent_obj.config_size >= endof(struct
> virtio_blk_config, zoned));
> > +
> > + /* Apple abuses the field for max_secure_erase_sectors as type id */
> > + blkcfg->max_secure_erase_sectors = dev->apple_type;
> > +}
> > +
> > +static Property vmapple_virtio_blk_properties[] = {
> > + DEFINE_PROP_UINT32("apple-type", VMAppleVirtIOBlk, apple_type, 0),
> > + DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void vmapple_virtio_blk_class_init(ObjectClass *klass, void
> *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + VirtIOBlkClass *vbk = VIRTIO_BLK_CLASS(klass);
> > + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> > + VMAppleVirtIOBlkClass *vvbk = VMAPPLE_VIRTIO_BLK_CLASS(klass);
> > +
> > + vbk->handle_unknown_request =
> vmapple_virtio_blk_handle_unknown_request;
> > + vvbk->get_config = vdc->get_config;
> > + vdc->get_config = vmapple_virtio_blk_get_config;
> > + device_class_set_props(dc, vmapple_virtio_blk_properties);
> > +}
> > +
> > +static const TypeInfo vmapple_virtio_blk_info = {
> > + .name = TYPE_VMAPPLE_VIRTIO_BLK,
> > + .parent = TYPE_VIRTIO_BLK,
> > + .instance_size = sizeof(VMAppleVirtIOBlk),
> > + .class_init = vmapple_virtio_blk_class_init,
> > +};
> > +
> > +/* PCI Devices */
> > +
> > +typedef struct VMAppleVirtIOBlkPCI {
> > + VirtIOPCIProxy parent_obj;
> > + VMAppleVirtIOBlk vdev;
> > + uint32_t apple_type;
> > +} VMAppleVirtIOBlkPCI;
> > +
> > +/*
> > + * vmapple-virtio-blk-pci: This extends VirtioPCIProxy.
> > + */
> > +#define TYPE_VMAPPLE_VIRTIO_BLK_PCI "vmapple-virtio-blk-pci-base"
> > +DECLARE_INSTANCE_CHECKER(VMAppleVirtIOBlkPCI, VMAPPLE_VIRTIO_BLK_PCI,
> > + TYPE_VMAPPLE_VIRTIO_BLK_PCI)
> > +
> > +static Property vmapple_virtio_blk_pci_properties[] = {
> > + DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
> > + DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> > + VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> > + DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
> > + DEV_NVECTORS_UNSPECIFIED),
> > + DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void vmapple_virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev,
> Error **errp)
> > +{
> > + VMAppleVirtIOBlkPCI *dev = VMAPPLE_VIRTIO_BLK_PCI(vpci_dev);
> > + DeviceState *vdev = DEVICE(&dev->vdev);
> > + VirtIOBlkConf *conf = &dev->vdev.parent_obj.conf;
> > +
> > + if (conf->num_queues == VIRTIO_BLK_AUTO_NUM_QUEUES) {
> > + conf->num_queues = virtio_pci_optimal_num_queues(0);
> > + }
> > +
> > + if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> > + vpci_dev->nvectors = conf->num_queues + 1;
> > + }
> > +
> > + /*
> > + * We don't support zones, but we need the additional config space
> size.
> > + * Let's just expose the feature so the rest of the virtio-blk logic
> > + * allocates enough space for us. The guest will ignore zones
> anyway.
> > + */
> > + virtio_add_feature(&dev->vdev.parent_obj.host_features,
> VIRTIO_BLK_F_ZONED);
> > + /* Propagate the apple type down to the virtio-blk device */
> > + qdev_prop_set_uint32(DEVICE(&dev->vdev), "apple-type",
> dev->apple_type);
> > + /* and spawn the virtio-blk device */
> > + qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > +
> > + /*
> > + * The virtio-pci machinery adjusts its vendor/device ID based on
> whether
> > + * we support modern or legacy virtio. Let's patch it back to the
> Apple
> > + * identifiers here.
> > + */
> > + pci_config_set_vendor_id(vpci_dev->pci_dev.config,
> PCI_VENDOR_ID_APPLE);
> > + pci_config_set_device_id(vpci_dev->pci_dev.config,
> PCI_DEVICE_ID_APPLE_VIRTIO_BLK);
> > +}
> > +
> > +static void vmapple_virtio_blk_pci_class_init(ObjectClass *klass, void
> *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> > + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
> > +
> > + set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > + device_class_set_props(dc, vmapple_virtio_blk_pci_properties);
> > + k->realize = vmapple_virtio_blk_pci_realize;
> > + pcidev_k->vendor_id = PCI_VENDOR_ID_APPLE;
> > + pcidev_k->device_id = PCI_DEVICE_ID_APPLE_VIRTIO_BLK;
> > + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> > + pcidev_k->class_id = PCI_CLASS_STORAGE_SCSI;
> > +}
> > +
> > +static void vmapple_virtio_blk_pci_instance_init(Object *obj)
> > +{
> > + VMAppleVirtIOBlkPCI *dev = VMAPPLE_VIRTIO_BLK_PCI(obj);
> > +
> > + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> > + TYPE_VMAPPLE_VIRTIO_BLK);
> > +}
> > +
> > +static const VirtioPCIDeviceTypeInfo vmapple_virtio_blk_pci_info = {
> > + .base_name = TYPE_VMAPPLE_VIRTIO_BLK_PCI,
> > + .generic_name = "vmapple-virtio-blk-pci",
> > + .instance_size = sizeof(VMAppleVirtIOBlkPCI),
> > + .instance_init = vmapple_virtio_blk_pci_instance_init,
> > + .class_init = vmapple_virtio_blk_pci_class_init,
> > +};
> > +
> > +static void vmapple_virtio_root_instance_init(Object *obj)
> > +{
> > + VMAppleVirtIOBlkPCI *dev = VMAPPLE_VIRTIO_BLK_PCI(obj);
> > +
> > + dev->apple_type = VIRTIO_APPLE_TYPE_ROOT;
> > +}
> > +
> > +static const TypeInfo vmapple_virtio_root_info = {
> > + .name = TYPE_VMAPPLE_VIRTIO_ROOT,
> > + .parent = "vmapple-virtio-blk-pci",
> > + .instance_size = sizeof(VMAppleVirtIOBlkPCI),
> > + .instance_init = vmapple_virtio_root_instance_init,
> > +};
> > +
> > +static void vmapple_virtio_aux_instance_init(Object *obj)
> > +{
> > + VMAppleVirtIOBlkPCI *dev = VMAPPLE_VIRTIO_BLK_PCI(obj);
> > +
> > + dev->apple_type = VIRTIO_APPLE_TYPE_AUX;
> > +}
> > +
> > +static const TypeInfo vmapple_virtio_aux_info = {
> > + .name = TYPE_VMAPPLE_VIRTIO_AUX,
> > + .parent = "vmapple-virtio-blk-pci",
> > + .instance_size = sizeof(VMAppleVirtIOBlkPCI),
> > + .instance_init = vmapple_virtio_aux_instance_init,
> > +};
> > +
> > +static void vmapple_virtio_blk_register_types(void)
> > +{
> > + type_register_static(&vmapple_virtio_blk_info);
> > + virtio_pci_types_register(&vmapple_virtio_blk_pci_info);
> > + type_register_static(&vmapple_virtio_root_info);
> > + type_register_static(&vmapple_virtio_aux_info);
> > +}
> > +
> > +type_init(vmapple_virtio_blk_register_types)
> > diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> > index f1a53fea8d6..33e2898be95 100644
> > --- a/include/hw/pci/pci_ids.h
> > +++ b/include/hw/pci/pci_ids.h
> > @@ -191,6 +191,7 @@
> > #define PCI_DEVICE_ID_APPLE_UNI_N_AGP 0x0020
> > #define PCI_DEVICE_ID_APPLE_U3_AGP 0x004b
> > #define PCI_DEVICE_ID_APPLE_UNI_N_GMAC 0x0021
> > +#define PCI_DEVICE_ID_APPLE_VIRTIO_BLK 0x1a00
> >
> > #define PCI_VENDOR_ID_SUN 0x108e
> > #define PCI_DEVICE_ID_SUN_EBUS 0x1000
> > diff --git a/include/hw/virtio/virtio-blk.h
> b/include/hw/virtio/virtio-blk.h
> > index 5c14110c4b1..28d5046ea6c 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -24,7 +24,7 @@
> > #include "qapi/qapi-types-virtio.h"
> >
> > #define TYPE_VIRTIO_BLK "virtio-blk-device"
> > -OBJECT_DECLARE_SIMPLE_TYPE(VirtIOBlock, VIRTIO_BLK)
> > +OBJECT_DECLARE_TYPE(VirtIOBlock, VirtIOBlkClass, VIRTIO_BLK)
> >
> > /* This is the last element of the write scatter-gather list */
> > struct virtio_blk_inhdr
> > @@ -100,6 +100,16 @@ typedef struct MultiReqBuffer {
> > bool is_write;
> > } MultiReqBuffer;
> >
> > +typedef struct VirtIOBlkClass {
> > + /*< private >*/
> > + VirtioDeviceClass parent;
> > + /*< public >*/
> > + bool (*handle_unknown_request)(VirtIOBlockReq *req, MultiReqBuffer
> *mrb,
> > + uint32_t type);
> > +} VirtIOBlkClass;
> > +
> > void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq);
> > +void virtio_blk_free_request(VirtIOBlockReq *req);
> > +void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status);
> >
> > #endif
> > diff --git a/include/hw/vmapple/virtio-blk.h
> b/include/hw/vmapple/virtio-blk.h
> > new file mode 100644
> > index 00000000000..b23106a3dfb
> > --- /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"
> > +#include "qom/object.h"
> > +#include "hw/virtio/virtio-pci.h"
> > +#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 */
>
>
