Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
On 28.02.20 22:01, Peter Xu wrote: > On Fri, Feb 28, 2020 at 09:16:28PM +0100, David Hildenbrand wrote: >> > > [...] > @@ -631,7 +658,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, size_t size, qemu_vfio_remove_mapping(s, mapping); goto out; } -s->low_water_mark += size; +s->low_water_mark += max_size; >>> >>> I think it's fine to only increase the low water mark here, however >>> imo it would be better to also cache the max size in IOVAMapping too, >>> then in resize() we double check new_size <= max_size? It also makes >>> IOVAMapping more self contained. >>> >> >> I‘ll have a look how much additional code that will imply - if it‘s simple, >> I‘ll do it. > > AFAICT it'll be as simple as introducing IOVAMapping.max_size, then > pass max_size into qemu_vfio_add_mapping(). Thanks, > Yeah, was answering from my mobile without code at hand :) added! Thanks! -- Thanks, David / dhildenb
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
On Fri, Feb 28, 2020 at 09:16:28PM +0100, David Hildenbrand wrote: > [...] > >> @@ -631,7 +658,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, > >> size_t size, > >> qemu_vfio_remove_mapping(s, mapping); > >> goto out; > >> } > >> -s->low_water_mark += size; > >> +s->low_water_mark += max_size; > > > > I think it's fine to only increase the low water mark here, however > > imo it would be better to also cache the max size in IOVAMapping too, > > then in resize() we double check new_size <= max_size? It also makes > > IOVAMapping more self contained. > > > > I‘ll have a look how much additional code that will imply - if it‘s simple, > I‘ll do it. AFAICT it'll be as simple as introducing IOVAMapping.max_size, then pass max_size into qemu_vfio_add_mapping(). Thanks, -- Peter Xu
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
> Am 28.02.2020 um 21:49 schrieb Peter Xu :
>
> On Fri, Feb 28, 2020 at 03:19:45PM -0500, David Hildenbrand wrote:
>>
>>
Am 28.02.2020 um 20:55 schrieb Peter Xu :
>>>
>>> On Thu, Feb 27, 2020 at 11:12:02AM +0100, David Hildenbrand wrote:
+static void qemu_vfio_dma_map_resize(QEMUVFIOState *s, void *host,
+ size_t old_size, size_t new_size)
+{
+IOVAMapping *m;
+int index = 0;
+
+qemu_mutex_lock(>lock);
+m = qemu_vfio_find_mapping(s, host, );
+if (!m) {
+return;
+}
+assert(m->size == old_size);
+
+/* Note: Not atomic - we need a new ioctl for that. */
+qemu_vfio_undo_mapping(s, m->iova, m->size);
+qemu_vfio_do_mapping(s, host, m->iova, new_size);
>>>
>>> Another way to ask my previous question 1 (in the other reply): Can we
>>> simply map/unmap the extra, while keep the shared untouched?
>>
>> As far as I understand the kernel implementation, unfortunately no. You
>> might be able to grow (by mapping the delta), but shrinking is not possible
>> AFAIR. And I *think* with many resizes, there could be an issue if I
>> remember correctly.
>
> Ah right (and I think the same thing will happen to vfio-pci during
> memory_region_set_size()). Then please double confirm that virtio-mem
> is disabled for both vfio-helper users and vfio-pci. Also, would you
Yes, will double check, should have taken care of both.
> mind comment above the unmap+map with more information? E.g.:
>
Sure, will add - thanks!
> For now, we must unmap the whole mapped range first and remap with
> the new size. The reason is that VFIO_IOMMU_UNMAP_DMA might fail
> with partial unmap of previous mappings, so even we can add extra
> new mappings to extend the old range, however we still won't able to
> always success on shrinking memories. The side effect is that it's
> never safe to do resizing during VM execution.
>
> Or something better. With an enriched comment, I think it's fine:
>
> Reviewed-by: Peter Xu
>
Thanks Peter! Have a nice weekend!
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
On Fri, Feb 28, 2020 at 03:19:45PM -0500, David Hildenbrand wrote:
>
>
> > Am 28.02.2020 um 20:55 schrieb Peter Xu :
> >
> > On Thu, Feb 27, 2020 at 11:12:02AM +0100, David Hildenbrand wrote:
> >> +static void qemu_vfio_dma_map_resize(QEMUVFIOState *s, void *host,
> >> + size_t old_size, size_t new_size)
> >> +{
> >> +IOVAMapping *m;
> >> +int index = 0;
> >> +
> >> +qemu_mutex_lock(>lock);
> >> +m = qemu_vfio_find_mapping(s, host, );
> >> +if (!m) {
> >> +return;
> >> +}
> >> +assert(m->size == old_size);
> >> +
> >> +/* Note: Not atomic - we need a new ioctl for that. */
> >> +qemu_vfio_undo_mapping(s, m->iova, m->size);
> >> +qemu_vfio_do_mapping(s, host, m->iova, new_size);
> >
> > Another way to ask my previous question 1 (in the other reply): Can we
> > simply map/unmap the extra, while keep the shared untouched?
>
> As far as I understand the kernel implementation, unfortunately no. You might
> be able to grow (by mapping the delta), but shrinking is not possible AFAIR.
> And I *think* with many resizes, there could be an issue if I remember
> correctly.
Ah right (and I think the same thing will happen to vfio-pci during
memory_region_set_size()). Then please double confirm that virtio-mem
is disabled for both vfio-helper users and vfio-pci. Also, would you
mind comment above the unmap+map with more information? E.g.:
For now, we must unmap the whole mapped range first and remap with
the new size. The reason is that VFIO_IOMMU_UNMAP_DMA might fail
with partial unmap of previous mappings, so even we can add extra
new mappings to extend the old range, however we still won't able to
always success on shrinking memories. The side effect is that it's
never safe to do resizing during VM execution.
Or something better. With an enriched comment, I think it's fine:
Reviewed-by: Peter Xu
Thanks,
--
Peter Xu
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
> Am 28.02.2020 um 20:55 schrieb Peter Xu :
>
> On Thu, Feb 27, 2020 at 11:12:02AM +0100, David Hildenbrand wrote:
>> +static void qemu_vfio_dma_map_resize(QEMUVFIOState *s, void *host,
>> + size_t old_size, size_t new_size)
>> +{
>> +IOVAMapping *m;
>> +int index = 0;
>> +
>> +qemu_mutex_lock(>lock);
>> +m = qemu_vfio_find_mapping(s, host, );
>> +if (!m) {
>> +return;
>> +}
>> +assert(m->size == old_size);
>> +
>> +/* Note: Not atomic - we need a new ioctl for that. */
>> +qemu_vfio_undo_mapping(s, m->iova, m->size);
>> +qemu_vfio_do_mapping(s, host, m->iova, new_size);
>
> Another way to ask my previous question 1 (in the other reply): Can we
> simply map/unmap the extra, while keep the shared untouched?
As far as I understand the kernel implementation, unfortunately no. You might
be able to grow (by mapping the delta), but shrinking is not possible AFAIR.
And I *think* with many resizes, there could be an issue if I remember
correctly.
Thanks!
>
> Thanks,
>
>> +
>> +m->size = new_size;
>> +assert(qemu_vfio_verify_mappings(s));
>> +
>> +qemu_mutex_unlock(>lock);
>> +}
>
> --
> Peter Xu
>
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
> Am 28.02.2020 um 20:43 schrieb Peter Xu : > > On Thu, Feb 27, 2020 at 11:12:02AM +0100, David Hildenbrand wrote: >> Let's implement ram_block_resized(), allowing resizeable mappings. >> >> For resizeable mappings, we reserve $max_size IOVA address space, but only >> map $size of it. When resizing, unmap the old part and remap the new >> part. We'll need a new ioctl to do this atomically (e.g., to resize >> while the guest is running - not allowed for now). > > Curious: I think it's true for now because resizing only happens > during reboot or destination VM during migration (but before > switching). However is that guaranteed too in the future? > E.g., virtio-mem will run mutual exclusive with vfio until vfio won‘t pin all guest memory anymore blindly (iow, is compatible with memory overcommit and discarding of ram blocks). > [...] > >> @@ -631,7 +658,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, >> size_t size, >> qemu_vfio_remove_mapping(s, mapping); >> goto out; >> } >> -s->low_water_mark += size; >> +s->low_water_mark += max_size; > > I think it's fine to only increase the low water mark here, however > imo it would be better to also cache the max size in IOVAMapping too, > then in resize() we double check new_size <= max_size? It also makes > IOVAMapping more self contained. > I‘ll have a look how much additional code that will imply - if it‘s simple, I‘ll do it. Thanks! > Thanks, > > -- > Peter Xu >
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
On Thu, Feb 27, 2020 at 11:12:02AM +0100, David Hildenbrand wrote:
> +static void qemu_vfio_dma_map_resize(QEMUVFIOState *s, void *host,
> + size_t old_size, size_t new_size)
> +{
> +IOVAMapping *m;
> +int index = 0;
> +
> +qemu_mutex_lock(>lock);
> +m = qemu_vfio_find_mapping(s, host, );
> +if (!m) {
> +return;
> +}
> +assert(m->size == old_size);
> +
> +/* Note: Not atomic - we need a new ioctl for that. */
> +qemu_vfio_undo_mapping(s, m->iova, m->size);
> +qemu_vfio_do_mapping(s, host, m->iova, new_size);
Another way to ask my previous question 1 (in the other reply): Can we
simply map/unmap the extra, while keep the shared untouched?
Thanks,
> +
> +m->size = new_size;
> +assert(qemu_vfio_verify_mappings(s));
> +
> +qemu_mutex_unlock(>lock);
> +}
--
Peter Xu
Re: [PATCH v3 12/15] util: vfio-helpers: Implement ram_block_resized()
On Thu, Feb 27, 2020 at 11:12:02AM +0100, David Hildenbrand wrote: > Let's implement ram_block_resized(), allowing resizeable mappings. > > For resizeable mappings, we reserve $max_size IOVA address space, but only > map $size of it. When resizing, unmap the old part and remap the new > part. We'll need a new ioctl to do this atomically (e.g., to resize > while the guest is running - not allowed for now). Curious: I think it's true for now because resizing only happens during reboot or destination VM during migration (but before switching). However is that guaranteed too in the future? [...] > @@ -631,7 +658,7 @@ int qemu_vfio_dma_map(QEMUVFIOState *s, void *host, > size_t size, > qemu_vfio_remove_mapping(s, mapping); > goto out; > } > -s->low_water_mark += size; > +s->low_water_mark += max_size; I think it's fine to only increase the low water mark here, however imo it would be better to also cache the max size in IOVAMapping too, then in resize() we double check new_size <= max_size? It also makes IOVAMapping more self contained. Thanks, -- Peter Xu
