Hi Markus,
On 6/24/20 10:10 AM, Markus Armbruster wrote:
> Auger Eric <eric.au...@redhat.com> writes:
>
>> Hi Markus,
>>
>> On 6/23/20 5:13 PM, Markus Armbruster wrote:
>>> Eric Auger <eric.au...@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.au...@redhat.com>
>>>> Reviewed-by: Markus Armbruster <arm...@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
>
