Re: [PATCH v5 2/5] virtio-iommu: Implement RESV_MEM probe request

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

Hi Markus

On 6/26/20 10:53 AM, Markus Armbruster wrote:
> Auger Eric <eric.auger@redhat.com> writes:
> 
>> Hi Markus,
>> On 6/25/20 9:05 AM, Markus Armbruster wrote:
>>> Eric Auger <eric.auger@redhat.com> writes:
>>>
>>>> This patch implements the PROBE request. At the moment,
>>>> only THE RESV_MEM property is handled. The first goal is
>>>> to report iommu wide reserved regions such as the MSI regions
>>>> set by the machine code. On x86 this will be the IOAPIC MSI
>>>> region, [0xFEE00000 - 0xFEEFFFFF], on ARM this may be the ITS
>>>> doorbell.
>>>>
>>>> In the future we may introduce per device reserved regions.
>>>> This will be useful when protecting host assigned devices
>>>> which may expose their own reserved regions
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>>>
>>>> ---
>>>>
>>>> v4 -> v5:
>>>> - assert if reserved region type is different from RESERVED or
>>>>   MSI
>>>>
>>>> v3 -> v4:
>>>> - removed any reference to the NONE property that does not
>>>>   exist anymore.
>>>>
>>>> v2 -> v3:
>>>> - on probe, do not fill the reminder of the buffer with zeroes
>>>>   as the buffer was already zero initialized (Bharat)
>>>>
>>>> v1 -> v2:
>>>> - move the unlock back to the same place
>>>> - remove the push label and factorize the code after the out label
>>>> - fix a bunch of cpu_to_leX according to the latest spec revision
>>>> - do not remove sizeof(last) from free space
>>>> - check the ep exists
>>>> ---
>>>>  include/hw/virtio/virtio-iommu.h |  2 +
>>>>  hw/virtio/virtio-iommu.c         | 92 ++++++++++++++++++++++++++++++--
>>>>  hw/virtio/trace-events           |  1 +
>>>>  3 files changed, 91 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
>>>> index e653004d7c..49eb105cd8 100644
>>>> --- a/include/hw/virtio/virtio-iommu.h
>>>> +++ b/include/hw/virtio/virtio-iommu.h
>>>> @@ -53,6 +53,8 @@ typedef struct VirtIOIOMMU {
>>>>      GHashTable *as_by_busptr;
>>>>      IOMMUPciBus *iommu_pcibus_by_bus_num[PCI_BUS_MAX];
>>>>      PCIBus *primary_bus;
>>>> +    ReservedRegion *reserved_regions;
>>>> +    uint32_t nb_reserved_regions;
>>>>      GTree *domains;
>>>>      QemuMutex mutex;
>>>>      GTree *endpoints;
>>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>>> index 483883ec1d..aabc3e36b1 100644
>>>> --- a/hw/virtio/virtio-iommu.c
>>>> +++ b/hw/virtio/virtio-iommu.c
>>>> @@ -38,6 +38,7 @@
>>>>  
>>>>  /* Max size */
>>>>  #define VIOMMU_DEFAULT_QUEUE_SIZE 256
>>>> +#define VIOMMU_PROBE_SIZE 512
>>>>  
>>>>  typedef struct VirtIOIOMMUDomain {
>>>>      uint32_t id;
>>>> @@ -378,6 +379,63 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
>>>>      return ret;
>>>>  }
>>>>  
>>>> +static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
>>>> +                                               uint8_t *buf, size_t free)
>>>> +{
>>>> +    struct virtio_iommu_probe_resv_mem prop = {};
>>>> +    size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
>>>> +    int i;
>>>> +
>>>> +    total = size * s->nb_reserved_regions;
>>>> +
>>>> +    if (total > free) {
>>>> +        return -ENOSPC;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < s->nb_reserved_regions; i++) {
>>>> +        prop.head.type = cpu_to_le16(VIRTIO_IOMMU_PROBE_T_RESV_MEM);
>>>> +        prop.head.length = cpu_to_le16(length);
>>>> +        prop.subtype = s->reserved_regions[i].type;
>>>> +        assert(prop.subtype == VIRTIO_IOMMU_RESV_MEM_T_RESERVED ||
>>>> +               prop.subtype == VIRTIO_IOMMU_RESV_MEM_T_MSI);
>>>
>>> The assertion makes sense here: we're mapping from the generic
>>> ReservedRegion type (which is unsigned) to the specific
>>> virtio_iommu_probe_resv_mem subtype (which is uint8_t, but only these
>>> two values are valid).
>>>
>>> Howver, the assertion should test s->reserved_regions[i].type and go
>>> before the assignment, to ensure it doesn't truncate!
>> OK I will do.
>>>
>>> Can I trigger the assertion with -device?  My try to find the answer
>>> myself failed:
>> At the moment the virtio-iommu-pci device only can be instantiated with
>> machvirt, booting in dt mode.
> 
> I asked because if I can, then invalid types need to be rejected
> cleanly.
> 
> Invalid property values can be rejected by the setter, or by the realize
> method.  Since this setter by design accepts anything that fits into
> unsigned, it's realize.

OK. Then the place where I do the assert is not the right one as this
will happen later. Probe request is triggered by the guest virtio-iommu
driver.

Thanks

Eric
> 
> [...]
>