Re: [Xen-devel] [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 03/21/2018 09:34 AM, Manish Jaggi wrote:
> On 03/21/2018 02:59 PM, Julien Grall wrote:
> > Hi Manish,
> Hi Julien,
> > On 03/13/2018 03:20 PM, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> > > From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>
> > >
> > > IORT has a hierarchical structure containing PCIRC nodes, IORT nodes
> > > and SMMU nodes. Each node has with it an array of ids and a mapping
> > > which maps a range of ids to another node's ids.
> > > PCIRC(requesterid)->SMMU(streamid)->ITS(devid) or PCIRC->ITS
> > >
> > > IORT is parsed multiple times when streamid(sid) / deviceid(devid)
> > > is queried from requesterid (rid).
> > >
> > > Xen needs to prepare IORT for hardware domain which might again
> > > require parsing. Thus it is prudent to parse IORT once and save
> > > mapping information into individual maps namely rid-sid rid-devid.
> > >
> > > This patch provides API to add a new mapping and query sid/devid based
> > > on rid. Two lists are created rid-sid list, rid-devid list.
> > > rid-devid list forms the basis of hardware domains' IORT.
> > Thank you for updating the commit message. However, you stil don't 
> > give an idea often those function will get called and whether unsorted 
> > list will be fine.
> I have added that in code comments.
Mind to show where?

[...]

> > > +#include <asm/acpi/ridmap.h>
> > > +#include <xen/iommu.h>
> > > +#include <xen/kernel.h>
> > > +#include <xen/list.h>
> > > +#include <xen/pci.h>
> > > +
> > > +LIST_HEAD(rid_sid_list);
> > > +LIST_HEAD(rid_devid_list);
> > > +
> > > +int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
> > > +                    struct acpi_iort_node *smmu_node,
> > Is there any one that will modify pcirc_node and smmu_node afterwards? 
> > If not, then they should be const.
> > > +                    uint32_t input_base, uint32_t output_base,
> > > +                    uint32_t id_count)
> > I suggested to put __init in front of that function. But you dismissed 
> > it saying it might not be valid and you will add rationale. I don't 
> > see any rationale in that patch nor an answer to my question on the 
> > previous version.
> Should a public API function be __init ?
The question is do you expect this function to be called after Xen has 
booted. Likely not, so this should be public to shrink down the size of 
Xen after boot.
[...]

> > > +/*
> > > + * API to add a rid-sid mapping
> > > + * This method should be called while parsing each entry in idmap array
> > > + * under the pcirc node in IORT.
> > > + */
> > > +int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
> > > +                    struct acpi_iort_node *smmu_node,
> > > +                    uint32_t input_base, uint32_t output_base,
> > > +                    uint32_t id_count);
> > > +/*
> > > + * API to query sid and smmu_node based on pcirc_node and rid.
> > > + *
> > > + * Example of usage:
> > > + *  int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void 
> > > *data)
> > > + *  {
> > > + *     struct iort_pci_alias_info *info = data;
> > > + *   ...
> > > + *      if ( query_sid(info->node, alias, &streamid, &smmu_node) )
> > > + *          return iort_iommu_xlate(info->dev, smmu_node, streamid);
> > > + *   ...
> > > + *   }
> > I don't see the benefits of the example usage. If the function is 
> > difficult to use, then it much better to describe each argument.
> It is not difficult, I was citing an example usecase, as you requested 
> in earlier comments, What is the best place to add it ?
> commit message / code comments.
Use case belongs to the commit message. The comment code should only 
describe the expected usage or a couple of words for the use case (but 
that usually part of the description of the function).
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel