A nested SMMU must use iommufd ioctls to communicate with the host-level
SMMU instance for 2-stage translation support. Add an iommufd link to the
ARM virt-machine, allowing QEMU command to pass in an iommufd object.
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
hw/arm/virt.c | 14 ++++++++++++++
include/hw/arm/virt.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 78af2d2195..71093d7c60 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1404,6 +1404,13 @@ static void create_smmu(const VirtMachineState *vms,
object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
&error_abort);
+
+ if (vms->iommu == VIRT_IOMMU_NESTED_SMMUV3) {
+ g_assert(vms->iommufd);
+ object_property_set_link(OBJECT(dev), "iommufd", OBJECT(vms->iommufd),
+ &error_abort);
+ object_property_set_bool(OBJECT(dev), "nested", true, &error_abort);
+ }
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
for (i = 0; i < NUM_SMMU_IRQS; i++) {
@@ -3114,6 +3121,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
"Set GIC version. "
"Valid values are 2, 3, 4, host and max");
+ object_class_property_add_link(oc, "iommufd", TYPE_IOMMUFD_BACKEND,
+ offsetof(VirtMachineState, iommufd),
+ object_property_allow_set_link,
+ OBJ_PROP_LINK_STRONG);
+ object_class_property_set_description(oc, "iommufd",
+ "Set the IOMMUFD handler from \"-iommufd\"");
+
object_class_property_add_str(oc, "iommu", virt_get_iommu, virt_set_iommu);
object_class_property_set_description(oc, "iommu",
"Set the IOMMU type. "
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 7df0813e28..d5cbce1a30 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -36,6 +36,7 @@
#include "hw/arm/boot.h"
#include "hw/arm/bsa.h"
#include "hw/block/flash.h"
+#include "sysemu/iommufd.h"
#include "sysemu/kvm.h"
#include "hw/intc/arm_gicv3_common.h"
#include "qom/object.h"
@@ -154,6 +155,7 @@ struct VirtMachineState {
bool dtb_randomness;
OnOffAuto acpi;
VirtGICType gic_version;
+ IOMMUFDBackend *iommufd;
VirtIOMMUType iommu;
bool default_bus_bypass_iommu;
VirtMSIControllerType msi_controller;
--
2.43.0Hi Nicolin,
On 6/26/24 02:28, Nicolin Chen wrote:
> A nested SMMU must use iommufd ioctls to communicate with the host-level
> SMMU instance for 2-stage translation support. Add an iommufd link to the
> ARM virt-machine, allowing QEMU command to pass in an iommufd object.
If I am not wrong vfio devices are allowed to use different iommufd's
(although there is no real benefice). So this command line wouldn't
match with that option.
Also while reading the commit msg it is not clear with the iommufd is
needed in the machine whereas the vfio iommufd BE generally calls those
ioctls.
Thanks
Eric
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> hw/arm/virt.c | 14 ++++++++++++++
> include/hw/arm/virt.h | 2 ++
> 2 files changed, 16 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 78af2d2195..71093d7c60 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1404,6 +1404,13 @@ static void create_smmu(const VirtMachineState *vms,
>
> object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
> &error_abort);
> +
> + if (vms->iommu == VIRT_IOMMU_NESTED_SMMUV3) {
> + g_assert(vms->iommufd);
> + object_property_set_link(OBJECT(dev), "iommufd", OBJECT(vms->iommufd),
> + &error_abort);
> + object_property_set_bool(OBJECT(dev), "nested", true, &error_abort);
> + }
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> for (i = 0; i < NUM_SMMU_IRQS; i++) {
> @@ -3114,6 +3121,13 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> "Set GIC version. "
> "Valid values are 2, 3, 4, host and max");
>
> + object_class_property_add_link(oc, "iommufd", TYPE_IOMMUFD_BACKEND,
> + offsetof(VirtMachineState, iommufd),
> + object_property_allow_set_link,
> + OBJ_PROP_LINK_STRONG);
> + object_class_property_set_description(oc, "iommufd",
> + "Set the IOMMUFD handler from \"-iommufd\"");
> +
> object_class_property_add_str(oc, "iommu", virt_get_iommu, virt_set_iommu);
> object_class_property_set_description(oc, "iommu",
> "Set the IOMMU type. "
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 7df0813e28..d5cbce1a30 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -36,6 +36,7 @@
> #include "hw/arm/boot.h"
> #include "hw/arm/bsa.h"
> #include "hw/block/flash.h"
> +#include "sysemu/iommufd.h"
> #include "sysemu/kvm.h"
> #include "hw/intc/arm_gicv3_common.h"
> #include "qom/object.h"
> @@ -154,6 +155,7 @@ struct VirtMachineState {
> bool dtb_randomness;
> OnOffAuto acpi;
> VirtGICType gic_version;
> + IOMMUFDBackend *iommufd;
> VirtIOMMUType iommu;
> bool default_bus_bypass_iommu;
> VirtMSIControllerType msi_controller;
Hi Eric, Thanks for the comments! On Tue, Jul 09, 2024 at 11:11:56AM +0200, Eric Auger wrote: > On 6/26/24 02:28, Nicolin Chen wrote: > > A nested SMMU must use iommufd ioctls to communicate with the host-level > > SMMU instance for 2-stage translation support. Add an iommufd link to the > > ARM virt-machine, allowing QEMU command to pass in an iommufd object. > If I am not wrong vfio devices are allowed to use different iommufd's > (although there is no real benefice). So this command line wouldn't > match with that option. I think Jason's remarks highlighted that FD should be one per VM: https://lore.kernel.org/qemu-devel/20240503141024.GE3341011@nvidia.com/ > Also while reading the commit msg it is not clear with the iommufd is > needed in the machine whereas the vfio iommufd BE generally calls those > ioctls. I think I forgot to revisit it. Both intel_iommu and smmu-common used to call iommufd_backend_connect() for counting, so there was a need to pass in the same iommufd handler to the viommu driver. For SMMU, since it is created in the virt code, we had to pass in with this patch. That being said, it looks like intel_iommu had removed that. So, likely we don't need an extra user counting for SMMU too. Thank you Nicolin
On 7/9/24 18:59, Nicolin Chen wrote: > Hi Eric, > > Thanks for the comments! > > On Tue, Jul 09, 2024 at 11:11:56AM +0200, Eric Auger wrote: >> On 6/26/24 02:28, Nicolin Chen wrote: >>> A nested SMMU must use iommufd ioctls to communicate with the host-level >>> SMMU instance for 2-stage translation support. Add an iommufd link to the >>> ARM virt-machine, allowing QEMU command to pass in an iommufd object. >> If I am not wrong vfio devices are allowed to use different iommufd's >> (although there is no real benefice). So this command line wouldn't >> match with that option. > I think Jason's remarks highlighted that FD should be one per VM: > https://lore.kernel.org/qemu-devel/20240503141024.GE3341011@nvidia.com/ OK I thought this was still envisionned althought not really meaningful. By the way, please add Yi and Zhenzhong in cc since thre problematics are connected I think. > >> Also while reading the commit msg it is not clear with the iommufd is >> needed in the machine whereas the vfio iommufd BE generally calls those >> ioctls. > I think I forgot to revisit it. Both intel_iommu and smmu-common > used to call iommufd_backend_connect() for counting, so there was > a need to pass in the same iommufd handler to the viommu driver. > For SMMU, since it is created in the virt code, we had to pass in > with this patch. > > That being said, it looks like intel_iommu had removed that. So, > likely we don't need an extra user counting for SMMU too. OK at least it deserves some explanation about the "why" Thanks Eric > > Thank you > Nicolin >
On Tue, Jul 09, 2024 at 07:06:50PM +0200, Eric Auger wrote: > On 7/9/24 18:59, Nicolin Chen wrote: > > Hi Eric, > > > > Thanks for the comments! > > > > On Tue, Jul 09, 2024 at 11:11:56AM +0200, Eric Auger wrote: > >> On 6/26/24 02:28, Nicolin Chen wrote: > >>> A nested SMMU must use iommufd ioctls to communicate with the host-level > >>> SMMU instance for 2-stage translation support. Add an iommufd link to the > >>> ARM virt-machine, allowing QEMU command to pass in an iommufd object. > >> If I am not wrong vfio devices are allowed to use different iommufd's > >> (although there is no real benefice). So this command line wouldn't > >> match with that option. > > I think Jason's remarks highlighted that FD should be one per VM: > > https://lore.kernel.org/qemu-devel/20240503141024.GE3341011@nvidia.com/ > OK I thought this was still envisionned althought not really meaningful. > By the way, please add Yi and Zhenzhong in cc since thre problematics > are connected I think. Yea. Yi/Zhenzhong, would you please shed some light on forwarding an iommufd handler to the intel_iommu code? IIRC, we did that at the beginning but removed it later? > >> Also while reading the commit msg it is not clear with the iommufd is > >> needed in the machine whereas the vfio iommufd BE generally calls those > >> ioctls. > > I think I forgot to revisit it. Both intel_iommu and smmu-common > > used to call iommufd_backend_connect() for counting, so there was > > a need to pass in the same iommufd handler to the viommu driver. > > For SMMU, since it is created in the virt code, we had to pass in > > with this patch. > > > > That being said, it looks like intel_iommu had removed that. So, > > likely we don't need an extra user counting for SMMU too. > OK at least it deserves some explanation about the "why" Yes, I agree that the commit message isn't good enough. Thanks Nicolin
Hi Nicolin, >-----Original Message----- >From: Nicolin Chen <nicolinc@nvidia.com> >Subject: Re: [PATCH RFCv1 02/10] hw/arm/virt: Add iommufd link to virt- >machine > >On Tue, Jul 09, 2024 at 07:06:50PM +0200, Eric Auger wrote: >> On 7/9/24 18:59, Nicolin Chen wrote: >> > Hi Eric, >> > >> > Thanks for the comments! >> > >> > On Tue, Jul 09, 2024 at 11:11:56AM +0200, Eric Auger wrote: >> >> On 6/26/24 02:28, Nicolin Chen wrote: >> >>> A nested SMMU must use iommufd ioctls to communicate with the >host-level >> >>> SMMU instance for 2-stage translation support. Add an iommufd link >to the >> >>> ARM virt-machine, allowing QEMU command to pass in an iommufd >object. >> >> If I am not wrong vfio devices are allowed to use different iommufd's >> >> (although there is no real benefice). So this command line wouldn't >> >> match with that option. >> > I think Jason's remarks highlighted that FD should be one per VM: >> > https://lore.kernel.org/qemu- >devel/20240503141024.GE3341011@nvidia.com/ >> OK I thought this was still envisionned althought not really meaningful. >> By the way, please add Yi and Zhenzhong in cc since thre problematics >> are connected I think. > >Yea. > >Yi/Zhenzhong, would you please shed some light on forwarding an >iommufd handler to the intel_iommu code? IIRC, we did that at the >beginning but removed it later? IOMMUFD/devid/ioas handler is packaged in HostIOMMUDeviceIOMMUFD and passed to vIOMMU, see https://github.com/yiliu1765/qemu/commit/02892a5b452382866e804c3db3bb392c8f8f500f The whole nesting series is at https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2/ We gave the user flexibility to use different iommufd backends in one VM in iommufd cdev series. We want to be backward compatible in nesting series, the code change to support that is also trivial. Thanks Zhenzhong > >> >> Also while reading the commit msg it is not clear with the iommufd is >> >> needed in the machine whereas the vfio iommufd BE generally calls >those >> >> ioctls. >> > I think I forgot to revisit it. Both intel_iommu and smmu-common >> > used to call iommufd_backend_connect() for counting, so there was >> > a need to pass in the same iommufd handler to the viommu driver. >> > For SMMU, since it is created in the virt code, we had to pass in >> > with this patch. >> > >> > That being said, it looks like intel_iommu had removed that. So, >> > likely we don't need an extra user counting for SMMU too. >> OK at least it deserves some explanation about the "why" > >Yes, I agree that the commit message isn't good enough. > >Thanks >Nicolin
© 2016 - 2024 Red Hat, Inc.