On Thu, Feb 29, 2024 at 11:22 AM Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:
>
> On 29.02.24 02:11, Peter Xu wrote:
> > On Wed, Feb 28, 2024 at 08:07:47PM +0100, Heinrich Schuchardt wrote:
> >> On 28.02.24 19:39, Peter Maydell wrote:
> >>> On Wed, 28 Feb 2024 at 18:28, Heinrich Schuchardt
> >>> <heinrich.schucha...@canonical.com> wrote:
> >>>>
> >>>> On 28.02.24 16:06, Philippe Mathieu-Daudé wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On 28/2/24 13:59, Heinrich Schuchardt wrote:
> >>>>>> virtqueue_map_desc() is called with values of sz exceeding that may
> >>>>>> exceed
> >>>>>> TARGET_PAGE_SIZE. sz = 0x2800 has been observed.
> >
> > Pure (and can also be stupid) question: why virtqueue_map_desc() would map
> > to !direct mem? Shouldn't those buffers normally allocated from guest RAM?
> >
> >>>>>>
> >>>>>> We only support a single bounce buffer. We have to avoid
> >>>>>> virtqueue_map_desc() calling address_space_map() multiple times.
> >>>>>> Otherwise
> >>>>>> we see an error
> >>>>>>
> >>>>>> qemu: virtio: bogus descriptor or out of resources
> >>>>>>
> >>>>>> Increase the minimum size of the bounce buffer to 0x10000 which matches
> >>>>>> the largest value of TARGET_PAGE_SIZE for all architectures.
> >>>>>>
> >>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
> >>>>>> ---
> >>>>>> v2:
> >>>>>> remove unrelated change
> >>>>>> ---
> >>>>>> system/physmem.c | 8 ++++++--
> >>>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/system/physmem.c b/system/physmem.c
> >>>>>> index e3ebc19eef..3c82da1c86 100644
> >>>>>> --- a/system/physmem.c
> >>>>>> +++ b/system/physmem.c
> >>>>>> @@ -3151,8 +3151,12 @@ void *address_space_map(AddressSpace *as,
> >>>>>> *plen = 0;
> >>>>>> return NULL;
> >>>>>> }
> >>>>>> - /* Avoid unbounded allocations */
> >>>>>> - l = MIN(l, TARGET_PAGE_SIZE);
> >>>>>> + /*
> >>>>>> + * There is only one bounce buffer. The largest occuring
> >>>>>> value of
> >>>>>> + * parameter sz of virtqueue_map_desc() must fit into the
> >>>>>> bounce
> >>>>>> + * buffer.
> >>>>>> + */
> >>>>>> + l = MIN(l, 0x10000);
> >>>>>
> >>>>> Please define this magic value. Maybe ANY_TARGET_PAGE_SIZE or
> >>>>> TARGETS_BIGGEST_PAGE_SIZE?
> >>>>>
> >>>>> Then along:
> >>>>> QEMU_BUILD_BUG_ON(TARGET_PAGE_SIZE <= TARGETS_BIGGEST_PAGE_SIZE);
> >>>>
> >>>> Thank you Philippe for reviewing.
> >>>>
> >>>> TARGETS_BIGGEST_PAGE_SIZE does not fit as the value is not driven by the
> >>>> page size.
> >>>> How about MIN_BOUNCE_BUFFER_SIZE?
> >>>> Is include/exec/memory.h the right include for the constant?
> >>>>
> >>>> I don't think that TARGET_PAGE_SIZE has any relevance for setting the
> >>>> bounce buffer size. I only mentioned it to say that we are not
> >>>> decreasing the value on any existing architecture.
> >>>>
> >>>> I don't know why TARGET_PAGE_SIZE ever got into this piece of code.
> >>>> e3127ae0cdcd ("exec: reorganize address_space_map") does not provide a
> >>>> reason for this choice. Maybe Paolo remembers.
> >>>
> >>> The limitation to a page dates back to commit 6d16c2f88f2a in 2009,
> >>> which was the first implementation of this function. I don't think
> >>> there's a particular reason for that value beyond that it was
> >>> probably a convenient value that was assumed to be likely "big enough".
> >>>
> >>> I think the idea with this bounce-buffer has always been that this
> >>> isn't really a code path we expected to end up in very often --
> >>> it's supposed to be for when devices are doing DMA, which they
> >>> will typically be doing to memory (backed by host RAM), not
> >>> devices (backed by MMIO and needing a bounce buffer). So the
> >>> whole mechanism is a bit "last fallback to stop things breaking
> >>> entirely".
> >>>
> >>> The address_space_map() API says that it's allowed to return
> >>> a subset of the range you ask for, so if the virtio code doesn't
> >>> cope with the minimum being set to TARGET_PAGE_SIZE then either
> >>> we need to fix that virtio code or we need to change the API
> >>> of this function. (But I think you will also get a reduced
> >>> range if you try to use it across a boundary between normal
> >>> host-memory-backed RAM and a device MemoryRegion.)
> >>
> >> If we allow a bounce buffer only to be used once (via the in_use flag), why
> >> do we allow only a single bounce buffer?
> >>
> >> Could address_space_map() allocate a new bounce buffer on every call and
> >> address_space_unmap() deallocate it?
> >>
> >> Isn't the design with a single bounce buffer bound to fail with a
> >> multi-threaded client as collision can be expected?
> >
> > See:
> >
> > https://lore.kernel.org/r/20240212080617.2559498-1-mniss...@rivosinc.com
> >
> > For some reason that series didn't land, but it seems to be helpful in this
> > case too if e.g. there can be multiple of such devices.
> >
> > Thanks,
> >
>
> Hello Peter Xu,
>
> thanks for pointing to your series. What I like about it is that it
> removes the limit of a single bounce buffer per AddressSpace.
>
> Unfortunately it does not solve my problem. You limit the sum of all of
> the allocations for a single AddressSpcace to
> DEFAULT_MAX_BOUNCE_BUFFER_SIZE = 4096 which is too small for my use case.
Note that the limit is configured for address spaces attached to PCI
devices with a parameter.
>
> Why do we need a limit?
The rationale is to prevent a guest from allocating unlimited amounts
of host memory.
> Why is it so tiny?
Nobody has come up with a good way to determine a "sufficient" amount
that works with all use cases, while at the same time addressing the
concern due to malicious guest behavior mentioned above.
(Note that I'm merely reciting previous conversations as the author of
the series Peter pointed you at)
>
> Best regards
>
> Heinrich
>
>
>
