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

[Markus Armbruster <armbru@redhat.com>]

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

> 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?

Knowing nothing about reserved region types, I (naively?) assume that a
finite set of types are encoded as small integers.  Any other integers
are then meaningless, and should be rejected.

> 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.

One, rejecting invalid values is useful even when they're set
programmatically, because it can catch programming errors.

Two, expecting users to only do what you expect is walking on rather
thin ice ;)