| Message ID | 1402935486-29136-13-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 16 Jun 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 corrupt or > expose some part of DOM0 memory. > > 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). > > 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> Makes sense. The toolstack knows if it is able to reset the device. If so, it could call an hypercall to give it back to dom0. > 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 afb4dfc..8a4bc69 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); > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
On Mon, 2014-06-16 at 17:17 +0100, 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 corrupt or > expose some part of DOM0 memory. "corruption". I'd go further and say "and 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). Can you describe here what happen in this case (I presume Xen tears down the iommu to quiesce them somehow?) > 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> > --- > 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 afb4dfc..8a4bc69 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);
Hi Ian, On 07/03/2014 12:48 PM, Ian Campbell wrote: > On Mon, 2014-06-16 at 17:17 +0100, 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 corrupt or >> expose some part of DOM0 memory. > > "corruption". > > I'd go further and say "and we may have no way to reset some platform > devices". Ok. >> 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). > > Can you describe here what happen in this case (I presume Xen tears down > the iommu to quiesce them somehow?) The SMMU drivers will mark the different Context Bank, S2CR, SMR as invalid. If the device is attempt to access the memory then, we will receive an interrupt in Xen. Actually it's only happen once, if the device is still enabled when the domain is shutdown. Regards,
On Thu, 2014-07-03 at 13:07 +0100, Julien Grall wrote: > >> 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). > > > > Can you describe here what happen in this case (I presume Xen tears down > > the iommu to quiesce them somehow?) > > The SMMU drivers will mark the different Context Bank, S2CR, SMR as > invalid. If the device is attempt to access the memory then, we will > receive an interrupt in Xen. > > Actually it's only happen once, if the device is still enabled when the > domain is shutdown. My concern was with getting a storm of such interrupts after this point. If it only happens once and any subsequent ones are damped by some means then great. Ian.
On 07/03/2014 01:53 PM, Ian Campbell wrote: > On Thu, 2014-07-03 at 13:07 +0100, Julien Grall wrote: >>>> 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). >>> >>> Can you describe here what happen in this case (I presume Xen tears down >>> the iommu to quiesce them somehow?) >> >> The SMMU drivers will mark the different Context Bank, S2CR, SMR as >> invalid. If the device is attempt to access the memory then, we will >> receive an interrupt in Xen. >> >> Actually it's only happen once, if the device is still enabled when the >> domain is shutdown. > > My concern was with getting a storm of such interrupts after this point. > If it only happens once and any subsequent ones are damped by some means > then great. I guess, it can happen with a buggy device trying to access memory alone. But I don't think we should care about this case. Regards,
On Thu, 2014-07-03 at 14:01 +0100, Julien Grall wrote: > On 07/03/2014 01:53 PM, Ian Campbell wrote: > > On Thu, 2014-07-03 at 13:07 +0100, Julien Grall wrote: > >>>> 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). > >>> > >>> Can you describe here what happen in this case (I presume Xen tears down > >>> the iommu to quiesce them somehow?) > >> > >> The SMMU drivers will mark the different Context Bank, S2CR, SMR as > >> invalid. If the device is attempt to access the memory then, we will > >> receive an interrupt in Xen. > >> > >> Actually it's only happen once, if the device is still enabled when the > >> domain is shutdown. > > > > My concern was with getting a storm of such interrupts after this point. > > If it only happens once and any subsequent ones are damped by some means > > then great. > > I guess, it can happen with a buggy device trying to access memory > alone. But I don't think we should care about this case. Ideally such a device wouldn't be able to DoS the rest of the system. Does the SMMU not have a bit to say: deny all MMIO from this context without raising an exception? Ian.
On 07/03/2014 02:42 PM, Ian Campbell wrote: > On Thu, 2014-07-03 at 14:01 +0100, Julien Grall wrote: >> On 07/03/2014 01:53 PM, Ian Campbell wrote: >>> On Thu, 2014-07-03 at 13:07 +0100, Julien Grall wrote: >>>>>> 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). >>>>> >>>>> Can you describe here what happen in this case (I presume Xen tears down >>>>> the iommu to quiesce them somehow?) >>>> >>>> The SMMU drivers will mark the different Context Bank, S2CR, SMR as >>>> invalid. If the device is attempt to access the memory then, we will >>>> receive an interrupt in Xen. >>>> >>>> Actually it's only happen once, if the device is still enabled when the >>>> domain is shutdown. >>> >>> My concern was with getting a storm of such interrupts after this point. >>> If it only happens once and any subsequent ones are damped by some means >>> then great. >> >> I guess, it can happen with a buggy device trying to access memory >> alone. But I don't think we should care about this case. > > Ideally such a device wouldn't be able to DoS the rest of the system. > > Does the SMMU not have a bit to say: deny all MMIO from this context > without raising an exception? AFAIK, no. We receive a transaction fault via the global interrupt. If we disable this interrupt we also disable potentially helpful message when the register are misconfigured. Regards,
On Thu, 2014-07-03 at 14:51 +0100, Julien Grall wrote: > On 07/03/2014 02:42 PM, Ian Campbell wrote: > > On Thu, 2014-07-03 at 14:01 +0100, Julien Grall wrote: > >> On 07/03/2014 01:53 PM, Ian Campbell wrote: > >>> On Thu, 2014-07-03 at 13:07 +0100, Julien Grall wrote: > >>>>>> 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). > >>>>> > >>>>> Can you describe here what happen in this case (I presume Xen tears down > >>>>> the iommu to quiesce them somehow?) > >>>> > >>>> The SMMU drivers will mark the different Context Bank, S2CR, SMR as > >>>> invalid. If the device is attempt to access the memory then, we will > >>>> receive an interrupt in Xen. > >>>> > >>>> Actually it's only happen once, if the device is still enabled when the > >>>> domain is shutdown. > >>> > >>> My concern was with getting a storm of such interrupts after this point. > >>> If it only happens once and any subsequent ones are damped by some means > >>> then great. > >> > >> I guess, it can happen with a buggy device trying to access memory > >> alone. But I don't think we should care about this case. > > > > Ideally such a device wouldn't be able to DoS the rest of the system. > > > > Does the SMMU not have a bit to say: deny all MMIO from this context > > without raising an exception? > > AFAIK, no. We receive a transaction fault via the global interrupt. If > we disable this interrupt we also disable potentially helpful message > when the register are misconfigured. That seems like something of a hardware shortcoming. It might be worth asking one of the ARM guys what we should do with a device which won't shut up. Ian.
On 07/03/2014 03:04 PM, Ian Campbell wrote: > On Thu, 2014-07-03 at 14:51 +0100, Julien Grall wrote: >> On 07/03/2014 02:42 PM, Ian Campbell wrote: >>> On Thu, 2014-07-03 at 14:01 +0100, Julien Grall wrote: >>>> On 07/03/2014 01:53 PM, Ian Campbell wrote: >>>>> On Thu, 2014-07-03 at 13:07 +0100, Julien Grall wrote: >>>>>>>> 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). >>>>>>> >>>>>>> Can you describe here what happen in this case (I presume Xen tears down >>>>>>> the iommu to quiesce them somehow?) >>>>>> >>>>>> The SMMU drivers will mark the different Context Bank, S2CR, SMR as >>>>>> invalid. If the device is attempt to access the memory then, we will >>>>>> receive an interrupt in Xen. >>>>>> >>>>>> Actually it's only happen once, if the device is still enabled when the >>>>>> domain is shutdown. >>>>> >>>>> My concern was with getting a storm of such interrupts after this point. >>>>> If it only happens once and any subsequent ones are damped by some means >>>>> then great. >>>> >>>> I guess, it can happen with a buggy device trying to access memory >>>> alone. But I don't think we should care about this case. >>> >>> Ideally such a device wouldn't be able to DoS the rest of the system. >>> >>> Does the SMMU not have a bit to say: deny all MMIO from this context >>> without raising an exception? >> >> AFAIK, no. We receive a transaction fault via the global interrupt. If >> we disable this interrupt we also disable potentially helpful message >> when the register are misconfigured. > > That seems like something of a hardware shortcoming. > > It might be worth asking one of the ARM guys what we should do with a > device which won't shut up. I will send an email to Marc & Will.
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 afb4dfc..8a4bc69 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 corrupt or expose some part of DOM0 memory. 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). 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> --- xen/drivers/passthrough/arm/smmu.c | 7 ++++--- xen/drivers/passthrough/device_tree.c | 8 +++----- 2 files changed, 7 insertions(+), 8 deletions(-)