On Wed, Sep 09, 2026 at 04:54:18PM +0300, Alex Fishman wrote:
> It is not desirable to merge every coherent adjacent section.
> Virtio-mem sections are marked unmergeable so that their lifetimes can
> be observed independently.


To be more specific. If qemu sent add regions first then remove
regions command, we will never try to access an unmapped region.
Doesn't this solve the problem? The only issue is we can temporarily
need more regions, but that is maybe fixable.


> For example, if regions 1 and 2 are merged, unconditionally merging an
> adjacent region 3 would change the vhost memory table from:
> 
>   [ region 1 + region 2 ]
> 
> to:
> 
>   [ region 1 + region 2 + region 3 ]
> 
> This requires removing the existing region and adding an enlarged one,
> even though region 3 is unrelated to the vring.
> 
> At the failing boundary, regions 1 and 2 must be represented as one
> vhost region because a vring part crosses them; otherwise ring
> verification fails. The patch limits coalescing to that necessary
> boundary. Region 3 remains separate and can be added without reshaping
> the region containing the vring.
> 
> I'll fix the empty lines in the next version.
> 
> Thanks,
> Alex
> 
> 
> On Wed, Sep 9, 2026 at 4:31 PM Michael S. Tsirkin <[email protected]> wrote:
> 
>     On Wed, Sep 09, 2026 at 03:59:22PM +0300, Alex Fishman wrote:
>     > Virtio-mem dynamic memslots are marked unmergeable so listeners can
>     > track their lifetimes independently. A vring part crossing the boundary
>     > between two such slots consequently cannot be contained in a single
>     > vhost memory region.
>     >
>     > Coalesce adjacent unmergeable sections only when a descriptor table,
>     > available ring, or used ring spans their boundary and the sections
>     > preserve a coherent GPA-to-HVA translation. Keep unrelated slots
>     > separate so activating them does not reshape the region containing the
>     > vring.
> 
> 
>     I don't get what does it have to do with vrings. If merging them like
>     this is ok, then it's always ok?
> 
>     >
>     > Fixes: 533f5d667909 ("memory,vhost: Allow for marking memory device
>     memory regions unmergeable")
>     >
>     > Buglink: https://redhat.atlassian.net/browse/RHEL-146583
>     >
>     > Signed-off-by: Alex Fishman <[email protected]>
> 
> 
>     No empty lines between trailers,please.
> 
>     > ---
>     >  hw/virtio/vhost.c | 72 +++++++++++++++++++++++++++++++++++++++++++----
>     >  1 file changed, 67 insertions(+), 5 deletions(-)
>     >
>     > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>     > index 371dca17dd..76910e2628 100644
>     > --- a/hw/virtio/vhost.c
>     > +++ b/hw/virtio/vhost.c
>     > @@ -796,6 +796,70 @@ out:
>     >      g_free(old_sections);
>     >  }
>     > 
>     > +static bool vhost_vring_part_crosses_boundary(uint64_t ring_gpa,
>     > +                                              uint64_t ring_size,
>     > +                                              uint64_t boundary)
>     > +{
>     > +    return ring_size && ring_gpa < boundary &&
>     > +           range_get_last(ring_gpa, ring_size) >= boundary;
>     > +}
>     > +
>     > +static bool vhost_vring_crosses_boundary(struct vhost_dev *dev,
>     > +                                         uint64_t boundary)
>     > +{
>     > +    int i;
>     > +
>     > +    if (vhost_dev_has_iommu(dev)) {
>     > +        return false;
>     > +    }
>     > +
>     > +    for (i = 0; i < dev->nvqs; i++) {
>     > +        struct vhost_virtqueue *vq = &dev->vqs[i];
>     > +
>     > +        if (vhost_vring_part_crosses_boundary(vq->desc_phys, vq->
>     desc_size,
>     > +                                              boundary) ||
>     > +            vhost_vring_part_crosses_boundary(vq->avail_phys, vq->
>     avail_size,
>     > +                                              boundary) ||
>     > +            vhost_vring_part_crosses_boundary(vq->used_phys, vq->
>     used_size,
>     > +                                              boundary)) {
>     > +            return true;
>     > +        }
>     > +    }
>     > +
>     > +    return false;
>     > +}
>     > +
>     > +static bool vhost_sections_can_merge(struct vhost_dev *dev,
>     > +                                     const MemoryRegionSection
>     *prev_sec,
>     > +                                     const MemoryRegionSection 
> *section,
>     > +                                     uint64_t section_gpa,
>     > +                                     uintptr_t section_host)
>     > +{
>     > +    uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
>     > +    uintptr_t prev_host_start =
>     > +        (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
>     > +        prev_sec->offset_within_region;
>     > +    uint64_t offset;
>     > +
>     > +    if (section->mr != prev_sec->mr || section_gpa < prev_gpa_start) {
>     > +        return false;
>     > +    }
>     > +
>     > +    offset = section_gpa - prev_gpa_start;
>     > +
>     > +    if (prev_host_start + offset != section_host) {
>     > +        return false;
>     > +    }
>     > +
>     > +    if (!prev_sec->unmergeable && !section->unmergeable) {
>     > +        return true;
>     > +    }
>     > +
>     > +    /* Only override an unmergeable boundary when a ring part spans it.
>     */
>     > +    return vhost_vring_crosses_boundary(
>     > +        dev, section->offset_within_address_space);
>     > +}
>     > +
>     >  /* Adds the section data to the tmp_section structure.
>     >   * It relies on the listener calling us in memory address order
>     >   * and for each region (via the _add and _nop methods) to
>     > @@ -833,7 +897,7 @@ static void vhost_region_add_section(struct 
> vhost_dev
>     *dev,
>     >                                                 mrs_size, mrs_host);
>     >      }
>     > 
>     > -    if (dev->n_tmp_sections && !section->unmergeable) {
>     > +    if (dev->n_tmp_sections) {
>     >          /* Since we already have at least one section, lets see if
>     >           * this extends it; since we're scanning in order, we only
>     >           * have to look at the last one, and the FlatView that calls
>     > @@ -862,11 +926,9 @@ static void vhost_region_add_section(struct
>     vhost_dev *dev,
>     >                  /* A way to cleanly fail here would be better */
>     >                  return;
>     >              }
>     > -            /* Offset from the start of the previous GPA to this GPA */
>     > -            size_t offset = mrs_gpa - prev_gpa_start;
>     > 
>     > -            if (prev_host_start + offset == mrs_host &&
>     > -                section->mr == prev_sec->mr && !prev_sec->unmergeable) 
> {
>     > +            if (vhost_sections_can_merge(dev, prev_sec, section,
>     > +                                         mrs_gpa, mrs_host)) {
>     >                  uint64_t max_end = MAX(prev_host_end, mrs_host +
>     mrs_size);
>     >                  need_add = false;
>     >                  prev_sec->offset_within_address_space =
> 
> 
> 
> 
> 
> 
>     > --
>     > 2.52.0
> 
> 


Reply via email to