| Message ID | 20241112220212.2237-1-juanpablo.ruiz@unikie.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] vfio/platform: Add mmio-base property to define start address for MMIO mapping | expand |
On Tue, 12 Nov 2024 22:02:12 +0000 Juan Pablo Ruiz <jpruiz84@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.ruiz@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 }, > }; > > /* > @@ -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;
On Wed, Nov 13, 2024 at 1:43 AM Alex Williamson <alex.williamson@redhat.com> wrote: > > On Tue, 12 Nov 2024 22:02:12 +0000 > Juan Pablo Ruiz <jpruiz84@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? > For certain devices, especially those with large MMIO regions like GPU reserved memory, it's important to have 1:1 address translations to avoid extensive modifications to the driver source code. In platforms like the NVIDIA Jetson, some drivers have not integrated the dma-ranges property from the device tree to set DMA address translations. This means the drivers expect devices at fixed physical addresses. Using 1:1 mappings allows us to passthrough these devices without altering the drivers, facilitating scalability and ease of deployment. > How does a user know which devices need fixed base addresses and what > those addresses should be? > This is primarily only used to passthrough CMA regions that are required for DMA opperatinos. For devices that do not require a specific address, users can omit the mmio-base parameter, and the platform bus will assign addresses automatically. We can improve the documentation to help users determine when and how to specify `mmio-base`. > > This patch: > > ... should be split into at least 3 patches. > You're right; the patch should be split into separate commits for clarity: - Patch 1: Increase the platform bus size. - Patch 2: Change the `mmio_size` property from 32 to 64 bits. - Patch 3: Add the `mmio-base` property. > > > > 1. Increases the VFIO platform bus size from 32MB to 130GB. > > That's a very strange and specific size. The bus size increase to 130GB (actually 127GB or 130048MB) aligns the platform bus's MMIO region with the VIRT_MEM region starting at `0x2000000000`. This alignment is necessary to accommodate devices with large MMIO requirements and to maintain a contiguous address space. > > > 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.ruiz@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 }, > > }; > > > > /* > > @@ -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; > Juan Pablo Ruiz Rosero On Wed, Nov 13, 2024 at 1:43 AM Alex Williamson <alex.williamson@redhat.com> wrote: > > On Tue, 12 Nov 2024 22:02:12 +0000 > Juan Pablo Ruiz <jpruiz84@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.ruiz@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 }, > > }; > > > > /* > > @@ -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, Regarding your concern about breaking device abstraction by directly casting `sbdev` to `VFIOPlatformDevice`, I realize this is not ideal. I'll look into alternative approaches. Thanks for your comments Juan Pablo > > 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; >
Hi Juan Pablo, On 11/13/24 00:43, Alex Williamson wrote: > On Tue, 12 Nov 2024 22:02:12 +0000 > Juan Pablo Ruiz <jpruiz84@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.ruiz@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;
On Wed, Nov 13, 2024 at 10:57 AM Eric Auger <eric.auger@redhat.com> wrote: > > Hi Juan Pablo, > > On 11/13/24 00:43, Alex Williamson wrote: > > On Tue, 12 Nov 2024 22:02:12 +0000 > > Juan Pablo Ruiz <jpruiz84@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.ruiz@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 Thanks Eric, I will use another approach. I will try with the new SMMUv3 nested translation support for DMA operations of passthrough platform GPUs. Juan Pablo > >> }; > >> > >> /* > >> @@ -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; >
Juan Pablo Ruiz <jpruiz84@gmail.com> writes: > 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. > > This patch: > > 1. Increases the VFIO platform bus size from 32MB to 130GB. > 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. Could these be split patches? > > Signed-off-by: Juan Pablo Ruiz juanpablo.ruiz@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 *If* this is accepted you should move its definition so its in order. > [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 Please use multiples of GiB for clarity. However this now reduces the memory a highmem=off armv7 virt machine can use. > }, > }; > > /* > @@ -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); > 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;
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 }, }; /* @@ -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); 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;
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. This patch: 1. Increases the VFIO platform bus size from 32MB to 130GB. 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.ruiz@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(-)