Am 15.06.2023 um 00:56 hat Alexander Graf geschrieben:
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 discard
command.
In other words, it's a different device. We shouldn't try to
differentiate only with a property, but actually model it as a separate
device.
Signed-off-by: Alexander Graf <graf@amazon.com>
---
hw/block/virtio-blk.c | 23 +++++++++++++++++++++
hw/virtio/virtio-blk-pci.c | 7 +++++++
include/hw/pci/pci_ids.h | 1 +
include/hw/virtio/virtio-blk.h | 1 +
include/standard-headers/linux/virtio_blk.h | 3 +++
5 files changed, 35 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 39e7f23fab..76b85bb3cb 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1120,6 +1120,20 @@ static int virtio_blk_handle_request(VirtIOBlockReq
*req, MultiReqBuffer *mrb)
break;
}
+ case VIRTIO_BLK_T_APPLE1:
Can we have a more descriptive name?
+ {
+ if (s->conf.x_apple_type) {
+ /* Only valid on Apple Virtio */
+ char buf[iov_size(in_iov, in_num)];
+ memset(buf, 0, sizeof(buf));
+ iov_from_buf(in_iov, in_num, 0, buf, sizeof(buf));
+ virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
So this is a command that simply fills the guest buffer with zeros
without accessing the disk content? Weird, but ok, if that's what they
are doing...
The commit message talks about a discard command. I would have expected
a command that discards/unmaps data from the disk. I think it would be
good to call it something else in the commit message if it has nothing
to do with this.
+ } else {
+ virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
+ }
+ virtio_blk_free_request(req);
+ break;
+ }
default:
virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
virtio_blk_free_request(req);
@@ -1351,6 +1365,10 @@ static void virtio_blk_update_config(VirtIODevice *vdev,
uint8_t *config)
} else {
blkcfg.zoned.model = VIRTIO_BLK_Z_NONE;
}
+ if (s->conf.x_apple_type) {
+ /* Apple abuses the same location for its type id */
+ blkcfg.max_secure_erase_sectors = s->conf.x_apple_type;
Ideally, blkcfg would contain a union there. Since this is a type
imported from the kernel, we can't change it inside of QEMU only. Works
for me with this comment.
+ }
memcpy(config, &blkcfg, s->config_size);
}
@@ -1625,6 +1643,10 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
s->config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
s->host_features);
+ if (s->conf.x_apple_type) {
+ /* Apple Virtio puts the blk type at 0x3c, make sure we have space. */
+ s->config_size = MAX(s->config_size, 0x3d);
+ }
virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
s->blk = conf->conf.blk;
@@ -1734,6 +1756,7 @@ static Property virtio_blk_properties[] = {
conf.max_write_zeroes_sectors,
BDRV_REQUEST_MAX_SECTORS),
DEFINE_PROP_BOOL("x-enable-wce-if-config-wce", VirtIOBlock,
conf.x_enable_wce_if_config_wce, true),
+ DEFINE_PROP_UINT32("x-apple-type", VirtIOBlock, conf.x_apple_type, 0),
In a separate device, this would probably be called "apple-type"
(without "x-") like promised in the commit message?
If not, what is the reason for having an "x-" prefix?