|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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;
>> }
>>
>> -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.
>
>> }
>>
>> - return ret ? NULL : ops;
>> + return ret;
>> }
>>
>> +#if 0 /* Xen: We do not need this function for Xen */
>> /**
>> * iort_set_dma_mask - Set-up dma mask for a device.
>> *
>> @@ -567,39 +602,43 @@ void iort_set_dma_mask(struct device *dev)
>> if (!dev->dma_mask)
>> dev->dma_mask = &dev->coherent_dma_mask;
>> }
>> -
>> +#endif
>
> Please avoid dropping newline
>
>> /**
>> - * 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)
>
> I don't think this change is necessary. Returning the iommu_ops here would be
> the best.
>
> [...]
>
>> +/*
>> + * Xen: Not using the parsin ops for now. Need to check and see if it will
>> + * be useful to use these in some form, or let the driver parse IORT node.
>> + */
>> +#if 0
>> static void __init acpi_iort_register_irq(int hwirq, const char *name,
>> int trigger,
>> struct resource *res)
>> @@ -807,93 +852,63 @@ const struct iort_iommu_config
>> *iort_get_iommu_cfg(struct acpi_iort_node *node)
>> return NULL;
>> }
>> }
>> -
>> +#endif
>
> Please avoid dropping newline.
>
>> /**
>> - * iort_add_smmu_platform_device() - Allocate a platform device for SMMU
>> + * Xen: rename the function to iort_add_smmu_device
>
> Renaming the function make more difficult to backport change. So why renaming
> it?
>
> [...]
>
>> -static void __init iort_init_platform_devices(void)
>> +/*
>> + * Xen: Rename the function to iort_init_devices as this function will
>> + * populate the device object for SMMU devices.
>
> Ditto.
>
>> + */
>> +static void __init iort_init_devices(void)
>
> [...]
>
>> diff --git a/xen/drivers/passthrough/arm/iommu.c
>> b/xen/drivers/passthrough/arm/iommu.c
>> index edf70c2..1397da5 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -74,24 +74,26 @@ int arch_iommu_populate_page_table(struct domain *d)
>> return -ENOSYS;
>> }
>>
>> +/*
>> + * The ops parameter in this function will always be NULL for Xen,
>> + * as the ops are set per domain.
>> + */
>> int iommu_fwspec_init(struct device *dev, struct fwnode_handle
>> *iommu_fwnode,
>> const struct iommu_ops *ops)
>> {
>> struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>
>> + /*
>> + * fwspec is already allocated for this device.
>> + */
>> if (fwspec)
>> - return ops == fwspec->ops ? 0 : -EINVAL;
>> + return 0;
>>
>> fwspec = xzalloc(struct iommu_fwspec);
>> if (!fwspec)
>> return -ENOMEM;
>>
>> - /* Ref counting for the dt device node is not needed */
>> -
>> - /*of_node_get(to_of_node(iommu_fwnode));*/
>
> This could should have neever been commented at first hand.
Needed some help on enabling the dt related code. I will try to enable this.
>
>> -
>> fwspec->iommu_fwnode = iommu_fwnode;
>> - fwspec->ops = ops;
>> dev->iommu_fwspec = fwspec;
>> return 0;
>> }
>> @@ -101,7 +103,6 @@ void iommu_fwspec_free(struct device *dev)
>> struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>>
>> if (fwspec) {
>> - /*fwnode_handle_put(fwspec->iommu_fwnode);*/
>
> Ditto.
>
>> xfree(fwspec);
>> dev->iommu_fwspec = NULL;
>> }
>> diff --git a/xen/include/acpi/acpi.h b/xen/include/acpi/acpi.h
>> index c852701..1ac92b2 100644
>> --- a/xen/include/acpi/acpi.h
>> +++ b/xen/include/acpi/acpi.h
>> @@ -60,6 +60,7 @@
>> #include "actbl.h" /* ACPI table definitions */
>> #include "aclocal.h" /* Internal data types */
>> #include "acoutput.h" /* Error output and Debug macros */
>> +#include "acpi_iort.h" /* Utility defines for IORT */
>
> I think this is a pretty bad idea. You don't need to include acpi_iort.h
> everywhere.
True this is needed for arm only. I will include this in arm specific code.
>
>> #include "acpiosxf.h" /* Interfaces to the ACPI-to-OS layer */
>> #include "acpixf.h" /* ACPI core subsystem external interfaces */
>> #include "acglobal.h" /* All global variables */
>> diff --git a/xen/include/acpi/acpi_iort.h b/xen/include/acpi/acpi_iort.h
>> index 77e0809..c0b5b8d 100644
>> --- a/xen/include/acpi/acpi_iort.h
>> +++ b/xen/include/acpi/acpi_iort.h
>> @@ -19,27 +19,35 @@
>> #ifndef __ACPI_IORT_H__
>> #define __ACPI_IORT_H__
>>
>> -#include <linux/acpi.h>
>> -#include <linux/fwnode.h>
>> -#include <linux/irqdomain.h>
>> +#include <xen/acpi.h>
>> +#include <asm/device.h>
>>
>> +/*
>> + * We are not using IORT IRQ bindings for this change set
>> + */
>> +#if 0
>
> Whilst I can understand why we want to ifdef in the *.c. I think it less
> warrant in the header. I would prefer them to either kept or dropped. But not
> #if 0.
>
>> #define IORT_IRQ_MASK(irq) (irq & 0xffffffffULL)
>> #define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xffffffffULL)
>>
>> 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?
>
>> void acpi_iort_init(void);
>> bool iort_node_match(u8 type);
>> void acpi_iort_init(void);
>> bool iort_node_match(u8 type);
>> +#if 0
>> u32 iort_msi_map_rid(struct device *dev, u32 req_id);
>> struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
>> /* IOMMU interface */
>> void iort_set_dma_mask(struct device *dev);
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
>> +#endif
>> +acpi_status iort_iommu_configure(struct device *dev);
>> #else
>> static inline void acpi_iort_init(void) { }
>> static inline bool iort_node_match(u8 type) { return false; }
>> +#if 0
>> static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
>> { return req_id; }
>> static inline struct irq_domain *iort_get_device_domain(struct device *dev,
>> @@ -47,12 +55,11 @@ static inline struct irq_domain
>> *iort_get_device_domain(struct device *dev,
>> { return NULL; }
>> /* IOMMU interface */
>> static inline void iort_set_dma_mask(struct device *dev) { }
>> +#endif
>> static inline
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>> -{ return NULL; }
>> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
>> +{ return AE_NOT_IMPLEMENTED; }
>> #endif
>>
>> +#if 0
>> u32 iort_msi_map_rid(struct device *dev, u32 req_id);
>> struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
>> /* IOMMU interface */
>> void iort_set_dma_mask(struct device *dev);
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev);
>> +#endif
>> +acpi_status iort_iommu_configure(struct device *dev);
>> #else
>> static inline void acpi_iort_init(void) { }
>> static inline bool iort_node_match(u8 type) { return false; }
>> +#if 0
>> static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
>> { return req_id; }
>> static inline struct irq_domain *iort_get_device_domain(struct device *dev,
>> @@ -47,12 +55,11 @@ static inline struct irq_domain
>> *iort_get_device_domain(struct device *dev,
>> { return NULL; }
>> /* IOMMU interface */
>> static inline void iort_set_dma_mask(struct device *dev) { }
>> +#endif
>> static inline
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>> -{ return NULL; }
>> +acpi_status iommu_ops iort_iommu_configure(struct device *dev)
>> +{ return AE_NOT_IMPLEMENTED; }
>> #endif
>>
>> -#define IORT_ACPI_DECLARE(name, table_id, fn) \
>> - ACPI_DECLARE_PROBE_ENTRY(iort, name, table_id, 0, NULL, 0, fn)
>
> I think we whould need something similar for Xen.
Yes. I have defined something similar to what you have defined for DT in smmu
driver.
>
> [...]
>
>> 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 <asm/bug.h>
>>
>> #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); \
>> +})
>
> hmmmm. 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.
>
>>
>> #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>> /* Force a compilation error if condition is true */
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index 59b6e8a..c518569 100644
>> --- 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;
>
> struct device does not exist on x86. If you still want to add it, then you
> should do it in a separate patch and CC relevant maintainers.
Ok, I will go ahead and do that.
>
>> };
>>
>> #define for_each_pdev(domain, pdev) \
>>
>
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |