Re: [PATCH v4 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> Introduce a new property defining a reserved region:
> <low address>:<high address>:<type>.
>
> This will be used to encode reserved IOVA regions.
>
> For instance, in virtio-iommu use case, reserved IOVA regions
> will be passed by the machine code to the virtio-iommu-pci
> device (an array of those). The type of the reserved region
> will match the virtio_iommu_probe_resv_mem subtype value:
> - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0)
> - VIRTIO_IOMMU_RESV_MEM_T_MSI (1)
>
> on PC/Q35 machine, this will be used to inform the
> virtio-iommu-pci device it should bypass the MSI region.
> The reserved region will be: 0xfee00000:0xfeefffff:1.
>
> On ARM, we can declare the ITS MSI doorbell as an MSI
> region to prevent MSIs from being mapped on guest side.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> ---
>
> v3 -> v4:
> - use ':' instead of commas as separators.
> - rearrange error messages
> - check snprintf returned value
> - dared to keep Markus' R-b despite those changes
> ---
>  include/exec/memory.h        |  6 +++
>  include/hw/qdev-properties.h |  3 ++
>  include/qemu/typedefs.h      |  1 +
>  hw/core/qdev-properties.c    | 89 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 99 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 7207025bd4..d7a53b96cc 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -51,6 +51,12 @@ extern bool global_dirty_log;
>  
>  typedef struct MemoryRegionOps MemoryRegionOps;
>  
> +struct ReservedRegion {
> +    hwaddr low;
> +    hwaddr high;
> +    unsigned int type;

Suggest to s/unsigned int/unsigned/.

> +};
> +
>  typedef struct IOMMUTLBEntry IOMMUTLBEntry;
>  
>  /* See address_space_translate: bit 0 is read, bit 1 is write.  */
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 5252bb6b1a..95d0e7201d 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string;
>  extern const PropertyInfo qdev_prop_chr;
>  extern const PropertyInfo qdev_prop_tpm;
>  extern const PropertyInfo qdev_prop_macaddr;
> +extern const PropertyInfo qdev_prop_reserved_region;
>  extern const PropertyInfo qdev_prop_on_off_auto;
>  extern const PropertyInfo qdev_prop_multifd_compression;
>  extern const PropertyInfo qdev_prop_losttickpolicy;
> @@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>      DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *)
>  #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
> +#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f)         \
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion)
>  #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto)
>  #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index ce4a78b687..15f5047bf1 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -58,6 +58,7 @@ typedef struct ISABus ISABus;
>  typedef struct ISADevice ISADevice;
>  typedef struct IsaDma IsaDma;
>  typedef struct MACAddr MACAddr;
> +typedef struct ReservedRegion ReservedRegion;
>  typedef struct MachineClass MachineClass;
>  typedef struct MachineState MachineState;
>  typedef struct MemoryListener MemoryListener;
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index ead35d7ffd..193d0d95f9 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -15,6 +15,7 @@
>  #include "chardev/char.h"
>  #include "qemu/uuid.h"
>  #include "qemu/units.h"
> +#include "qemu/cutils.h"
>  
>  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>                                    Error **errp)
> @@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = {
>      .set   = set_mac,
>  };
>  
> +/* --- Reserved Region --- */
> +
> +/*
> + * accepted syntax version:

"version" feels redundant.  Suggest to capitalize "Accepted".

> + *   <low address>:<high address>:<type>
> + *   where low/high addresses are uint64_t in hexadecimal
> + *   and type is an unsigned integer in decimal
> + */
> +static void get_reserved_region(Object *obj, Visitor *v, const char *name,
> +                                void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    ReservedRegion *rr = qdev_get_prop_ptr(dev, prop);
> +    char buffer[64];
> +    char *p = buffer;
> +    int rc;
> +
> +    rc = snprintf(buffer, sizeof(buffer), "0x%"PRIx64":0x%"PRIx64":%u",
> +                  rr->low, rr->high, rr->type);
> +    assert(rc < sizeof(buffer));
> +
> +    visit_type_str(v, name, &p, errp);
> +}
> +
> +static void set_reserved_region(Object *obj, Visitor *v, const char *name,
> +                                void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    ReservedRegion *rr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    const char *endptr;
> +    char *str;
> +    int ret;
> +
> +    if (dev->realized) {
> +        qdev_prop_set_after_realize(dev, name, errp);
> +        return;
> +    }
> +
> +    visit_type_str(v, name, &str, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    ret = qemu_strtou64(str, &endptr, 16, &rr->low);
> +    if (ret) {
> +        error_setg(errp, "start address of '%s'"
> +                   " must be a hexadecimal integer", name);
> +        goto out;
> +    }
> +    if (*endptr != ':') {
> +        goto separator_error;
> +    }
> +
> +    ret = qemu_strtou64(endptr + 1, &endptr, 16, &rr->high);
> +    if (ret) {
> +        error_setg(errp, "end address of '%s'"
> +                   " must be a hexadecimal integer", name);
> +        goto out;
> +    }
> +    if (*endptr != ':') {
> +        goto separator_error;
> +    }
> +
> +    ret = qemu_strtoui(endptr + 1, &endptr, 10, &rr->type);
> +    if (ret) {
> +        error_setg(errp, "type of '%s'"
> +                   " must be an unsigned integer in decimal", name);

Suggest "must be a non-negative decimal integer".

Whatever uses the property needs a range check.  I can't see that the
patches that follow.  What am I missing?

> +    }
> +    goto out;
> +
> +separator_error:
> +    error_setg(errp, "reserved region fields must be separated with ':'");
> +out:
> +    g_free(str);
> +    return;
> +}
> +
> +const PropertyInfo qdev_prop_reserved_region = {
> +    .name  = "reserved_region",
> +    .description = "Reserved Region, example: 0xFEE00000:0xFEEFFFFF:0",
> +    .get   = get_reserved_region,
> +    .set   = set_reserved_region,
> +};
> +
>  /* --- on/off/auto --- */
>  
>  const PropertyInfo qdev_prop_on_off_auto = {

R-by for this patch stands.