[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 2/5] virtio-iommu: Implement RESV_MEM probe request
From: |
Auger Eric |
Subject: |
Re: [PATCH v5 2/5] virtio-iommu: Implement RESV_MEM probe request |
Date: |
Fri, 26 Jun 2020 11:09:15 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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
>
> [...]
>
- [PATCH v5 5/5] hw/arm/virt: Let the virtio-iommu bypass MSIs, (continued)
[PATCH v5 3/5] virtio-iommu: Handle reserved regions in the translation process, Eric Auger, 2020/06/24
Re: [PATCH v5 0/5] VIRTIO-IOMMU probe request support and MSI bypass on ARM, Michael S. Tsirkin, 2020/06/24
Re: [PATCH v5 0/5] VIRTIO-IOMMU probe request support and MSI bypass on ARM, Michael S. Tsirkin, 2020/06/24