Re: [PATCH for-5.0 v11 12/20] qapi: Introduce DEFINE_PROP_INTERVAL

[Markus Armbruster]

Auger Eric <address@hidden> writes:

> Hi Markus,
>
> On 12/12/19 1:17 PM, Markus Armbruster wrote:
>> Eric Auger <address@hidden> writes:
>> 
>>> Introduce a new property defining a labelled interval:
>>> <low address>,<high address>,label.
>>>
>>> This will be used to encode reserved IOVA regions. The label
>>> is left undefined to ease reuse accross use cases.
>> 
>> What does the last sentence mean?
> The dilemma was shall I specialize this property such as ReservedRegion
> or shall I leave it generic enough to serve somebody else use case. I
> first chose the latter but now I think I should rather call it something
> like ReservedRegion as in any case it has addresses and an integer label.
>> 
>>> 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 label will match the
>>> virtio_iommu_probe_resv_mem subtype value:
>>> - VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0)
>>> - VIRTIO_IOMMU_RESV_MEM_T_MSI (1)
>>>
>>> This is used to inform the virtio-iommu-pci device it should
>>> bypass the MSI region: 0xfee00000, 0xfeefffff, 1.
>> 
>> So the "label" part of "<low address>,<high address>,label" is a number?
> yes it is.
>> 
>> Is a number appropriate for your use case, or would an enum be better?
> I think a number is OK. There might be other types of reserved regions
> in the future. Also if we want to allow somebody else to reuse that
> property in another context, I would rather leave it open?

I'd prioritize the user interface over possible reuse (which might never
happen).  Mind, I'm not telling you using numbers is a bad user
interface.  In general, enums are nicer, but I don't know enough about
this particular case.

>> 
>>>
>>> Signed-off-by: Eric Auger <address@hidden> ---
[...]
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index e499dc215b..e238d1c352 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -57,6 +57,12 @@ struct MemoryRegionMmio {
>>>      CPUWriteMemoryFunc *write[3];
>>>  };
>>>  
>>> +struct Interval {
>>> +    hwaddr low;
>>> +    hwaddr high;
>>> +    unsigned int type;
>>> +};
>> 
>> This isn't an interval.  An interval consists of two values, not three.
>> 
>> The third one is called "type" here, and "label" elsewhere.  Pick one
>> and stick to it.
>> 
>> Then pick a name for the triple.  Elsewhere, you call it "labelled
>> interval".
> I would tend to use ReservedRegion now if nobody objects.

Sounds good to me.

> Thank you for the review!

You're welcome!