| Message ID | 20241213225440.571382-2-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/pci: relax I/O address translation requirement | expand |
Hi Matthew, On 13/12/24 23:54, Matthew Rosato wrote: > When receiving a guest mpcifc(4) or mpcifc(6) instruction without the T > bit set, treat this as a request to perform direct mapping instead of > address translation. In order to facilitate this, pin the entirety of > guest memory into the host iommu. > > Subsequent guest DMA operations are all expected to be of the format > guest_phys+sdma, allowing them to be used as lookup into the host > iommu table. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > hw/s390x/s390-pci-bus.c | 34 +++++++++++++++++++++++++++++++-- > hw/s390x/s390-pci-inst.c | 13 +++++++++++-- > hw/s390x/s390-virtio-ccw.c | 5 +++++ > include/hw/s390x/s390-pci-bus.h | 4 ++++ > 4 files changed, 52 insertions(+), 4 deletions(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 40b2567aa7..95dbe0c984 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -18,6 +18,7 @@ > #include "hw/s390x/s390-pci-inst.h" > #include "hw/s390x/s390-pci-kvm.h" > #include "hw/s390x/s390-pci-vfio.h" > +#include "hw/boards.h" > #include "hw/pci/pci_bus.h" > #include "hw/qdev-properties.h" > #include "hw/pci/pci_bridge.h" > @@ -720,16 +721,43 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) > TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr), > name, iommu->pal + 1); > iommu->enabled = true; > + iommu->direct_map = false; > memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); > g_free(name); > } > > +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu) > +{ > + MachineState *ms = MACHINE(qdev_get_machine()); > + > + /* > + * For direct-mapping we must map the entire guest address space. Rather > + * than using an iommu, create a memory region alias that maps GPA X to > + * iova X + SDMA. VFIO will handle pinning via its memory listener. > + */ > + g_autofree char *name = g_strdup_printf("iommu-dm-s390-%04x", > + iommu->pbdev->uid); > + memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, > + 0, ms->ram_size); Is it a good idea to take the whole machine ram-size here? Could it be better to pass it as qdev property? > + iommu->enabled = true; > + iommu->direct_map = true; > + memory_region_add_subregion(&iommu->mr, iommu->pbdev->zpci_fn.sdma, > + &iommu->dm_mr); > +} > + > void s390_pci_iommu_disable(S390PCIIOMMU *iommu) > { > iommu->enabled = false; > g_hash_table_remove_all(iommu->iotlb); > - memory_region_del_subregion(&iommu->mr, MEMORY_REGION(&iommu->iommu_mr)); > - object_unparent(OBJECT(&iommu->iommu_mr)); > + if (iommu->direct_map) { > + memory_region_del_subregion(&iommu->mr, &iommu->dm_mr); > + iommu->direct_map = false; > + object_unparent(OBJECT(&iommu->dm_mr)); > + } else { > + memory_region_del_subregion(&iommu->mr, > + MEMORY_REGION(&iommu->iommu_mr)); > + object_unparent(OBJECT(&iommu->iommu_mr)); > + } > } > > static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn) > @@ -1488,6 +1516,8 @@ static Property s390_pci_device_properties[] = { > DEFINE_PROP_BOOL("interpret", S390PCIBusDevice, interp, true), > DEFINE_PROP_BOOL("forwarding-assist", S390PCIBusDevice, forwarding_assist, > true), > + DEFINE_PROP_BOOL("relaxed-translation", S390PCIBusDevice, rtr_allowed, > + true), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 41655082da..bb6f83b0c9 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -16,6 +16,7 @@ > #include "exec/memory.h" > #include "qemu/error-report.h" > #include "sysemu/hw_accel.h" > +#include "hw/boards.h" > #include "hw/pci/pci_device.h" > #include "hw/s390x/s390-pci-inst.h" > #include "hw/s390x/s390-pci-bus.h" > @@ -1008,17 +1009,25 @@ static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib, > } > > /* currently we only support designation type 1 with translation */ > - if (!(dt == ZPCI_IOTA_RTTO && t)) { > + if (t && !(dt == ZPCI_IOTA_RTTO)) { > error_report("unsupported ioat dt %d t %d", dt, t); > s390_program_interrupt(env, PGM_OPERAND, ra); > return -EINVAL; > + } else if (!t && !pbdev->rtr_allowed) { > + error_report("relaxed translation not allowed"); > + s390_program_interrupt(env, PGM_OPERAND, ra); > + return -EINVAL; > } > > iommu->pba = pba; > iommu->pal = pal; > iommu->g_iota = g_iota; > > - s390_pci_iommu_enable(iommu); > + if (t) { > + s390_pci_iommu_enable(iommu); > + } else { > + s390_pci_iommu_dm_enable(iommu); > + } > > return 0; > } > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 67ae34aead..0334467371 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -865,8 +865,13 @@ static void ccw_machine_9_2_instance_options(MachineState *machine) > > static void ccw_machine_9_2_class_options(MachineClass *mc) > { > + static GlobalProperty compat[] = { > + { TYPE_S390_PCI_DEVICE, "relaxed-translation", "off", }, > + }; > + > ccw_machine_10_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_9_2, hw_compat_9_2_len); > + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); > } > DEFINE_CCW_MACHINE(9, 2); > > diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h > index 2c43ea123f..27732247cf 100644 > --- a/include/hw/s390x/s390-pci-bus.h > +++ b/include/hw/s390x/s390-pci-bus.h > @@ -277,7 +277,9 @@ struct S390PCIIOMMU { > AddressSpace as; > MemoryRegion mr; > IOMMUMemoryRegion iommu_mr; > + MemoryRegion dm_mr; > bool enabled; > + bool direct_map; > uint64_t g_iota; > uint64_t pba; > uint64_t pal; > @@ -362,6 +364,7 @@ struct S390PCIBusDevice { > bool interp; > bool forwarding_assist; > bool aif; > + bool rtr_allowed; > QTAILQ_ENTRY(S390PCIBusDevice) link; > }; > > @@ -389,6 +392,7 @@ int pci_chsc_sei_nt2_have_event(void); > void s390_pci_sclp_configure(SCCB *sccb); > void s390_pci_sclp_deconfigure(SCCB *sccb); > void s390_pci_iommu_enable(S390PCIIOMMU *iommu); > +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu); > void s390_pci_iommu_disable(S390PCIIOMMU *iommu); > void s390_pci_generate_error_event(uint16_t pec, uint32_t fh, uint32_t fid, > uint64_t faddr, uint32_t e);
On 16.12.24 16:37, Philippe Mathieu-Daudé wrote: > Hi Matthew, > > On 13/12/24 23:54, Matthew Rosato wrote: >> When receiving a guest mpcifc(4) or mpcifc(6) instruction without the T >> bit set, treat this as a request to perform direct mapping instead of >> address translation. In order to facilitate this, pin the entirety of >> guest memory into the host iommu. >> >> Subsequent guest DMA operations are all expected to be of the format >> guest_phys+sdma, allowing them to be used as lookup into the host >> iommu table. >> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> hw/s390x/s390-pci-bus.c | 34 +++++++++++++++++++++++++++++++-- >> hw/s390x/s390-pci-inst.c | 13 +++++++++++-- >> hw/s390x/s390-virtio-ccw.c | 5 +++++ >> include/hw/s390x/s390-pci-bus.h | 4 ++++ >> 4 files changed, 52 insertions(+), 4 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index 40b2567aa7..95dbe0c984 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -18,6 +18,7 @@ >> #include "hw/s390x/s390-pci-inst.h" >> #include "hw/s390x/s390-pci-kvm.h" >> #include "hw/s390x/s390-pci-vfio.h" >> +#include "hw/boards.h" >> #include "hw/pci/pci_bus.h" >> #include "hw/qdev-properties.h" >> #include "hw/pci/pci_bridge.h" >> @@ -720,16 +721,43 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) >> TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr), >> name, iommu->pal + 1); >> iommu->enabled = true; >> + iommu->direct_map = false; >> memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); >> g_free(name); >> } >> >> +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu) >> +{ >> + MachineState *ms = MACHINE(qdev_get_machine()); >> + >> + /* >> + * For direct-mapping we must map the entire guest address space. Rather >> + * than using an iommu, create a memory region alias that maps GPA X to >> + * iova X + SDMA. VFIO will handle pinning via its memory listener. >> + */ >> + g_autofree char *name = g_strdup_printf("iommu-dm-s390-%04x", >> + iommu->pbdev->uid); >> + memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, >> + 0, ms->ram_size); > > Is it a good idea to take the whole machine ram-size here? > Could it be better to pass it as qdev property? I think we want all guest RAM, just like ordinary vfio on !s390x without a viommu would do. Matthew, I assume to handle virtio-mem, we would actually pass in here the result from s390_get_memory_limit(), which will cover initial+device RAM, correct? Until then, this would map initial RAM only.
>>> +{ >>> + MachineState *ms = MACHINE(qdev_get_machine()); >>> + >>> + /* >>> + * For direct-mapping we must map the entire guest address space. Rather >>> + * than using an iommu, create a memory region alias that maps GPA X to >>> + * iova X + SDMA. VFIO will handle pinning via its memory listener. >>> + */ >>> + g_autofree char *name = g_strdup_printf("iommu-dm-s390-%04x", >>> + iommu->pbdev->uid); >>> + memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, >>> + 0, ms->ram_size); >> >> Is it a good idea to take the whole machine ram-size here? >> Could it be better to pass it as qdev property? > > I think we want all guest RAM, just like ordinary vfio on !s390x without a viommu would do. > > Matthew, I assume to handle virtio-mem, we would actually pass in here the result from s390_get_memory_limit(), which will cover initial+device RAM, correct? Until then, this would map initial RAM only. > Good point. Using s390_get_memory_limit() sounds good to me; That will make v3 of this series dependent on the s390x virtio-mem series but sounds like you're sending that sometime this week anyway. I'll start testing on top of that with something like.. memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, 0, s390_get_memory_limit(s390ms));
On 16.12.24 17:53, Matthew Rosato wrote: > >>>> +{ >>>> + MachineState *ms = MACHINE(qdev_get_machine()); >>>> + >>>> + /* >>>> + * For direct-mapping we must map the entire guest address space. Rather >>>> + * than using an iommu, create a memory region alias that maps GPA X to >>>> + * iova X + SDMA. VFIO will handle pinning via its memory listener. >>>> + */ >>>> + g_autofree char *name = g_strdup_printf("iommu-dm-s390-%04x", >>>> + iommu->pbdev->uid); >>>> + memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, >>>> + 0, ms->ram_size); >>> >>> Is it a good idea to take the whole machine ram-size here? >>> Could it be better to pass it as qdev property? >> >> I think we want all guest RAM, just like ordinary vfio on !s390x without a viommu would do. >> >> Matthew, I assume to handle virtio-mem, we would actually pass in here the result from s390_get_memory_limit(), which will cover initial+device RAM, correct? Until then, this would map initial RAM only. >> > > Good point. Using s390_get_memory_limit() sounds good to me; That will make v3 of this series dependent on the s390x virtio-mem series but sounds like you're sending that sometime this week anyway. If my testing is good and there are no further comments, I'll queue it directly (no change to v2) to send it upstream. So it's probably a good idea to wait with a new series her. > > I'll start testing on top of that with something like.. > > memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, > 0, s390_get_memory_limit(s390ms)); > Yes, I think so. We discussed at some point maybe requiring disabling uncoordinated discarding of RAM (virtio-balloon), is that already done implicitly now?
>> Good point. Using s390_get_memory_limit() sounds good to me; That will make v3 of this series dependent on the s390x virtio-mem series but sounds like you're sending that sometime this week anyway. > > If my testing is good and there are no further comments, I'll queue it directly (no change to v2) to send it upstream. > > So it's probably a good idea to wait with a new series her. > OK > > We discussed at some point maybe requiring disabling uncoordinated discarding of RAM (virtio-balloon), is that already done implicitly now? > Yes, this should be handled via the call to ram_block_uncoordinated_discard_disable() in vfio_ram_block_discard_disable() - I just traced that now to double-check.
On 16.12.24 18:26, Matthew Rosato wrote: > >>> Good point. Using s390_get_memory_limit() sounds good to me; That will make v3 of this series dependent on the s390x virtio-mem series but sounds like you're sending that sometime this week anyway. >> >> If my testing is good and there are no further comments, I'll queue it directly (no change to v2) to send it upstream. >> >> So it's probably a good idea to wait with a new series her. >> > > OK > >> >> We discussed at some point maybe requiring disabling uncoordinated discarding of RAM (virtio-balloon), is that already done implicitly now? >> > > Yes, this should be handled via the call to ram_block_uncoordinated_discard_disable() in vfio_ram_block_discard_disable() - I just traced that now to double-check. > Cool, can you briefly mention that in the patch description? Thanks!
On 12/16/24 12:29 PM, David Hildenbrand wrote: > On 16.12.24 18:26, Matthew Rosato wrote: >> >>>> Good point. Using s390_get_memory_limit() sounds good to me; That will make v3 of this series dependent on the s390x virtio-mem series but sounds like you're sending that sometime this week anyway. >>> >>> If my testing is good and there are no further comments, I'll queue it directly (no change to v2) to send it upstream. >>> >>> So it's probably a good idea to wait with a new series her. >>> >> >> OK >> >>> >>> We discussed at some point maybe requiring disabling uncoordinated discarding of RAM (virtio-balloon), is that already done implicitly now? >>> >> >> Yes, this should be handled via the call to ram_block_uncoordinated_discard_disable() in vfio_ram_block_discard_disable() - I just traced that now to double-check. >> > > Cool, can you briefly mention that in the patch description? Thanks! > Sure, I'll add something like Pinning for the direct mapping case is handled via vfio and its memory listener. Additionally, ram discard settings are inherited from vfio: coordinated discards (e.g. virtio-mem) are allowed while uncoordinated discards (e.g. virtio-balloon) are disabled.
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 40b2567aa7..95dbe0c984 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -18,6 +18,7 @@ #include "hw/s390x/s390-pci-inst.h" #include "hw/s390x/s390-pci-kvm.h" #include "hw/s390x/s390-pci-vfio.h" +#include "hw/boards.h" #include "hw/pci/pci_bus.h" #include "hw/qdev-properties.h" #include "hw/pci/pci_bridge.h" @@ -720,16 +721,43 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu) TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr), name, iommu->pal + 1); iommu->enabled = true; + iommu->direct_map = false; memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr)); g_free(name); } +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu) +{ + MachineState *ms = MACHINE(qdev_get_machine()); + + /* + * For direct-mapping we must map the entire guest address space. Rather + * than using an iommu, create a memory region alias that maps GPA X to + * iova X + SDMA. VFIO will handle pinning via its memory listener. + */ + g_autofree char *name = g_strdup_printf("iommu-dm-s390-%04x", + iommu->pbdev->uid); + memory_region_init_alias(&iommu->dm_mr, OBJECT(&iommu->mr), name, ms->ram, + 0, ms->ram_size); + iommu->enabled = true; + iommu->direct_map = true; + memory_region_add_subregion(&iommu->mr, iommu->pbdev->zpci_fn.sdma, + &iommu->dm_mr); +} + void s390_pci_iommu_disable(S390PCIIOMMU *iommu) { iommu->enabled = false; g_hash_table_remove_all(iommu->iotlb); - memory_region_del_subregion(&iommu->mr, MEMORY_REGION(&iommu->iommu_mr)); - object_unparent(OBJECT(&iommu->iommu_mr)); + if (iommu->direct_map) { + memory_region_del_subregion(&iommu->mr, &iommu->dm_mr); + iommu->direct_map = false; + object_unparent(OBJECT(&iommu->dm_mr)); + } else { + memory_region_del_subregion(&iommu->mr, + MEMORY_REGION(&iommu->iommu_mr)); + object_unparent(OBJECT(&iommu->iommu_mr)); + } } static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn) @@ -1488,6 +1516,8 @@ static Property s390_pci_device_properties[] = { DEFINE_PROP_BOOL("interpret", S390PCIBusDevice, interp, true), DEFINE_PROP_BOOL("forwarding-assist", S390PCIBusDevice, forwarding_assist, true), + DEFINE_PROP_BOOL("relaxed-translation", S390PCIBusDevice, rtr_allowed, + true), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 41655082da..bb6f83b0c9 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -16,6 +16,7 @@ #include "exec/memory.h" #include "qemu/error-report.h" #include "sysemu/hw_accel.h" +#include "hw/boards.h" #include "hw/pci/pci_device.h" #include "hw/s390x/s390-pci-inst.h" #include "hw/s390x/s390-pci-bus.h" @@ -1008,17 +1009,25 @@ static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib, } /* currently we only support designation type 1 with translation */ - if (!(dt == ZPCI_IOTA_RTTO && t)) { + if (t && !(dt == ZPCI_IOTA_RTTO)) { error_report("unsupported ioat dt %d t %d", dt, t); s390_program_interrupt(env, PGM_OPERAND, ra); return -EINVAL; + } else if (!t && !pbdev->rtr_allowed) { + error_report("relaxed translation not allowed"); + s390_program_interrupt(env, PGM_OPERAND, ra); + return -EINVAL; } iommu->pba = pba; iommu->pal = pal; iommu->g_iota = g_iota; - s390_pci_iommu_enable(iommu); + if (t) { + s390_pci_iommu_enable(iommu); + } else { + s390_pci_iommu_dm_enable(iommu); + } return 0; } diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 67ae34aead..0334467371 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -865,8 +865,13 @@ static void ccw_machine_9_2_instance_options(MachineState *machine) static void ccw_machine_9_2_class_options(MachineClass *mc) { + static GlobalProperty compat[] = { + { TYPE_S390_PCI_DEVICE, "relaxed-translation", "off", }, + }; + ccw_machine_10_0_class_options(mc); compat_props_add(mc->compat_props, hw_compat_9_2, hw_compat_9_2_len); + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); } DEFINE_CCW_MACHINE(9, 2); diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h index 2c43ea123f..27732247cf 100644 --- a/include/hw/s390x/s390-pci-bus.h +++ b/include/hw/s390x/s390-pci-bus.h @@ -277,7 +277,9 @@ struct S390PCIIOMMU { AddressSpace as; MemoryRegion mr; IOMMUMemoryRegion iommu_mr; + MemoryRegion dm_mr; bool enabled; + bool direct_map; uint64_t g_iota; uint64_t pba; uint64_t pal; @@ -362,6 +364,7 @@ struct S390PCIBusDevice { bool interp; bool forwarding_assist; bool aif; + bool rtr_allowed; QTAILQ_ENTRY(S390PCIBusDevice) link; }; @@ -389,6 +392,7 @@ int pci_chsc_sei_nt2_have_event(void); void s390_pci_sclp_configure(SCCB *sccb); void s390_pci_sclp_deconfigure(SCCB *sccb); void s390_pci_iommu_enable(S390PCIIOMMU *iommu); +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu); void s390_pci_iommu_disable(S390PCIIOMMU *iommu); void s390_pci_generate_error_event(uint16_t pec, uint32_t fh, uint32_t fid, uint64_t faddr, uint32_t e);
When receiving a guest mpcifc(4) or mpcifc(6) instruction without the T bit set, treat this as a request to perform direct mapping instead of address translation. In order to facilitate this, pin the entirety of guest memory into the host iommu. Subsequent guest DMA operations are all expected to be of the format guest_phys+sdma, allowing them to be used as lookup into the host iommu table. Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- hw/s390x/s390-pci-bus.c | 34 +++++++++++++++++++++++++++++++-- hw/s390x/s390-pci-inst.c | 13 +++++++++++-- hw/s390x/s390-virtio-ccw.c | 5 +++++ include/hw/s390x/s390-pci-bus.h | 4 ++++ 4 files changed, 52 insertions(+), 4 deletions(-)