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

[Markus Armbruster <armbru@redhat.com>]

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

> Hi Markus,
>
> On 6/22/20 1:22 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>
[...]
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index cc924815da..15b84adbee 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
[...]
>>> +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, "Failed to decode reserved region low addr");
>>> +        error_append_hint(errp,
>>> +                          "should be an address in hexadecimal\n");
>> 
>> Comes out like this:
>> 
>>     qemu-system-x86_64: -device ...: Failed to decode reserved region low addr
>>     should be an address in hexadecimal
>> 
>> I'd capitalize the other way, to get
>> 
>>     qemu-system-x86_64: -device ...: failed to decode reserved region low addr
>>     Should be an address in hexadecimal
>> 
>> Note: output is made up; I failed at figuring out how to use the new
>> property.  An example in PATCH 4's commit message might help.
> OK I will add one example. In practice in the virtio-iommu case the
> property is not really meant to be passed by the end-user but should be
> set by the machine code. However I have just tested from the cmd line
> and it looks using commas as separators is a bad idea because it
> collides with ',' separating properties. So if you're OK I will change
> the comma into ':'.

Please do.

[...]