Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
Hi Sameer, On 21/09/17 01:37, Goel, Sameer wrote: [...] diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 995a85a..3785fae 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -9,7 +9,12 @@ #include #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0) +#define WARN_ON(p) ({ \ +int __ret_warn_on = !!(p); \ +if (unlikely(__ret_warn_on)) \ +WARN();\ +unlikely(__ret_warn_on); \ +}) h. Why this change? Was getting a compilation error when I was using WARN_ON as a conditional in an if statement regarding the return value. So removed the loop. This looks similar to Linux now. This should belong to a separate patch with a commit message explaining why the change. Agreed. Since this is more of a Linux compat function, I do not want to change the system-wide behavior. I will move this in the IORT code. I don't see any reason to keep this implementation only for IORT. It would be possible to have similar construction in Xen. So I would first try to see if maintainers would be willing to accept a system-wide change before thinking to re-implement WARN_ON in IORT. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
On 9/12/2017 5:25 AM, Julien Grall wrote: > Hi Sameer, > > On 28/08/17 23:21, Goel, Sameer wrote: >> On 6/12/2017 7:24 AM, Julien Grall wrote: static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, struct fwnode_handle *fwnode, const struct iommu_ops *ops) @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, return ret; } -static const struct iommu_ops *iort_iommu_xlate(struct device *dev, - struct acpi_iort_node *node, - u32 streamid) +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node, + u32 streamid) { - const struct iommu_ops *ops = NULL; int ret = -ENODEV; struct fwnode_handle *iort_fwnode; if (node) { iort_fwnode = iort_get_fwnode(node); if (!iort_fwnode) - return NULL; - - ops = iommu_ops_from_fwnode(iort_fwnode); - if (!ops) - return NULL; + return ret; - ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops); + ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL); >>> >>> Why don't you get the IOMMU ops here? This would avoid unecessary change. >> From the linux definition it seems that there will be eventually used to >> override the ops >> set by the bus. I did not find a use for this here, so removed it to >> simplify code. I can >> add these back, but see this as dead code. > > You will always have dead code if you import code as it is from Linux. This > is the price to pay to help rebase the code in the future. > > In that specific case, I think return the ops is the right thing to do. > Potentially it will allow us to support different IOMMU at the same time. > Agreed. > [...] > #define IORT_IRQ_MASK(irq) (irq & 0xULL) #define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xULL) int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node); void iort_deregister_domain_token(int trans_id); struct fwnode_handle *iort_find_domain_token(int trans_id); -#ifdef CONFIG_ACPI_IORT +#endif + +#ifdef CONFIG_ARM_64 >>> >>> Why #ifdef CONFIG_ARM_64? >> Was trying to keep the impact low for this RFC iteration (My use-case is >> arm64 only). Looking for the right recommendation? > > IORT is not specific to ARM64. Even though ACPI is only supported for ARM64 > on Xen today, we try to keep the code as generic as possible. > > In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when ACPI is > enabled. Agreed. I will add the new config in the next patch set. > > [...] > >>> >>> [...] >>> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 995a85a..3785fae 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -9,7 +9,12 @@ #include #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0) +#define WARN_ON(p) ({ \ + int __ret_warn_on = !!(p); \ + if (unlikely(__ret_warn_on)) \ + WARN(); \ + unlikely(__ret_warn_on); \ +}) >>> >>> h. Why this change? >> Was getting a compilation error when I was using WARN_ON as a conditional >> in an if statement regarding the return value. So removed the loop. This >> looks similar to Linux now. > > This should belong to a separate patch with a commit message explaining why > the change. Agreed. Since this is more of a Linux compat function, I do not want to change the system-wide behavior. I will move this in the IORT code. > > Cheers, > -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
Hi Sameer, On 28/08/17 23:21, Goel, Sameer wrote: On 6/12/2017 7:24 AM, Julien Grall wrote: static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, struct fwnode_handle *fwnode, const struct iommu_ops *ops) @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, return ret; } -static const struct iommu_ops *iort_iommu_xlate(struct device *dev, -struct acpi_iort_node *node, -u32 streamid) +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node, +u32 streamid) { -const struct iommu_ops *ops = NULL; int ret = -ENODEV; struct fwnode_handle *iort_fwnode; if (node) { iort_fwnode = iort_get_fwnode(node); if (!iort_fwnode) -return NULL; - -ops = iommu_ops_from_fwnode(iort_fwnode); -if (!ops) -return NULL; +return ret; -ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops); +ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL); Why don't you get the IOMMU ops here? This would avoid unecessary change. From the linux definition it seems that there will be eventually used to override the ops set by the bus. I did not find a use for this here, so removed it to simplify code. I can add these back, but see this as dead code. You will always have dead code if you import code as it is from Linux. This is the price to pay to help rebase the code in the future. In that specific case, I think return the ops is the right thing to do. Potentially it will allow us to support different IOMMU at the same time. [...] #define IORT_IRQ_MASK(irq)(irq & 0xULL) #define IORT_IRQ_TRIGGER_MASK(irq)((irq >> 32) & 0xULL) int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node); void iort_deregister_domain_token(int trans_id); struct fwnode_handle *iort_find_domain_token(int trans_id); -#ifdef CONFIG_ACPI_IORT +#endif + +#ifdef CONFIG_ARM_64 Why #ifdef CONFIG_ARM_64? Was trying to keep the impact low for this RFC iteration (My use-case is arm64 only). Looking for the right recommendation? IORT is not specific to ARM64. Even though ACPI is only supported for ARM64 on Xen today, we try to keep the code as generic as possible. In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when ACPI is enabled. [...] [...] diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 995a85a..3785fae 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -9,7 +9,12 @@ #include #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0) +#define WARN_ON(p) ({ \ +int __ret_warn_on = !!(p); \ +if (unlikely(__ret_warn_on)) \ +WARN();\ +unlikely(__ret_warn_on); \ +}) h. Why this change? Was getting a compilation error when I was using WARN_ON as a conditional in an if statement regarding the return value. So removed the loop. This looks similar to Linux now. This should belong to a separate patch with a commit message explaining why the change. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
On 6/12/2017 7:24 AM, Julien Grall wrote: > Hi Sameer, > > On 08/06/17 20:30, Sameer Goel wrote: >> Add limited support for parsing IORT table to initialize SMMU devices. > > It would be nice to explain what you actually support in the commit message. > > [...] > >> >> #define IORT_TYPE_MASK(type) (1 << (type)) >> #define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP) >> #define IORT_IOMMU_TYPE ((1 << ACPI_IORT_NODE_SMMU) | \ >> (1 << ACPI_IORT_NODE_SMMU_V3)) >> >> +#if 0 >> struct iort_its_msi_chip { >> struct list_head list; >> struct fwnode_handle *fw_node; >> u32 translation_id; >> }; >> >> +#endif >> + > > Why cannot you enable MSI now? Actually this would allow us to know whether > we should import the code from Linux or reimplement or own. Well was not too sure about how this will fit in, so was leaving this for next iteration. I will try to enable this. > >> struct iort_fwnode { >> struct list_head list; >> struct acpi_iort_node *iort_node; >> @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node >> *iort_node, >> { >> struct iort_fwnode *np; >> >> - np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC); >> + np = xzalloc(struct iort_fwnode); > > If we decide to keep this code close to Linux you need to: > - avoid replacing Linux name by Xen name as much as possible. Instead use > macro > - explain above each change why you need then > > See what I did for the ARM SMMU. Agreed > >> >> if (WARN_ON(!np)) >> return -ENOMEM; >> @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct >> acpi_iort_node *node) >> list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) { >> if (curr->iort_node == node) { >> list_del(&curr->list); >> - kfree(curr); >> + xfree(curr); >> break; >> } >> } >> @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback) >> /* Root pointer to the mapped IORT table */ >> static struct acpi_table_header *iort_table; >> >> +#if 0 >> static LIST_HEAD(iort_msi_chip_list); >> static DEFINE_SPINLOCK(iort_msi_chip_lock); >> >> @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int >> trans_id) >> >> return fw_node; >> } >> - >> +#endif > > Please avoid dropping newline. > >> static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type, >> iort_find_node_callback callback, >> void *context) >> @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum >> acpi_iort_node_type type, >> iort_table->length); >> >> for (i = 0; i < iort->node_count; i++) { >> - if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND, >> - "IORT node pointer overflows, bad table!\n")) >> + if (iort_node >= iort_end) { >> + printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n"); >> return NULL; >> + } >> >> if (iort_node->type == type && >> ACPI_SUCCESS(callback(iort_node, context))) >> @@ -249,6 +262,14 @@ bool iort_node_match(u8 type) >> return node != NULL; >> } >> >> +/* >> + * Following 2 definies should come from the PCI passthrough implementation. >> + * Based on the current pci_dev define the bus number and seg number come >> + * from pci_dev so making an API assumption >> + */ >> +#define to_pci_dev(p) container_of(p, struct pci_dev,dev) >> +#define pci_domain_nr(dev) dev->seg > > This should go in pci.h. > >> + >> static acpi_status iort_match_node_callback(struct acpi_iort_node *node, >> void *context) >> { >> @@ -256,6 +277,11 @@ static acpi_status iort_match_node_callback(struct >> acpi_iort_node *node, >> acpi_status status; >> >> if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) { >> + status = AE_NOT_IMPLEMENTED; >> +/* >> + * Named components not supported yet. > > Can you expand? Were not needed for now, but I agree we can enable the code. > > [...] > >> +/* >> + * RID is the same as PCI_DEVID(BDF) for QDF2400 > > Xen is meant to be agnostic to the platform. Rather than making assumption, > we should discuss on way to get the RID. I will answer on Robin's mail about > it. > >> + */ >> static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) >> { >> u32 *rid = data; >> @@ -510,7 +550,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 >> alias, void *data) >> *rid = alias; >> return 0; >> } >> - >> +#endif > > Please avoid dropping newline > >> static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, >> struct fwnode_handle *fwnode, >> const struct iommu_ops *ops) >> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 >> streamid, >> return ret; >> } >
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
On 6/9/2017 5:15 AM, Robin Murphy wrote: > On 08/06/17 20:30, Sameer Goel wrote: > [...] >> /** >> - * iort_iommu_configure - Set-up IOMMU configuration for a device. >> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This >> + * function sets up the fwspec as needed for a given device. Only PCI >> + * devices are supported for now. >> * >> * @dev: device to configure >> * >> - * Returns: iommu_ops pointer on configuration success >> - * NULL on configuration failure >> + * Returns: Appropriate acpi_status >> */ >> -const struct iommu_ops *iort_iommu_configure(struct device *dev) >> +acpi_status iort_iommu_configure(struct device *dev) >> { >> struct acpi_iort_node *node, *parent; >> -const struct iommu_ops *ops = NULL; >> u32 streamid = 0; >> +acpi_status status = AE_OK; >> >> if (dev_is_pci(dev)) { >> -struct pci_bus *bus = to_pci_dev(dev)->bus; >> +struct pci_dev *pci_device = to_pci_dev(dev); >> u32 rid; >> >> -pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, >> - &rid); >> +rid = PCI_BDF2(pci_device->bus,pci_device->devfn); > > Beware that the Linux code isn't actually correct to begin with[1]. I > don't know how much Xen deals with PCI bridges and quirks, but as it > stands you should be able to trivially expose the flaw here by plugging > in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs > (or even both) kick up stream table faults. > Appreciate the feedback Robin. I will try to incorporate the new IORT changes when I release the first patch set. > Robin. > > [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html > >> >> node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, >> - iort_match_node_callback, &bus->dev); >> + iort_match_node_callback, dev); >> if (!node) >> -return NULL; >> +return AE_NOT_FOUND; >> >> parent = iort_node_map_rid(node, rid, &streamid, >> IORT_IOMMU_TYPE); >> >> -ops = iort_iommu_xlate(dev, parent, streamid); >> +status = iort_iommu_xlate(dev, parent, streamid); >> + >> +status = status ? AE_ERROR : AE_OK; >> >> } else { >> +status = AE_NOT_IMPLEMENTED; >> +#if 0 >> int i = 0; >> >> node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT, >> @@ -616,11 +655,17 @@ const struct iommu_ops *iort_iommu_configure(struct >> device *dev) >> parent = iort_node_get_id(node, &streamid, >>IORT_IOMMU_TYPE, i++); >> } >> +#endif >> } >> >> -return ops; >> +return status; >> } > -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
>>> On 08.06.17 at 21:30, wrote: > Add limited support for parsing IORT table to initialize SMMU devices. > > Signed-off-by: Sameer Goel > --- > xen/arch/arm/setup.c| 3 + > xen/drivers/acpi/Makefile | 1 + > xen/drivers/acpi/arm/Makefile | 1 + > xen/drivers/acpi/arm/iort.c | 232 > +++- With the amount of changes done to this file I question even more the value of first pulling in the plain Linux commits. > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -9,7 +9,12 @@ > #include > > #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) > -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0) > +#define WARN_ON(p) ({ \ > +int __ret_warn_on = !!(p); \ > +if (unlikely(__ret_warn_on)) \ > +WARN();\ > +unlikely(__ret_warn_on); \ > +}) This has nothing to do with the intention of the patch. If you want WARN_ON()s behavior to change, please submit a separate patch doing just that. > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -88,6 +88,7 @@ struct pci_dev { > #define PT_FAULT_THRESHOLD 10 > } fault; > u64 vf_rlen[6]; > +struct device dev; Why? Please rationalize your changes in the patch description (and perhaps split them). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
On 12/06/17 14:44, Jan Beulich wrote: On 12.06.17 at 15:36, wrote: >> On 09/06/17 12:15, Robin Murphy wrote: >>> On 08/06/17 20:30, Sameer Goel wrote: >>> [...] /** - * iort_iommu_configure - Set-up IOMMU configuration for a device. + * iort_iommu_configure - Set-up IOMMU configuration for a device. This + * function sets up the fwspec as needed for a given device. Only PCI + * devices are supported for now. * * @dev: device to configure * - * Returns: iommu_ops pointer on configuration success - * NULL on configuration failure + * Returns: Appropriate acpi_status */ -const struct iommu_ops *iort_iommu_configure(struct device *dev) +acpi_status iort_iommu_configure(struct device *dev) { struct acpi_iort_node *node, *parent; - const struct iommu_ops *ops = NULL; u32 streamid = 0; + acpi_status status = AE_OK; if (dev_is_pci(dev)) { - struct pci_bus *bus = to_pci_dev(dev)->bus; + struct pci_dev *pci_device = to_pci_dev(dev); u32 rid; - pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, - &rid); + rid = PCI_BDF2(pci_device->bus,pci_device->devfn); >>> >>> Beware that the Linux code isn't actually correct to begin with[1]. I >>> don't know how much Xen deals with PCI bridges and quirks, but as it >>> stands you should be able to trivially expose the flaw here by plugging >>> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs >>> (or even both) kick up stream table faults. >>> >>> Robin. >>> >>> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html >> >> I am not sure how x86 is handling that. The closest I can find would be >> domain_context_mapping. I have CCed x86 folks to get more feedback here. > > I'm lacking enough context to know whether this is the issue > with what we call phantom devices, or something else. Phantom functions, PCIe-PCI bridges, basically anything that can result in traffic from a single endpoint appearing on multiple RIDs, or a RID other than the device's BDF, at the root complex (and IOMMUs/MSI doorbells beyond). Robin. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
>>> On 12.06.17 at 15:36, wrote: > On 09/06/17 12:15, Robin Murphy wrote: >> On 08/06/17 20:30, Sameer Goel wrote: >> [...] >>> /** >>> - * iort_iommu_configure - Set-up IOMMU configuration for a device. >>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This >>> + * function sets up the fwspec as needed for a given device. Only PCI >>> + * devices are supported for now. >>> * >>> * @dev: device to configure >>> * >>> - * Returns: iommu_ops pointer on configuration success >>> - * NULL on configuration failure >>> + * Returns: Appropriate acpi_status >>> */ >>> -const struct iommu_ops *iort_iommu_configure(struct device *dev) >>> +acpi_status iort_iommu_configure(struct device *dev) >>> { >>> struct acpi_iort_node *node, *parent; >>> - const struct iommu_ops *ops = NULL; >>> u32 streamid = 0; >>> + acpi_status status = AE_OK; >>> >>> if (dev_is_pci(dev)) { >>> - struct pci_bus *bus = to_pci_dev(dev)->bus; >>> + struct pci_dev *pci_device = to_pci_dev(dev); >>> u32 rid; >>> >>> - pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, >>> - &rid); >>> + rid = PCI_BDF2(pci_device->bus,pci_device->devfn); >> >> Beware that the Linux code isn't actually correct to begin with[1]. I >> don't know how much Xen deals with PCI bridges and quirks, but as it >> stands you should be able to trivially expose the flaw here by plugging >> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs >> (or even both) kick up stream table faults. >> >> Robin. >> >> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html > > I am not sure how x86 is handling that. The closest I can find would be > domain_context_mapping. I have CCed x86 folks to get more feedback here. I'm lacking enough context to know whether this is the issue with what we call phantom devices, or something else. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
(CC x86 folks) Hi, On 09/06/17 12:15, Robin Murphy wrote: On 08/06/17 20:30, Sameer Goel wrote: [...] /** - * iort_iommu_configure - Set-up IOMMU configuration for a device. + * iort_iommu_configure - Set-up IOMMU configuration for a device. This + * function sets up the fwspec as needed for a given device. Only PCI + * devices are supported for now. * * @dev: device to configure * - * Returns: iommu_ops pointer on configuration success - * NULL on configuration failure + * Returns: Appropriate acpi_status */ -const struct iommu_ops *iort_iommu_configure(struct device *dev) +acpi_status iort_iommu_configure(struct device *dev) { struct acpi_iort_node *node, *parent; - const struct iommu_ops *ops = NULL; u32 streamid = 0; + acpi_status status = AE_OK; if (dev_is_pci(dev)) { - struct pci_bus *bus = to_pci_dev(dev)->bus; + struct pci_dev *pci_device = to_pci_dev(dev); u32 rid; - pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, - &rid); + rid = PCI_BDF2(pci_device->bus,pci_device->devfn); Beware that the Linux code isn't actually correct to begin with[1]. I don't know how much Xen deals with PCI bridges and quirks, but as it stands you should be able to trivially expose the flaw here by plugging in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs (or even both) kick up stream table faults. Robin. [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html I am not sure how x86 is handling that. The closest I can find would be domain_context_mapping. I have CCed x86 folks to get more feedback here. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
Hi Sameer, On 08/06/17 20:30, Sameer Goel wrote: Add limited support for parsing IORT table to initialize SMMU devices. It would be nice to explain what you actually support in the commit message. [...] #define IORT_TYPE_MASK(type) (1 << (type)) #define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP) #define IORT_IOMMU_TYPE((1 << ACPI_IORT_NODE_SMMU) | \ (1 << ACPI_IORT_NODE_SMMU_V3)) +#if 0 struct iort_its_msi_chip { struct list_headlist; struct fwnode_handle*fw_node; u32 translation_id; }; +#endif + Why cannot you enable MSI now? Actually this would allow us to know whether we should import the code from Linux or reimplement or own. struct iort_fwnode { struct list_head list; struct acpi_iort_node *iort_node; @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node *iort_node, { struct iort_fwnode *np; - np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC); + np = xzalloc(struct iort_fwnode); If we decide to keep this code close to Linux you need to: - avoid replacing Linux name by Xen name as much as possible. Instead use macro - explain above each change why you need then See what I did for the ARM SMMU. if (WARN_ON(!np)) return -ENOMEM; @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node) list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) { if (curr->iort_node == node) { list_del(&curr->list); - kfree(curr); + xfree(curr); break; } } @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback) /* Root pointer to the mapped IORT table */ static struct acpi_table_header *iort_table; +#if 0 static LIST_HEAD(iort_msi_chip_list); static DEFINE_SPINLOCK(iort_msi_chip_lock); @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id) return fw_node; } - +#endif Please avoid dropping newline. static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type, iort_find_node_callback callback, void *context) @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type, iort_table->length); for (i = 0; i < iort->node_count; i++) { - if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND, - "IORT node pointer overflows, bad table!\n")) + if (iort_node >= iort_end) { + printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n"); return NULL; + } if (iort_node->type == type && ACPI_SUCCESS(callback(iort_node, context))) @@ -249,6 +262,14 @@ bool iort_node_match(u8 type) return node != NULL; } +/* + * Following 2 definies should come from the PCI passthrough implementation. + * Based on the current pci_dev define the bus number and seg number come + * from pci_dev so making an API assumption + */ +#define to_pci_dev(p) container_of(p, struct pci_dev,dev) +#define pci_domain_nr(dev) dev->seg This should go in pci.h. + static acpi_status iort_match_node_callback(struct acpi_iort_node *node, void *context) { @@ -256,6 +277,11 @@ static acpi_status iort_match_node_callback(struct acpi_iort_node *node, acpi_status status; if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) { + status = AE_NOT_IMPLEMENTED; +/* + * Named components not supported yet. Can you expand? [...] +/* + * RID is the same as PCI_DEVID(BDF) for QDF2400 Xen is meant to be agnostic to the platform. Rather than making assumption, we should discuss on way to get the RID. I will answer on Robin's mail about it. + */ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) { u32 *rid = data; @@ -510,7 +550,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data) *rid = alias; return 0; } - +#endif Please avoid dropping newline static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, struct fwnode_handle *fwnode, const struct iommu_ops *ops) @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid, return ret; } -static const struct iommu_ops *iort_iommu_xlate(struct device *dev, - struct acpi_iort_node *node, - u32 streamid) +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node, +
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
On 08/06/17 20:30, Sameer Goel wrote: [...] > /** > - * iort_iommu_configure - Set-up IOMMU configuration for a device. > + * iort_iommu_configure - Set-up IOMMU configuration for a device. This > + * function sets up the fwspec as needed for a given device. Only PCI > + * devices are supported for now. > * > * @dev: device to configure > * > - * Returns: iommu_ops pointer on configuration success > - * NULL on configuration failure > + * Returns: Appropriate acpi_status > */ > -const struct iommu_ops *iort_iommu_configure(struct device *dev) > +acpi_status iort_iommu_configure(struct device *dev) > { > struct acpi_iort_node *node, *parent; > - const struct iommu_ops *ops = NULL; > u32 streamid = 0; > + acpi_status status = AE_OK; > > if (dev_is_pci(dev)) { > - struct pci_bus *bus = to_pci_dev(dev)->bus; > + struct pci_dev *pci_device = to_pci_dev(dev); > u32 rid; > > - pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid, > -&rid); > + rid = PCI_BDF2(pci_device->bus,pci_device->devfn); Beware that the Linux code isn't actually correct to begin with[1]. I don't know how much Xen deals with PCI bridges and quirks, but as it stands you should be able to trivially expose the flaw here by plugging in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs (or even both) kick up stream table faults. Robin. [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html > > node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, > - iort_match_node_callback, &bus->dev); > + iort_match_node_callback, dev); > if (!node) > - return NULL; > + return AE_NOT_FOUND; > > parent = iort_node_map_rid(node, rid, &streamid, > IORT_IOMMU_TYPE); > > - ops = iort_iommu_xlate(dev, parent, streamid); > + status = iort_iommu_xlate(dev, parent, streamid); > + > + status = status ? AE_ERROR : AE_OK; > > } else { > + status = AE_NOT_IMPLEMENTED; > +#if 0 > int i = 0; > > node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT, > @@ -616,11 +655,17 @@ const struct iommu_ops *iort_iommu_configure(struct > device *dev) > parent = iort_node_get_id(node, &streamid, > IORT_IOMMU_TYPE, i++); > } > +#endif > } > > - return ops; > + return status; > } ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
CCing Jan On Thu, 8 Jun 2017, Sameer Goel wrote: > Add limited support for parsing IORT table to initialize SMMU devices. > > Signed-off-by: Sameer Goel > --- > xen/arch/arm/setup.c| 3 + > xen/drivers/acpi/Makefile | 1 + > xen/drivers/acpi/arm/Makefile | 1 + > xen/drivers/acpi/arm/iort.c | 232 > +++- > xen/drivers/passthrough/arm/iommu.c | 15 +-- > xen/include/acpi/acpi.h | 1 + > xen/include/acpi/acpi_iort.h| 25 ++-- > xen/include/asm-arm/device.h| 2 + > xen/include/xen/acpi.h | 21 > xen/include/xen/lib.h | 7 +- > xen/include/xen/pci.h | 1 + > 11 files changed, 184 insertions(+), 125 deletions(-) > create mode 100644 xen/drivers/acpi/arm/Makefile This patch doesn't apply with "git am" and doesn't build on x86 with: In file included from /local/repos/xen-upstream/xen/include/xen/iommu.h:24:0, from /local/repos/xen-upstream/xen/include/asm/hvm/domain.h:23, from /local/repos/xen-upstream/xen/include/asm/domain.h:7, from /local/repos/xen-upstream/xen/include/xen/domain.h:8, from /local/repos/xen-upstream/xen/include/xen/sched.h:11, from x86_64/asm-offsets.c:9: /local/repos/xen-upstream/xen/include/xen/pci.h:91:19: error: field ‘dev’ has incomplete type struct device dev; ^ In file included from /local/repos/xen-upstream/xen/include/acpi/acpi.h:63:0, from /local/repos/xen-upstream/xen/include/xen/acpi.h:33, from /local/repos/xen-upstream/xen/include/asm/fixmap.h:21, from x86_64/asm-offsets.c:12: /local/repos/xen-upstream/xen/include/acpi/acpi_iort.h:60:23: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘iort_iommu_configure’ acpi_status iommu_ops iort_iommu_configure(struct device *dev) ^ make[3]: *** [asm-offsets.s] Error 1 And it doesn't build on arm64 with: /local/repos/xen-upstream/xen/arch/arm/setup.c:765: undefined reference to `acpi_iort_init' /local/repos/gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu/bin/aarch64-linux-gnu-ld: /local/repos/xen-upstream/xen/.xen-syms.0: hidden symbol `acpi_iort_init' isn't defined /local/repos/gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu/bin/aarch64-linux-gnu-ld: final link failed: Bad value make[3]: *** [/local/repos/xen-upstream/xen/xen-syms] Error 1 > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 92a2de6..5dc93ff 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -753,6 +753,9 @@ void __init start_xen(unsigned long boot_phys_offset, > /* Parse the ACPI tables for possible boot-time configuration */ > acpi_boot_table_init(); > > +/* Initialize the IORT tables */ > +acpi_iort_init(); > + > if ( acpi_disabled ) > printk("Booting using Device Tree\n"); > else > diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile > index 444b11d..4165318 100644 > --- a/xen/drivers/acpi/Makefile > +++ b/xen/drivers/acpi/Makefile > @@ -1,5 +1,6 @@ > subdir-y += tables > subdir-y += utilities > +subdir-$(CONFIG_ARM_64) += arm > subdir-$(CONFIG_X86) += apei > > obj-bin-y += tables.init.o > diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile > new file mode 100644 > index 000..7c039bb > --- /dev/null > +++ b/xen/drivers/acpi/arm/Makefile > @@ -0,0 +1 @@ > +obj-y += iort.o > diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c > index 4a5bb96..c22ec31 100644 > --- a/xen/drivers/acpi/arm/iort.c > +++ b/xen/drivers/acpi/arm/iort.c > @@ -14,29 +14,40 @@ > * This file implements early detection/parsing of I/O mapping > * reported to OS through firmware via I/O Remapping Table (IORT) > * IORT document number: ARM DEN 0049A > + * > + * Based on Linux drivers/acpi/arm64/iort.c > + * => commit ca78d3173cff3503bcd15723b049757f75762d15 > + * > + * Xen modification: > + * Sameer Goel > + * Copyright (C) 2017, The Linux Foundation, All rights reserved. > + * > */ > > -#define pr_fmt(fmt) "ACPI: IORT: " fmt > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > > -#include > -#include > -#include > -#include > -#include > -#include > -#include > > #define IORT_TYPE_MASK(type) (1 << (type)) > #define IORT_MSI_TYPE(1 << ACPI_IORT_NODE_ITS_GROUP) > #define IORT_IOMMU_TYPE ((1 << ACPI_IORT_NODE_SMMU) | \ > (1 << ACPI_IORT_NODE_SMMU_V3)) > > +#if 0 > struct iort_its_msi_chip { > struct list_headlist; > struct fwnode_handle*fw_node; > u32 translation_id; > }; > > +#endif > + > struct iort_fwnode { > struct list_head list; > struct acpi_iort_node *iort_node; > @@ -60,7 +7
[Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table
Add limited support for parsing IORT table to initialize SMMU devices. Signed-off-by: Sameer Goel --- xen/arch/arm/setup.c| 3 + xen/drivers/acpi/Makefile | 1 + xen/drivers/acpi/arm/Makefile | 1 + xen/drivers/acpi/arm/iort.c | 232 +++- xen/drivers/passthrough/arm/iommu.c | 15 +-- xen/include/acpi/acpi.h | 1 + xen/include/acpi/acpi_iort.h| 25 ++-- xen/include/asm-arm/device.h| 2 + xen/include/xen/acpi.h | 21 xen/include/xen/lib.h | 7 +- xen/include/xen/pci.h | 1 + 11 files changed, 184 insertions(+), 125 deletions(-) create mode 100644 xen/drivers/acpi/arm/Makefile diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 92a2de6..5dc93ff 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -753,6 +753,9 @@ void __init start_xen(unsigned long boot_phys_offset, /* Parse the ACPI tables for possible boot-time configuration */ acpi_boot_table_init(); +/* Initialize the IORT tables */ +acpi_iort_init(); + if ( acpi_disabled ) printk("Booting using Device Tree\n"); else diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile index 444b11d..4165318 100644 --- a/xen/drivers/acpi/Makefile +++ b/xen/drivers/acpi/Makefile @@ -1,5 +1,6 @@ subdir-y += tables subdir-y += utilities +subdir-$(CONFIG_ARM_64) += arm subdir-$(CONFIG_X86) += apei obj-bin-y += tables.init.o diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile new file mode 100644 index 000..7c039bb --- /dev/null +++ b/xen/drivers/acpi/arm/Makefile @@ -0,0 +1 @@ +obj-y += iort.o diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c index 4a5bb96..c22ec31 100644 --- a/xen/drivers/acpi/arm/iort.c +++ b/xen/drivers/acpi/arm/iort.c @@ -14,29 +14,40 @@ * This file implements early detection/parsing of I/O mapping * reported to OS through firmware via I/O Remapping Table (IORT) * IORT document number: ARM DEN 0049A + * + * Based on Linux drivers/acpi/arm64/iort.c + * => commit ca78d3173cff3503bcd15723b049757f75762d15 + * + * Xen modification: + * Sameer Goel + * Copyright (C) 2017, The Linux Foundation, All rights reserved. + * */ -#define pr_fmt(fmt)"ACPI: IORT: " fmt +#include +#include +#include +#include +#include +#include + +#include -#include -#include -#include -#include -#include -#include -#include #define IORT_TYPE_MASK(type) (1 << (type)) #define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP) #define IORT_IOMMU_TYPE((1 << ACPI_IORT_NODE_SMMU) | \ (1 << ACPI_IORT_NODE_SMMU_V3)) +#if 0 struct iort_its_msi_chip { struct list_headlist; struct fwnode_handle*fw_node; u32 translation_id; }; +#endif + struct iort_fwnode { struct list_head list; struct acpi_iort_node *iort_node; @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node *iort_node, { struct iort_fwnode *np; - np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC); + np = xzalloc(struct iort_fwnode); if (WARN_ON(!np)) return -ENOMEM; @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node *node) list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list) { if (curr->iort_node == node) { list_del(&curr->list); - kfree(curr); + xfree(curr); break; } } @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback) /* Root pointer to the mapped IORT table */ static struct acpi_table_header *iort_table; +#if 0 static LIST_HEAD(iort_msi_chip_list); static DEFINE_SPINLOCK(iort_msi_chip_lock); @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id) return fw_node; } - +#endif static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type, iort_find_node_callback callback, void *context) @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type, iort_table->length); for (i = 0; i < iort->node_count; i++) { - if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND, - "IORT node pointer overflows, bad table!\n")) + if (iort_node >= iort_end) { + printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n"); return NULL; + } if (iort_node->type == type && ACPI_SUCCESS(callback(iort_node, context))) @@ -249,6 +262,14 @@ bool iort_node_match