Hi Juan Pablo,
On 11/13/24 00:43, Alex Williamson wrote:
> On Tue, 12 Nov 2024 22:02:12 +0000
> Juan Pablo Ruiz <jprui...@gmail.com> wrote:
>
>> Some platform devices have large MMIO regions (e.g., GPU reserved memory).
>> For
>> certain devices, it's preferable to have a 1:1 address translation in the VM
>> to
>> avoid modifying driver source code.
> Why do we need 1:1 mappings? Shouldn't the device tree describe where
> the device lives in the VM address space and the driver should adapt
> rather than use hard coded addresses?
>
> How does a user know which devices need fixed base addresses and what
> those addresses should be?
>
>> This patch:
> ... should be split into at least 3 patches.
>
>> 1. Increases the VFIO platform bus size from 32MB to 130GB.
> That's a very strange and specific size.
>
>> 2. Changes the mmio_size property from 32 to 64 bits.
>> 3. Adds an mmio-base property to define the starting MMIO address for mapping
>> the VFIO device.
>>
>> Signed-off-by: Juan Pablo Ruiz juanpablo.r...@unikie.com
>> ---
>> hw/arm/virt.c | 6 +++---
>> hw/core/platform-bus.c | 28 ++++++++++++++++++++++++++--
>> hw/vfio/platform.c | 1 +
>> include/hw/platform-bus.h | 2 +-
>> include/hw/vfio/vfio-platform.h | 1 +
>> 5 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 1a381e9a2b..9fc8f4425a 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -183,13 +183,13 @@ static const MemMapEntry base_memmap[] = {
>> [VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 },
>> [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
>> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size
>> */
>> - [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
>> + [VIRT_PLATFORM_BUS] = { 0x60000000, 0x1FC0000000 }, //
>> 130048MB
>> [VIRT_SECURE_MEM] = { 0x0e000000, 0x01000000 },
>> [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 },
>> [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 },
>> [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 },
>> /* Actual RAM size depends on initial RAM and device memory settings */
>> - [VIRT_MEM] = { GiB, LEGACY_RAMLIMIT_BYTES },
>> + [VIRT_MEM] = { 0x2000000000, LEGACY_RAMLIMIT_BYTES },
This won't be accepted. We had this discussion in the past with Peter
(added in cc) and it is not possible to change the inital RAM layout
like that.
Thanks
Eric
>> };
>>
>> /*
>> @@ -1625,7 +1625,7 @@ static void create_platform_bus(VirtMachineState *vms)
>> dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
>> dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
>> qdev_prop_set_uint32(dev, "num_irqs", PLATFORM_BUS_NUM_IRQS);
>> - qdev_prop_set_uint32(dev, "mmio_size",
>> vms->memmap[VIRT_PLATFORM_BUS].size);
>> + qdev_prop_set_uint64(dev, "mmio_size",
>> vms->memmap[VIRT_PLATFORM_BUS].size);
>> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> vms->platform_bus_dev = dev;
>>
>> diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
>> index dc58bf505a..f545fab6e5 100644
>> --- a/hw/core/platform-bus.c
>> +++ b/hw/core/platform-bus.c
>> @@ -22,6 +22,7 @@
>> #include "qemu/osdep.h"
>> #include "hw/platform-bus.h"
>> #include "hw/qdev-properties.h"
>> +#include "hw/vfio/vfio-platform.h"
>> #include "qapi/error.h"
>> #include "qemu/error-report.h"
>> #include "qemu/module.h"
>> @@ -130,11 +131,29 @@ static void platform_bus_map_mmio(PlatformBusDevice
>> *pbus, SysBusDevice *sbdev,
>> int n)
>> {
>> MemoryRegion *sbdev_mr = sysbus_mmio_get_region(sbdev, n);
>> + VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> How do you know it's a vfio-platform device? This completely breaks
> device abstraction. Thanks,
>
> Alex
>
>> uint64_t size = memory_region_size(sbdev_mr);
>> uint64_t alignment = (1ULL << (63 - clz64(size + size - 1)));
>> uint64_t off;
>> + uint64_t mmio_base_off;
>> bool found_region = false;
>>
>> + if (vdev->mmio_base) {
>> + if(vdev->mmio_base < pbus->mmio.addr ||
>> + vdev->mmio_base >= pbus->mmio.addr + pbus->mmio_size){
>> + error_report("Platform Bus: MMIO base 0x%"PRIx64
>> + " outside platform bus region [0x%"PRIx64",0x%"PRIx64"]",
>> + vdev->mmio_base,
>> + pbus->mmio.addr,
>> + pbus->mmio.addr + pbus->mmio_size);
>> + exit(1);
>> + }
>> +
>> + mmio_base_off = vdev->mmio_base - pbus->mmio.addr;
>> + } else {
>> + mmio_base_off = 0;
>> + }
>> +
>> if (memory_region_is_mapped(sbdev_mr)) {
>> /* Region is already mapped, nothing to do */
>> return;
>> @@ -144,7 +163,7 @@ static void platform_bus_map_mmio(PlatformBusDevice
>> *pbus, SysBusDevice *sbdev,
>> * Look for empty space in the MMIO space that is naturally aligned with
>> * the target device's memory region
>> */
>> - for (off = 0; off < pbus->mmio_size; off += alignment) {
>> + for (off = mmio_base_off; off < pbus->mmio_size; off += alignment) {
>> MemoryRegion *mr = memory_region_find(&pbus->mmio, off, size).mr;
>> if (!mr) {
>> found_region = true;
>> @@ -154,6 +173,11 @@ static void platform_bus_map_mmio(PlatformBusDevice
>> *pbus, SysBusDevice *sbdev,
>> }
>> }
>>
>> + if (vdev->mmio_base && vdev->mmio_base != off + pbus->mmio.addr) {
>> + warn_report("Platform Bus: Not able to map in mmio base:
>> 0x%"PRIx64,
>> + vdev->mmio_base);
>> + }
>> +
>> if (!found_region) {
>> error_report("Platform Bus: Can not fit MMIO region of size
>> %"PRIx64,
>> size);
>> @@ -206,7 +230,7 @@ static void platform_bus_realize(DeviceState *dev, Error
>> **errp)
>>
>> static Property platform_bus_properties[] = {
>> DEFINE_PROP_UINT32("num_irqs", PlatformBusDevice, num_irqs, 0),
>> - DEFINE_PROP_UINT32("mmio_size", PlatformBusDevice, mmio_size, 0),
>> + DEFINE_PROP_UINT64("mmio_size", PlatformBusDevice, mmio_size, 0),
>> DEFINE_PROP_END_OF_LIST()
>> };
>>
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index a85c199c76..cfac564093 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -640,6 +640,7 @@ static Property vfio_platform_dev_properties[] = {
>> DEFINE_PROP_LINK("iommufd", VFIOPlatformDevice, vbasedev.iommufd,
>> TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
>> #endif
>> + DEFINE_PROP_UINT64("mmio-base", VFIOPlatformDevice, mmio_base, 0),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
>> index 44f30c5353..4e9913a5d7 100644
>> --- a/include/hw/platform-bus.h
>> +++ b/include/hw/platform-bus.h
>> @@ -34,7 +34,7 @@ struct PlatformBusDevice {
>> SysBusDevice parent_obj;
>>
>> /*< public >*/
>> - uint32_t mmio_size;
>> + uint64_t mmio_size;
>> MemoryRegion mmio;
>>
>> uint32_t num_irqs;
>> diff --git a/include/hw/vfio/vfio-platform.h
>> b/include/hw/vfio/vfio-platform.h
>> index c414c3dffc..90575b5852 100644
>> --- a/include/hw/vfio/vfio-platform.h
>> +++ b/include/hw/vfio/vfio-platform.h
>> @@ -59,6 +59,7 @@ struct VFIOPlatformDevice {
>> uint32_t mmap_timeout; /* delay to re-enable mmaps after interrupt */
>> QEMUTimer *mmap_timer; /* allows fast-path resume after IRQ hit */
>> QemuMutex intp_mutex; /* protect the intp_list IRQ state */
>> + uint64_t mmio_base; /* base address to start looking for mmio */
>> bool irqfd_allowed; /* debug option to force irqfd on/off */
>> };
>> typedef struct VFIOPlatformDevice VFIOPlatformDevice;
