Re: [PATCH v3 1/3] qdev: Add a granule_mode property

[Richard Henderson <richard.henderson@linaro.org>]

On 2/21/24 10:58, Eric Auger wrote:
> Introduce a new enum type property allowing to set an
> IOMMU granule. Values are 4K, 16K, 64K and host. This
> latter indicates the vIOMMU granule will matches the
> host page size.
> 
> A subsequent patch will add such a property to the
> virtio-iommu device.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   include/hw/qdev-properties-system.h |  3 +++
>   include/hw/virtio/virtio-iommu.h    | 11 +++++++++++
>   hw/core/qdev-properties-system.c    | 15 +++++++++++++++
>   hw/virtio/virtio-iommu.c            | 10 ++++++++++
>   4 files changed, 39 insertions(+)
> 
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index 06c359c190..626be87dd3 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -8,6 +8,7 @@ extern const PropertyInfo qdev_prop_macaddr;
>   extern const PropertyInfo qdev_prop_reserved_region;
>   extern const PropertyInfo qdev_prop_multifd_compression;
>   extern const PropertyInfo qdev_prop_mig_mode;
> +extern const PropertyInfo qdev_prop_granule_mode;
>   extern const PropertyInfo qdev_prop_losttickpolicy;
>   extern const PropertyInfo qdev_prop_blockdev_on_error;
>   extern const PropertyInfo qdev_prop_bios_chs_trans;
> @@ -47,6 +48,8 @@ extern const PropertyInfo qdev_prop_iothread_vq_mapping_list;
>   #define DEFINE_PROP_MIG_MODE(_n, _s, _f, _d) \
>       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_mig_mode, \
>                          MigMode)
> +#define DEFINE_PROP_GRANULE_MODE(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_granule_mode, GranuleMode)
>   #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>       DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
>                           LostTickPolicy)
> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index 5fbe4677c2..a82c4fa471 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -31,6 +31,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VirtIOIOMMU, VIRTIO_IOMMU)
>   
>   #define TYPE_VIRTIO_IOMMU_MEMORY_REGION "virtio-iommu-memory-region"
>   
> +typedef enum GranuleMode {
> +    GRANULE_MODE_4K,
> +    GRANULE_MODE_16K,
> +    GRANULE_MODE_64K,
> +    GRANULE_MODE_HOST,
> +    GRANULE_MODE__MAX,
> +} GranuleMode;
> +
> +extern const QEnumLookup GranuleMode_lookup;
> +
>   typedef struct IOMMUDevice {
>       void         *viommu;
>       PCIBus       *bus;
> @@ -67,6 +77,7 @@ struct VirtIOIOMMU {
>       Notifier machine_done;
>       bool granule_frozen;
>       uint8_t aw_bits;
> +    GranuleMode granule_mode;
>   };
>   
>   #endif
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 1a396521d5..3a0b36a7a7 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -34,6 +34,7 @@
>   #include "net/net.h"
>   #include "hw/pci/pci.h"
>   #include "hw/pci/pcie.h"
> +#include "hw/virtio/virtio-iommu.h"
>   #include "hw/i386/x86.h"
>   #include "util/block-helpers.h"
>   
> @@ -679,6 +680,20 @@ const PropertyInfo qdev_prop_mig_mode = {
>       .set_default_value = qdev_propinfo_set_default_value_enum,
>   };
>   
> +/* --- GranuleMode --- */
> +
> +QEMU_BUILD_BUG_ON(sizeof(GranuleMode) != sizeof(int));
> +
> +const PropertyInfo qdev_prop_granule_mode = {
> +    .name = "GranuleMode",
> +    .description = "granule_mode values, "
> +                   "4K, 16K, 64K, host",
> +    .enum_table = &GranuleMode_lookup,
> +    .get = qdev_propinfo_get_enum,
> +    .set = qdev_propinfo_set_enum,
> +    .set_default_value = qdev_propinfo_set_default_value_enum,
> +};
> +
>   /* --- Reserved Region --- */
>   
>   /*
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 2ec5ef3cd1..2ce5839c60 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -44,6 +44,16 @@
>   #define VIOMMU_DEFAULT_QUEUE_SIZE 256
>   #define VIOMMU_PROBE_SIZE 512
>   
> +const QEnumLookup GranuleMode_lookup = {
> +    .array = (const char *const[]) {
> +        [GRANULE_MODE_4K]  = "4K",
> +        [GRANULE_MODE_16K] = "16K",
> +        [GRANULE_MODE_64K] = "64K",
> +        [GRANULE_MODE_HOST] = "host",
> +    },
> +    .size = GRANULE_MODE__MAX
> +};

Does it ever make sense to use anything other than host?
Especially since 4k fails on a 64k host, as you report.
Why would we want to be able to select a granule that doesn't work?

There are a few older hosts with 8k pages, e.g. Alpha, MIPS, Sparc.


r~