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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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 */