Re: [PATCH 1/2] s390x/pci: add support for guests that request direct mapping

[David Hildenbrand <david@redhat.com>]

On 10.12.24 00:22, Matthew Rosato wrote:
> On 12/9/24 5:09 PM, David Hildenbrand wrote:
>> On 09.12.24 22:45, Matthew Rosato wrote:
>>> On 12/9/24 4:01 PM, David Hildenbrand wrote:
>>>> On 09.12.24 20:29, Matthew Rosato wrote:
>>>>
>>>> Hi,
>>>>
>>>> Trying to wrap my head around that ... you mention that "pin the entirety of guest memory".
>>>>
>>>> Do you mean that we will actually end up longterm pinning all guest RAM in the kernel, similar to what vfio ends up doing?
>>>
>>> Yes.  Actually, the usecase here is specifically PCI passthrough via vfio-pci on s390.  Unlike other platforms, the default s390 approach only pins on-demand and doesn't longterm pin all guest RAM, which is nice from a memory footprint perspective but pays a price via all those guest-2 RPCIT instructions.  The goal here is now provide the optional alternative to longterm pin like other platforms.
>>
>> Okay, thanks for confirming. One more question: who will trigger this longterm-pinning? Is it vfio?
>>
>> (the code flow from your code to the pinning code would be nice)
>>

Thanks for the details.

> 
> Yes, the vfio IOMMU code triggers it.  My s390_pci_setup_stage2_map added by this patch calls memory_region_notify_iommu in a loop such that we trigger iommu notifications to map iova X+pba -> GPA X for all GPA, where pba = a "base address" offset that has to be applied when mapping on s390.

Ah, now I see that you use "ms->ram_size", so indeed, this will map 
initial RAM only.

The more-future-proof approach would indeed be to register a memory 
listener on &address_space_memory, to then map/unmap whenever notified 
about map/unmap.

See "struct vfio_memory_listener" with its region_add/region_del 
callbacks that do that, and also implement the RAMDiscardManager plumbing.


And now I wonder if there would be a way to just reuse "struct 
vfio_memory_listener" here some way? Or at least reuse the map/unmap 
functions, because ...

The notifications are sent in the largest batches possible to minimize 
vfio ioctls / use the least number of vfio dma mappings.
> 
> The iommu notifications get picked up in vfio_iommu_map_notify where we will follow the container DMA path down to vfio_legacy_dma_map; then ultimately vfio is handling the pinning via the VFIO_IOMMU_MAP_DMA ioctl.

... vfio_listener_region_add() will also just call vfio_container_dma_map().

Maybe there is a reason s390x needs to handle this using 
memory_region_notify_iommu() callbacks instead of doing it similar to 
"struct vfio_memory_listener" when registered on &address_space_memory 
without a viommu.

> 
>>>
>>>>
>>>> In that case, it would be incompatible with virtio-balloon (and without modifications with upcoming virtio-mem). Is there already a mechanism in place to handle that -- a call  to ram_block_discard_disable() -- or even a way to support coordinated discarding of RAM (e.g., virtio-mem + vfio)?
>>>
>>> Good point, should be calling add ram_block_discard_disable(true) when set register + a corresponding (false) during deregister...  Will add for v2.
>>>
>>> As for supporting coordinated discard, I was hoping to subsequently look at virtio-mem for this.
>>
>> As long as discarding is blocked for now, we're good. To support it, the RAMDiscardManager would have to be wired up, similar to vfio.
>>
>> I think the current way of handling it via
>> vf
>> +    IOMMUTLBEvent event = {
>> +        .type = IOMMU_NOTIFIER_MAP,
>> +        .entry = {
>> +            .target_as = &address_space_memory,
>> +            .translated_addr = 0,
>> +            .perm = IOMMU_RW,
>> +        },
>> +    };
>>
>>
>> Is probably not ideal: it cannot cope with memory holes (which virtio-mem would create).
>>
>> Likely, you'd instead want an address space notifier, and really only map the memory region sections you get notified about.
>>
>> There, you can test for RAMDiscardManager and handle it like vfio does.
>>
> 
> I'll start looking into this; for the moment I'll plan on blocking discarding in this series with a follow-on to then enable virtio-mem, but if I get something working sooner I'll add it to this series.  Either way I'll put you on CC.

Thanks, exploring whether we can reuse the vfio bits to map/unmap might 
be very valuable and simplify things.

-- 
Cheers,

David / dhildenb