| Message ID | 1406818852-31856-17-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 31 Jul 2014, Julien Grall wrote: > Currently, when the device is deassigned from a domain, we directly reassign > to DOM0. > > As the device may not have been correctly reset, this may lead to corruption or > expose some part of DOM0 memory. Also, we may have no way to reset some > platform devices. > > If Xen reassigns the device to "nobody", it may receive some global/context > fault because the transaction has failed (indeed the context has been > marked invalid). Unfortunately there is no simple way to quiesce a buggy > hardware. I think we could live with that for a first version of platform > device passthrough. > > DOM0 will have to issue an hypercall to assign the device to itself if it > wants to use it. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v2: > - Fix typoes in the commit message > - Update commit message > --- > xen/drivers/passthrough/arm/smmu.c | 7 ++++--- > xen/drivers/passthrough/device_tree.c | 8 +++----- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index f4eb2a2..b25034e 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -1245,8 +1245,8 @@ static int arm_smmu_reassign_dt_dev(struct domain *s, struct domain *t, > { > int ret = 0; > > - /* Don't allow remapping on other domain than hwdom */ > - if ( t != hardware_domain ) > + /* Allow remapping either on the hardware domain or to nothing */ > + if ( t && t != hardware_domain ) > return -EPERM; > > if ( t == s ) > @@ -1256,7 +1256,8 @@ static int arm_smmu_reassign_dt_dev(struct domain *s, struct domain *t, > if ( ret ) > return ret; > > - ret = arm_smmu_attach_dev(t, dev); > + if ( t ) > + ret = arm_smmu_attach_dev(t, dev); > > return ret; > } > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index 45d4a59..f8affa0 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -75,14 +75,12 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev) > > spin_lock(&dtdevs_lock); > > - rc = hd->platform_ops->reassign_dt_device(d, hardware_domain, dev); > + rc = hd->platform_ops->reassign_dt_device(d, NULL, dev); > if ( rc ) > goto fail; At this point, isn't it better just to call arm_smmu_detach_dev? > - list_del(&dev->domain_list); > - > - dt_device_set_used_by(dev, hardware_domain->domain_id); > - list_add(&dev->domain_list, &domain_hvm_iommu(hardware_domain)->dt_devices); > + list_del_init(&dev->domain_list); > + dt_device_set_used_by(dev, DOMID_IO); > > fail: > spin_unlock(&dtdevs_lock); > -- > 1.7.10.4 >
Hi Stefano, Sorry for the very late answer on this patch. On 06/08/14 17:23, Stefano Stabellini wrote: >> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >> index 45d4a59..f8affa0 100644 >> --- a/xen/drivers/passthrough/device_tree.c >> +++ b/xen/drivers/passthrough/device_tree.c >> @@ -75,14 +75,12 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev) >> >> spin_lock(&dtdevs_lock); >> >> - rc = hd->platform_ops->reassign_dt_device(d, hardware_domain, dev); >> + rc = hd->platform_ops->reassign_dt_device(d, NULL, dev); >> if ( rc ) >> goto fail; > > At this point, isn't it better just to call arm_smmu_detach_dev? That would mean introducing an new callback in structure. I would prefer modify reassign_dt_device rather than adding a new one. Regards,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index f4eb2a2..b25034e 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -1245,8 +1245,8 @@ static int arm_smmu_reassign_dt_dev(struct domain *s, struct domain *t, { int ret = 0; - /* Don't allow remapping on other domain than hwdom */ - if ( t != hardware_domain ) + /* Allow remapping either on the hardware domain or to nothing */ + if ( t && t != hardware_domain ) return -EPERM; if ( t == s ) @@ -1256,7 +1256,8 @@ static int arm_smmu_reassign_dt_dev(struct domain *s, struct domain *t, if ( ret ) return ret; - ret = arm_smmu_attach_dev(t, dev); + if ( t ) + ret = arm_smmu_attach_dev(t, dev); return ret; } diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 45d4a59..f8affa0 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -75,14 +75,12 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev) spin_lock(&dtdevs_lock); - rc = hd->platform_ops->reassign_dt_device(d, hardware_domain, dev); + rc = hd->platform_ops->reassign_dt_device(d, NULL, dev); if ( rc ) goto fail; - list_del(&dev->domain_list); - - dt_device_set_used_by(dev, hardware_domain->domain_id); - list_add(&dev->domain_list, &domain_hvm_iommu(hardware_domain)->dt_devices); + list_del_init(&dev->domain_list); + dt_device_set_used_by(dev, DOMID_IO); fail: spin_unlock(&dtdevs_lock);
Currently, when the device is deassigned from a domain, we directly reassign to DOM0. As the device may not have been correctly reset, this may lead to corruption or expose some part of DOM0 memory. Also, we may have no way to reset some platform devices. If Xen reassigns the device to "nobody", it may receive some global/context fault because the transaction has failed (indeed the context has been marked invalid). Unfortunately there is no simple way to quiesce a buggy hardware. I think we could live with that for a first version of platform device passthrough. DOM0 will have to issue an hypercall to assign the device to itself if it wants to use it. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Fix typoes in the commit message - Update commit message --- xen/drivers/passthrough/arm/smmu.c | 7 ++++--- xen/drivers/passthrough/device_tree.c | 8 +++----- 2 files changed, 7 insertions(+), 8 deletions(-)