On 03/04/17 13:01, David Gibson wrote:
> On Sat, Apr 01, 2017 at 11:37:40PM +1100, Alexey Kardashevskiy wrote:
>> This implements a notification for a new IOMMU group attached to
>> sPAPR's logical IO bus (LIOBN) to enable in-kernel TCE acceleration.
>>
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>> ---
>> include/hw/ppc/spapr.h | 1 +
>> include/hw/vfio/vfio-common.h | 2 ++
>> hw/ppc/spapr_iommu.c | 5 +++++
>> hw/vfio/common.c | 10 ++++++++++
>> hw/vfio/spapr.c | 31 +++++++++++++++++++++++++++++++
>> hw/vfio/trace-events | 1 +
>> 6 files changed, 50 insertions(+)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 6997ed7e98..8a1b32f89a 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -617,6 +617,7 @@ void spapr_tce_table_enable(sPAPRTCETable *tcet,
>> uint32_t page_shift, uint64_t bus_offset,
>> uint32_t nb_table);
>> void spapr_tce_table_disable(sPAPRTCETable *tcet);
>> +int spapr_tce_get_fd(sPAPRTCETable *tcet);
>> void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio);
>>
>> MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 7a4135ae6f..b99f4af96e 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -175,6 +175,8 @@ extern const MemoryListener vfio_prereg_listener;
>> int vfio_spapr_create_window(VFIOContainer *container,
>> MemoryRegionSection *section,
>> hwaddr *pgsize);
>> +int vfio_spapr_notify_kvm(int vfio_kvm_device_fd, int groupfd,
>> + IOMMUMemoryRegion *iommumr);
>> int vfio_spapr_remove_window(VFIOContainer *container,
>> hwaddr offset_within_address_space);
>>
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index 5051110b9d..f7531a6408 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -171,6 +171,11 @@ static void
>> spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu,
>> }
>> }
>>
>> +int spapr_tce_get_fd(sPAPRTCETable *tcet)
>> +{
>> + return tcet->fd;
>> +}
>> +
>
> I don't think this actually abstracts anything worthwhile. The caller
> needs the sPAPRTCETable definition anyway to use container_of(), so it
> might as well just grab the field directly.
So far @fd has only been accesses from hw/ppc/spapr_iommu.c and I'd like to
keep it that way ("encapsulation"?).
>
>> static int spapr_tce_table_post_load(void *opaque, int version_id)
>> {
>> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index e8188eb3d5..b94b29be15 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -440,6 +440,16 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> goto fail;
>> }
>>
>> +#ifdef CONFIG_KVM
>> + if (kvm_enabled()) {
>> + VFIOGroup *group;
>> +
>> + QLIST_FOREACH(group, &container->group_list, container_next) {
>> + vfio_spapr_notify_kvm(vfio_kvm_device_fd, group->fd,
>> + IOMMU_MEMORY_REGION(section->mr));
>> + }
>> + }
>> +#endif
>> vfio_host_win_add(container, section->offset_within_address_space,
>> section->offset_within_address_space +
>> int128_get64(section->size) - 1, pgsize);
>> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
>> index 551870d46b..6410438e62 100644
>> --- a/hw/vfio/spapr.c
>> +++ b/hw/vfio/spapr.c
>> @@ -15,8 +15,12 @@
>>
>> #include "hw/vfio/vfio-common.h"
>> #include "hw/hw.h"
>> +#include "hw/ppc/spapr.h"
>> #include "qemu/error-report.h"
>> #include "trace.h"
>> +#ifdef CONFIG_KVM
>> +#include "linux/kvm.h"
>> +#endif
>>
>> static bool vfio_prereg_listener_skipped_section(MemoryRegionSection
>> *section)
>> {
>> @@ -188,6 +192,33 @@ int vfio_spapr_create_window(VFIOContainer *container,
>> return 0;
>> }
>>
>> +int vfio_spapr_notify_kvm(int vfio_kvm_device_fd, int groupfd,
>> + IOMMUMemoryRegion *iommumr)
>> +{
>> +#ifdef CONFIG_KVM
>> + struct kvm_vfio_spapr_tce param = {
>> + .groupfd = groupfd,
>> + };
>> + struct kvm_device_attr attr = {
>> + .group = KVM_DEV_VFIO_GROUP,
>> + .attr = KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE,
>> + .addr = (uint64_t)(unsigned long)¶m,
>> + };
>> + sPAPRTCETable *tcet = container_of(iommumr, sPAPRTCETable, iommu);
>
> This isn't safe. The caller has verified that the host backend IOMMU
> is sPAPR TCE, but you haven't verified that the *guest* IOMMU is TCE
> based. I suspect other details would prevent a TCG x86 machine with
> VT-d running on a Power host from getting this far, but it's not good
> to rely on that.
>
> So, you need to explicitly verify that the guest IOMMU region really
> is a PAPR TCE region. The obvious way would be to continue your
> QOMification and make sPAPRTCETable a subtype of IOMMUMemoryRegion,
> rather than just including it by composition.
sPAPRTCETable is a device now, with 2 memory regions - one for entire 64bit
space and different sPAPRTCETable root MRs overlap, another one is the
actual IOMMU MR - may be QOM just this one, not the entire sPAPRTCETable
thingy?
>
>> +
>> + param.tablefd = spapr_tce_get_fd(tcet);
>> + if (param.tablefd != -1) {
>> + if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) {
>> + error_report("vfio: failed to setup fd %d for a group with fd
>> %d: %s",
>> + param.tablefd, param.groupfd, strerror(errno));
>> + return -errno;
>> + }
>> + }
>> + trace_vfio_spapr_notify_kvm(groupfd, param.tablefd);
>> +#endif
>> + return 0;
>> +}
>> +
>> int vfio_spapr_remove_window(VFIOContainer *container,
>> hwaddr offset_within_address_space)
>> {
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index 2561c6d31a..084a92f7c2 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -123,3 +123,4 @@ vfio_prereg_register(uint64_t va, uint64_t size, int
>> ret) "va=%"PRIx64" size=%"P
>> vfio_prereg_unregister(uint64_t va, uint64_t size, int ret) "va=%"PRIx64"
>> size=%"PRIx64" ret=%d"
>> vfio_spapr_create_window(int ps, uint64_t ws, uint64_t off) "pageshift=0x%x
>> winsize=0x%"PRIx64" offset=0x%"PRIx64
>> vfio_spapr_remove_window(uint64_t off) "offset=%"PRIx64
>> +vfio_spapr_notify_kvm(int groupfd, int tablefd) "Attached groupfd %d to
>> liobn fd %d"
>
--
Alexey
signature.asc
Description: OpenPGP digital signature
