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

[Auger Eric]

Hi Markus,

On 6/23/20 5:13 PM, Markus Armbruster wrote:
> 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?
Do you mean, you would expect the virtio-iommu-pci device to abort in
case a wrong VIRTIO reserved region type has been registered?
Effectively I could do that.

For the time being, unexpected types are considered as RESERVED type.
Also reserved regions are set by the machinesa nd we don't expect users
to set them directly so I thought it was sufficient.

Thanks

Eric
> 
>> +    }
>> +    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.
>