| Message ID | 1566324587-3442-8-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu/arm: Add Renesas IPMMU-VMSA support + Linux's iommu_fwspec | expand |
Hi Oleksandr, On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > This patch adds new iommu_add_dt_device API for adding DT device > to the IOMMU using generic IOMMU DT bindings [1] and previously > added "iommu_fwspec" support and "add_device/of_xlate" callbacks. > > New function does the following: > - Parse the DT bindings according to the specification > - Provide DT IOMMU specifier which describes the IOMMU master > interfaces of that device (device IDs, etc) to the driver > - Add master device to the IOMMU if latter is present and available > > The additional benefit here is to avoid to go through the whole DT > multiple times in IOMMU driver trying to locate master devices which > belong to each IOMMU device being probed. So the commit title/message describes the new function iommu_add_dt_device, but not the main important thing i.e. "Why is it called when building dom0". While I agree the new function is the big part of the function what matter is we need to register device using the generic IOMMU bindings before assigning the device to a domain. The split is to keep separate "add" and "assign". The later can be called from dom0. > > [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Julien Grall <julien.grall@arm.com> > > --- > Changes V2 -> V3: > - clarified patch description > - clarified comments in code > - modified to provide DT IOMMU specifier to the driver > using "of_xlate" callback > - documented function usage > - modified to return an error if ops is not present/implemented, > - added ability to return a possitive value to indicate > that device doesn't need to be protected > - removed check for the "iommu" property presence > in the common code > - included <asm/iommu_fwspec.h> directly > --- > xen/arch/arm/domain_build.c | 11 ++++++++ > xen/drivers/passthrough/arm/iommu.c | 55 +++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/iommu.h | 11 ++++++++ > 3 files changed, 77 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index e79d4e2..159ea6a 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1240,6 +1240,7 @@ static int __init map_device_children(struct domain *d, > > /* > * For a given device node: > + * - Try to call iommu_add_dt_device to protect the device by an IOMMU > * - Give permission to the guest to manage IRQ and MMIO range > * - Retrieve the IRQ configuration (i.e edge/level) from device tree > * When the device is not marked for guest passthrough: > @@ -1257,6 +1258,16 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, > u64 addr, size; > bool need_mapping = !dt_device_for_passthrough(dev); > > + dt_dprintk("%s add to iommu\n", dt_node_full_name(dev)); This message is slightly confusing. You are not adding the device, you are trying to. So how about "Check if %s is behind an IOMMU and add it". > + > + res = iommu_add_dt_device(dev); > + if ( res < 0 ) > + { > + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n", > + dt_node_full_name(dev)); > + return res; > + } > + > nirq = dt_number_of_irq(dev); > naddr = dt_number_of_address(dev); > > diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c > index 72a30e0..47e4bc6 100644 > --- a/xen/drivers/passthrough/arm/iommu.c > +++ b/xen/drivers/passthrough/arm/iommu.c > @@ -20,6 +20,7 @@ > #include <xen/lib.h> > > #include <asm/device.h> > +#include <asm/iommu_fwspec.h> > > /* > * Deferred probe list is used to keep track of devices for which driver > @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct domain *d) > void __hwdom_init arch_iommu_hwdom_init(struct domain *d) > { > } > + > +int __init iommu_add_dt_device(struct dt_device_node *np) > +{ > + const struct iommu_ops *ops = iommu_get_ops(); > + struct dt_phandle_args iommu_spec; > + struct device *dev = dt_to_dev(np); > + int rc = 1, index = 0; > + > + if ( !iommu_enabled ) > + return 1; > + > + if ( !ops || !ops->add_device || !ops->of_xlate ) The SMMU does not implement of_xlate(). It is actually only mandatory if you are using the generic bindings. So I would only check ops->of_xlate if "iommus" exists. > + return -EINVAL; > + > + if ( dev_iommu_fwspec_get(dev) ) > + return -EEXIST; > + > + /* According Documentation/devicetree/bindings/iommu/iommu.txt from Linux */ s/According/According to/ > + while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells", > + index, &iommu_spec) ) > + { > + if ( !dt_device_is_available(iommu_spec.np) ) > + break; > + > + rc = iommu_fwspec_init(dev, &iommu_spec.np->dev); > + if ( rc ) > + break; > + > + /* > + * Provide DT IOMMU specifier which describes the IOMMU master > + * interfaces of that device (device IDs, etc) to the driver. > + * The driver's responsibility is to decide how to interpret them. NIT: "The driver is responsible to decide...". > + * It should also initialize/verify that device. What do you mean? of_xlate should mostly translate the specifier to fwspec. > + */ > + rc = ops->of_xlate(dev, &iommu_spec); > + if ( rc ) > + break; > + > + index++; > + } > + > + /* > + * Add master device to the IOMMU if latter is present and available. > + * The driver's responsibility is to check whether that device > + * was initialized/verified before and mark that device as protected. I don't understand the last sentence. For me, "device" refers to what's pointed by "dev". But the IOMMU driver is not responsible for initializing the device. > + */ > + if ( !rc ) > + rc = ops->add_device(0, dev); > + > + if ( rc < 0 ) > + iommu_fwspec_free(dev); > + > + return rc; > +} > diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h > index 20d865e..e75359c 100644 > --- a/xen/include/asm-arm/iommu.h > +++ b/xen/include/asm-arm/iommu.h > @@ -26,6 +26,17 @@ struct arch_iommu > const struct iommu_ops *iommu_get_ops(void); > void iommu_set_ops(const struct iommu_ops *ops); > > +/* > + * Helper to add master device to the IOMMU using generic IOMMU DT bindings. > + * > + * Return values: > + * 0 : device is protected by an IOMMU > + * <0 : device is not protected by an IOMMU, but must be (error condition) > + * >0 : device doesn't need to be protected by an IOMMU > + * (IOMMU is not enabled/present or device is not connected to it). > + */ > +int iommu_add_dt_device(struct dt_device_node *np); > + > /* mapping helpers */ > int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn, > unsigned int flags, > Cheers,
On 09.09.19 18:04, Julien Grall wrote: > Hi Oleksandr, Hi Julien > > > On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> This patch adds new iommu_add_dt_device API for adding DT device >> to the IOMMU using generic IOMMU DT bindings [1] and previously >> added "iommu_fwspec" support and "add_device/of_xlate" callbacks. >> >> New function does the following: >> - Parse the DT bindings according to the specification >> - Provide DT IOMMU specifier which describes the IOMMU master >> interfaces of that device (device IDs, etc) to the driver >> - Add master device to the IOMMU if latter is present and available >> >> The additional benefit here is to avoid to go through the whole DT >> multiple times in IOMMU driver trying to locate master devices which >> belong to each IOMMU device being probed. > > So the commit title/message describes the new function > iommu_add_dt_device, but not the main important thing i.e. "Why is it > called when building dom0". > > While I agree the new function is the big part of the function what > matter is we need to register device using the generic IOMMU bindings > before assigning the device to a domain. The split is to keep separate > "add" and "assign". The later can be called from dom0. Good point. I will update description. > > >> >> [1] >> https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes V2 -> V3: >> - clarified patch description >> - clarified comments in code >> - modified to provide DT IOMMU specifier to the driver >> using "of_xlate" callback >> - documented function usage >> - modified to return an error if ops is not present/implemented, >> - added ability to return a possitive value to indicate >> that device doesn't need to be protected >> - removed check for the "iommu" property presence >> in the common code >> - included <asm/iommu_fwspec.h> directly >> --- >> xen/arch/arm/domain_build.c | 11 ++++++++ >> xen/drivers/passthrough/arm/iommu.c | 55 >> +++++++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/iommu.h | 11 ++++++++ >> 3 files changed, 77 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index e79d4e2..159ea6a 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -1240,6 +1240,7 @@ static int __init map_device_children(struct >> domain *d, >> /* >> * For a given device node: >> + * - Try to call iommu_add_dt_device to protect the device by an IOMMU >> * - Give permission to the guest to manage IRQ and MMIO range >> * - Retrieve the IRQ configuration (i.e edge/level) from device tree >> * When the device is not marked for guest passthrough: >> @@ -1257,6 +1258,16 @@ static int __init handle_device(struct domain >> *d, struct dt_device_node *dev, >> u64 addr, size; >> bool need_mapping = !dt_device_for_passthrough(dev); >> + dt_dprintk("%s add to iommu\n", dt_node_full_name(dev)); > > This message is slightly confusing. You are not adding the device, you > are trying to. So how about "Check if %s is behind an IOMMU and add it". Sounds reasonable. > > >> + >> + res = iommu_add_dt_device(dev); >> + if ( res < 0 ) >> + { >> + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n", >> + dt_node_full_name(dev)); >> + return res; >> + } >> + >> nirq = dt_number_of_irq(dev); >> naddr = dt_number_of_address(dev); >> diff --git a/xen/drivers/passthrough/arm/iommu.c >> b/xen/drivers/passthrough/arm/iommu.c >> index 72a30e0..47e4bc6 100644 >> --- a/xen/drivers/passthrough/arm/iommu.c >> +++ b/xen/drivers/passthrough/arm/iommu.c >> @@ -20,6 +20,7 @@ >> #include <xen/lib.h> >> #include <asm/device.h> >> +#include <asm/iommu_fwspec.h> >> /* >> * Deferred probe list is used to keep track of devices for which >> driver >> @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct domain >> *d) >> void __hwdom_init arch_iommu_hwdom_init(struct domain *d) >> { >> } >> + >> +int __init iommu_add_dt_device(struct dt_device_node *np) >> +{ >> + const struct iommu_ops *ops = iommu_get_ops(); >> + struct dt_phandle_args iommu_spec; >> + struct device *dev = dt_to_dev(np); >> + int rc = 1, index = 0; >> + >> + if ( !iommu_enabled ) >> + return 1; >> + >> + if ( !ops || !ops->add_device || !ops->of_xlate ) > > The SMMU does not implement of_xlate(). It is actually only mandatory > if you are using the generic bindings. So I would only check > ops->of_xlate if "iommus" exists. Agree. Will do. > > >> + return -EINVAL; >> + >> + if ( dev_iommu_fwspec_get(dev) ) >> + return -EEXIST; > + >> + /* According Documentation/devicetree/bindings/iommu/iommu.txt >> from Linux */ > > s/According/According to/ ok > > >> + while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells", >> + index, &iommu_spec) ) >> + { >> + if ( !dt_device_is_available(iommu_spec.np) ) >> + break; >> + >> + rc = iommu_fwspec_init(dev, &iommu_spec.np->dev); >> + if ( rc ) >> + break; >> + >> + /* >> + * Provide DT IOMMU specifier which describes the IOMMU master >> + * interfaces of that device (device IDs, etc) to the driver. >> + * The driver's responsibility is to decide how to interpret >> them. > > NIT: "The driver is responsible to decide...". ok > >> + * It should also initialize/verify that device. > > What do you mean? of_xlate should mostly translate the specifier to > fwspec. Yes. Saying "initialize/verify that device" I meant to verify passed DT IOMMU specifier and initialize driver's private data for this device (iommu_priv). But, this is obvious. I will remove confusing word "initialize" or even the whole sentence. > > >> + */ >> + rc = ops->of_xlate(dev, &iommu_spec); >> + if ( rc ) >> + break; >> + >> + index++; >> + } >> + >> + /* >> + * Add master device to the IOMMU if latter is present and >> available. >> + * The driver's responsibility is to check whether that device >> + * was initialized/verified before and mark that device as >> protected. > > I don't understand the last sentence. For me, "device" refers to > what's pointed by "dev". But the IOMMU driver is not responsible for > initializing the device. Yes. The same as for comment above. I will remove confusing word "initialize".
Hi, Julien > >>> diff --git a/xen/drivers/passthrough/arm/iommu.c >>> b/xen/drivers/passthrough/arm/iommu.c >>> index 72a30e0..47e4bc6 100644 >>> --- a/xen/drivers/passthrough/arm/iommu.c >>> +++ b/xen/drivers/passthrough/arm/iommu.c >>> @@ -20,6 +20,7 @@ >>> #include <xen/lib.h> >>> #include <asm/device.h> >>> +#include <asm/iommu_fwspec.h> >>> /* >>> * Deferred probe list is used to keep track of devices for which >>> driver >>> @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct >>> domain *d) >>> void __hwdom_init arch_iommu_hwdom_init(struct domain *d) >>> { >>> } >>> + >>> +int __init iommu_add_dt_device(struct dt_device_node *np) >>> +{ >>> + const struct iommu_ops *ops = iommu_get_ops(); >>> + struct dt_phandle_args iommu_spec; >>> + struct device *dev = dt_to_dev(np); >>> + int rc = 1, index = 0; >>> + >>> + if ( !iommu_enabled ) >>> + return 1; >>> + >>> + if ( !ops || !ops->add_device || !ops->of_xlate ) >> >> The SMMU does not implement of_xlate(). It is actually only mandatory >> if you are using the generic bindings. So I would only check >> ops->of_xlate if "iommus" exists. > > Agree. Will do. Just to clarify. What about "ops->add_device", shall I check it if "iommus" exists as well?
On 9/10/19 2:34 PM, Oleksandr wrote: > > Hi, Julien Hi, >> >>>> diff --git a/xen/drivers/passthrough/arm/iommu.c >>>> b/xen/drivers/passthrough/arm/iommu.c >>>> index 72a30e0..47e4bc6 100644 >>>> --- a/xen/drivers/passthrough/arm/iommu.c >>>> +++ b/xen/drivers/passthrough/arm/iommu.c >>>> @@ -20,6 +20,7 @@ >>>> #include <xen/lib.h> >>>> #include <asm/device.h> >>>> +#include <asm/iommu_fwspec.h> >>>> /* >>>> * Deferred probe list is used to keep track of devices for which >>>> driver >>>> @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct >>>> domain *d) >>>> void __hwdom_init arch_iommu_hwdom_init(struct domain *d) >>>> { >>>> } >>>> + >>>> +int __init iommu_add_dt_device(struct dt_device_node *np) >>>> +{ >>>> + const struct iommu_ops *ops = iommu_get_ops(); >>>> + struct dt_phandle_args iommu_spec; >>>> + struct device *dev = dt_to_dev(np); >>>> + int rc = 1, index = 0; >>>> + >>>> + if ( !iommu_enabled ) >>>> + return 1; >>>> + >>>> + if ( !ops || !ops->add_device || !ops->of_xlate ) >>> >>> The SMMU does not implement of_xlate(). It is actually only mandatory >>> if you are using the generic bindings. So I would only check >>> ops->of_xlate if "iommus" exists. >> >> Agree. Will do. > > > Just to clarify. > > What about "ops->add_device", shall I check it if "iommus" exists as well? Yes. Somehow I thought add_device was implemented for the SMMU driver, but I got confused with the Linux IOMMU ops. Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index e79d4e2..159ea6a 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1240,6 +1240,7 @@ static int __init map_device_children(struct domain *d, /* * For a given device node: + * - Try to call iommu_add_dt_device to protect the device by an IOMMU * - Give permission to the guest to manage IRQ and MMIO range * - Retrieve the IRQ configuration (i.e edge/level) from device tree * When the device is not marked for guest passthrough: @@ -1257,6 +1258,16 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, u64 addr, size; bool need_mapping = !dt_device_for_passthrough(dev); + dt_dprintk("%s add to iommu\n", dt_node_full_name(dev)); + + res = iommu_add_dt_device(dev); + if ( res < 0 ) + { + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n", + dt_node_full_name(dev)); + return res; + } + nirq = dt_number_of_irq(dev); naddr = dt_number_of_address(dev); diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c index 72a30e0..47e4bc6 100644 --- a/xen/drivers/passthrough/arm/iommu.c +++ b/xen/drivers/passthrough/arm/iommu.c @@ -20,6 +20,7 @@ #include <xen/lib.h> #include <asm/device.h> +#include <asm/iommu_fwspec.h> /* * Deferred probe list is used to keep track of devices for which driver @@ -139,3 +140,57 @@ int arch_iommu_populate_page_table(struct domain *d) void __hwdom_init arch_iommu_hwdom_init(struct domain *d) { } + +int __init iommu_add_dt_device(struct dt_device_node *np) +{ + const struct iommu_ops *ops = iommu_get_ops(); + struct dt_phandle_args iommu_spec; + struct device *dev = dt_to_dev(np); + int rc = 1, index = 0; + + if ( !iommu_enabled ) + return 1; + + if ( !ops || !ops->add_device || !ops->of_xlate ) + return -EINVAL; + + if ( dev_iommu_fwspec_get(dev) ) + return -EEXIST; + + /* According Documentation/devicetree/bindings/iommu/iommu.txt from Linux */ + while ( !dt_parse_phandle_with_args(np, "iommus", "#iommu-cells", + index, &iommu_spec) ) + { + if ( !dt_device_is_available(iommu_spec.np) ) + break; + + rc = iommu_fwspec_init(dev, &iommu_spec.np->dev); + if ( rc ) + break; + + /* + * Provide DT IOMMU specifier which describes the IOMMU master + * interfaces of that device (device IDs, etc) to the driver. + * The driver's responsibility is to decide how to interpret them. + * It should also initialize/verify that device. + */ + rc = ops->of_xlate(dev, &iommu_spec); + if ( rc ) + break; + + index++; + } + + /* + * Add master device to the IOMMU if latter is present and available. + * The driver's responsibility is to check whether that device + * was initialized/verified before and mark that device as protected. + */ + if ( !rc ) + rc = ops->add_device(0, dev); + + if ( rc < 0 ) + iommu_fwspec_free(dev); + + return rc; +} diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h index 20d865e..e75359c 100644 --- a/xen/include/asm-arm/iommu.h +++ b/xen/include/asm-arm/iommu.h @@ -26,6 +26,17 @@ struct arch_iommu const struct iommu_ops *iommu_get_ops(void); void iommu_set_ops(const struct iommu_ops *ops); +/* + * Helper to add master device to the IOMMU using generic IOMMU DT bindings. + * + * Return values: + * 0 : device is protected by an IOMMU + * <0 : device is not protected by an IOMMU, but must be (error condition) + * >0 : device doesn't need to be protected by an IOMMU + * (IOMMU is not enabled/present or device is not connected to it). + */ +int iommu_add_dt_device(struct dt_device_node *np); + /* mapping helpers */ int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn, unsigned int flags,