Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-08-24 Thread Gerd Hoffmann
On Thu, Aug 24, 2023 at 04:30:48PM +0200, Alexander Graf wrote:

> The best thing I could come up with was the QEMU internal qom property
> x-apple-type. Happy to split them: Make the change of virtio-blk behavior
> depend on the property and make all of the PCI device/vendor swapping depend
> on a new class which then sets the x-apple-type.

Basically this, but there is no need to actually expose it as property,
the xhci emulation does something simliar for nec-specific commands
which work for '-device nec-usb-xhci' only.

Use 'git grep nec_quirks' to find the places in qemu code.

HTH,
  Gerd




Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-08-24 Thread Alexander Graf


On 16.06.23 13:48, Kevin Wolf wrote:


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.



I agree and is what I tried at first, but how do I change behavior of a 
virtio-blk-pci subclass all the way down to its virtio-blk 
implementation which lives completely outside of the scope of the 
respective class?


The best thing I could come up with was the QEMU internal qom property 
x-apple-type. Happy to split them: Make the change of virtio-blk 
behavior depend on the property and make all of the PCI device/vendor 
swapping depend on a new class which then sets the x-apple-type.






This patch adds a new qdev property called "apple-type" to virtio-blk-pci.
When that property is set, we assume the virtio-blk device is an Apple one
of the specific type and act accordingly.

Do we have any information on what the number in "apple-type" actually
means or do we have to treat it as a black box?



I have ideas, but no documentation. It's an enum space that defines 
different types of devices (AUX device, root device, etc)






Signed-off-by: Alexander Graf 
---
  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.



You're completely right. I looked it up again and turns out this is 
actually a barrier command. Any ideas on how to best implement an actual 
barrier in virtio-blk? Otherwise I'll just ignore it and always return 
S_OK. No need for the memset muckery above.






+} 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, , s->config_size);
  }

@@ -1625,6 +1643,10 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)

  s->config_size = virtio_get_config_size(_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?



I wanted to make it clear that the property is QEMU internal and not to 
be used 

Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-20 Thread Kevin Wolf
Am 20.06.2023 um 16:35 hat Stefan Hajnoczi geschrieben:
> On Wed, Jun 14, 2023 at 10:56:22PM +, Alexander Graf wrote:
> > 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:
> > +{
> > +if (s->conf.x_apple_type) {
> > +/* Only valid on Apple Virtio */
> > +char buf[iov_size(in_iov, in_num)];
> 
> I'm concerned that a variable-sized stack buffer could be abused by a
> malicious guest. Even if it's harmless in the Apple use case, someone
> else might copy this approach and use it where it creates a security
> problem. Please either implement iov_memset() or allocate the temporary
> buffer using bdrv_blockalign() (and free it with qemu_vfree()).
> 
> > +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);

Good point, even more so when iov_memset() should do the job with
simpler code.

Kevin


signature.asc
Description: PGP signature


Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-20 Thread Stefan Hajnoczi
On Wed, Jun 14, 2023 at 10:56:22PM +, Alexander Graf wrote:
> 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:
> +{
> +if (s->conf.x_apple_type) {
> +/* Only valid on Apple Virtio */
> +char buf[iov_size(in_iov, in_num)];

I'm concerned that a variable-sized stack buffer could be abused by a
malicious guest. Even if it's harmless in the Apple use case, someone
else might copy this approach and use it where it creates a security
problem. Please either implement iov_memset() or allocate the temporary
buffer using bdrv_blockalign() (and free it with qemu_vfree()).

> +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);


signature.asc
Description: PGP signature


Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-19 Thread Daniel P . Berrangé
On Wed, Jun 14, 2023 at 10:56:22PM +, 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 discard
> command.
> 
> This patch adds a new qdev property called "apple-type" to virtio-blk-pci.
> When that property is set, we assume the virtio-blk device is an Apple one
> of the specific type and act accordingly.

I wonder if we should treat these as two separate devices. ie define
a 'apple-virtio-blk' device name, and have it be a subclass of the
main virtio blk impl. That would allow distros to drop the apple
forked impl in their downstream when they seek to minimize their
support matrix.

> 
> Signed-off-by: Alexander Graf 
> ---
>  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:
> +{
> +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);
> +} 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;
> +}
>  memcpy(config, , s->config_size);
>  }
>  
> @@ -1625,6 +1643,10 @@ static void virtio_blk_device_realize(DeviceState 
> *dev, Error **errp)
>  
>  s->config_size = virtio_get_config_size(_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),
>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> index 9743bee965..5fbf98f750 100644
> --- a/hw/virtio/virtio-blk-pci.c
> +++ b/hw/virtio/virtio-blk-pci.c
> @@ -62,6 +62,13 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy 
> *vpci_dev, Error **errp)
>  }
>  
>  qdev_realize(vdev, BUS(_dev->bus), errp);
> +
> +if (conf->x_apple_type) {
> +/* Apple virtio-blk uses a different vendor/device id */
> +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 virtio_blk_pci_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index e4386ebb20..74e589a298 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -188,6 +188,7 @@
>  #define PCI_DEVICE_ID_APPLE_UNI_N_AGP0x0020
>  #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_SUN0x108e
>  #define PCI_DEVICE_ID_SUN_EBUS   0x1000
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index dafec432ce..7117ce754c 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -46,6 +46,7 @@ struct VirtIOBlkConf
>  uint32_t max_discard_sectors;
>  uint32_t max_write_zeroes_sectors;
>  bool 

Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-16 Thread Michael S. Tsirkin
On Fri, Jun 16, 2023 at 01:48:51PM +0200, Kevin Wolf wrote:
> 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.
> 
> > This patch adds a new qdev property called "apple-type" to virtio-blk-pci.
> > When that property is set, we assume the virtio-blk device is an Apple one
> > of the specific type and act accordingly.
> 
> Do we have any information on what the number in "apple-type" actually
> means or do we have to treat it as a black box?
> 
> > Signed-off-by: Alexander Graf 
> > ---
> >  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, , s->config_size);
> >  }
> >  
> > @@ -1625,6 +1643,10 @@ static void virtio_blk_device_realize(DeviceState 
> > *dev, Error **errp)
> >  
> >  s->config_size = virtio_get_config_size(_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?
> 
> >  DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> > index 9743bee965..5fbf98f750 100644
> > --- a/hw/virtio/virtio-blk-pci.c
> > +++ b/hw/virtio/virtio-blk-pci.c
> > @@ -62,6 +62,13 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy 
> > *vpci_dev, Error **errp)
> >  }
> >  
> >  qdev_realize(vdev, BUS(_dev->bus), errp);
> > +
> > +if (conf->x_apple_type) {
> > +/* Apple virtio-blk uses a different vendor/device id */
> > +pci_config_set_vendor_id(vpci_dev->pci_dev.config, 
> > PCI_VENDOR_ID_APPLE);
> > +

Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-16 Thread Philippe Mathieu-Daudé

On 16/6/23 13:48, Kevin Wolf wrote:

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.


I was thinking of qdev inheritance:

#define TYPE_VIRTIO_BLK_PCI_APPLE "virtio-blk-pci-apple"
OBJECT_DECLARE_SIMPLE_TYPE(VirtIOBlkPCIApple, TYPE_VIRTIO_BLK_PCI_APPLE)

struct VirtIOBlkPCIApple {
VirtIOBlkPCI parent_obj;
};

and add an optional custom handler in VirtioDeviceClass to handle the
VIRTIO_BLK_T_APPLE1 case.


This patch adds a new qdev property called "apple-type" to virtio-blk-pci.
When that property is set, we assume the virtio-blk device is an Apple one
of the specific type and act accordingly.


Do we have any information on what the number in "apple-type" actually
means or do we have to treat it as a black box?


Signed-off-by: Alexander Graf 
---
  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(+)





Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-16 Thread Kevin Wolf
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.

> This patch adds a new qdev property called "apple-type" to virtio-blk-pci.
> When that property is set, we assume the virtio-blk device is an Apple one
> of the specific type and act accordingly.

Do we have any information on what the number in "apple-type" actually
means or do we have to treat it as a black box?

> Signed-off-by: Alexander Graf 
> ---
>  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, , s->config_size);
>  }
>  
> @@ -1625,6 +1643,10 @@ static void virtio_blk_device_realize(DeviceState 
> *dev, Error **errp)
>  
>  s->config_size = virtio_get_config_size(_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?

>  DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
> index 9743bee965..5fbf98f750 100644
> --- a/hw/virtio/virtio-blk-pci.c
> +++ b/hw/virtio/virtio-blk-pci.c
> @@ -62,6 +62,13 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy 
> *vpci_dev, Error **errp)
>  }
>  
>  qdev_realize(vdev, BUS(_dev->bus), errp);
> +
> +if (conf->x_apple_type) {
> +/* Apple virtio-blk uses a different vendor/device id */
> +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 virtio_blk_pci_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index e4386ebb20..74e589a298 100644

[PATCH 05/12] hw/virtio: Add support for apple virtio-blk

2023-06-14 Thread Alexander Graf
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.

This patch adds a new qdev property called "apple-type" to virtio-blk-pci.
When that property is set, we assume the virtio-blk device is an Apple one
of the specific type and act accordingly.

Signed-off-by: Alexander Graf 
---
 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:
+{
+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);
+} 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;
+}
 memcpy(config, , s->config_size);
 }
 
@@ -1625,6 +1643,10 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 
 s->config_size = virtio_get_config_size(_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),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio/virtio-blk-pci.c b/hw/virtio/virtio-blk-pci.c
index 9743bee965..5fbf98f750 100644
--- a/hw/virtio/virtio-blk-pci.c
+++ b/hw/virtio/virtio-blk-pci.c
@@ -62,6 +62,13 @@ static void virtio_blk_pci_realize(VirtIOPCIProxy *vpci_dev, 
Error **errp)
 }
 
 qdev_realize(vdev, BUS(_dev->bus), errp);
+
+if (conf->x_apple_type) {
+/* Apple virtio-blk uses a different vendor/device id */
+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 virtio_blk_pci_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index e4386ebb20..74e589a298 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -188,6 +188,7 @@
 #define PCI_DEVICE_ID_APPLE_UNI_N_AGP0x0020
 #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_SUN0x108e
 #define PCI_DEVICE_ID_SUN_EBUS   0x1000
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index dafec432ce..7117ce754c 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -46,6 +46,7 @@ struct VirtIOBlkConf
 uint32_t max_discard_sectors;
 uint32_t max_write_zeroes_sectors;
 bool x_enable_wce_if_config_wce;
+uint32_t x_apple_type;
 };
 
 struct VirtIOBlockDataPlane;
diff --git a/include/standard-headers/linux/virtio_blk.h 
b/include/standard-headers/linux/virtio_blk.h
index 7155b1a470..bbea5d50b9 100644
--- a/include/standard-headers/linux/virtio_blk.h
+++ b/include/standard-headers/linux/virtio_blk.h
@@ -204,6 +204,9 @@ struct virtio_blk_config {
 /* Reset All zones command */
 #define VIRTIO_BLK_T_ZONE_RESET_ALL 26
 
+/* Write zeroes command */
+#define VIRTIO_BLK_T_APPLE10x1
+
 #ifndef VIRTIO_BLK_NO_LEGACY
 /* Barrier before this op. */