Auger Eric <eric.au...@redhat.com> writes:
> Hi Markus,
>
> On 12/12/19 1:17 PM, Markus Armbruster wrote:
>> Eric Auger <eric.au...@redhat.com> 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 <eric.au...@redhat.com> ---
[...]
>>> 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!
