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

[Auger Eric <eric.auger@redhat.com>]

Hi Markus,

On 6/24/20 10:10 AM, Markus Armbruster wrote:
> 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.
you're right.
> 
>> 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 ;)
Agreed, I will add an assert()

Thanks!

Eric
>