Re: [Xen-devel] [RFC 01/11] acpi: arm: Public API for populating and query based on requesterid

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 01/19/2018 05:33 PM, Julien Grall wrote:
> On 19/01/18 06:05, Manish Jaggi wrote:
> > On 01/17/2018 12:01 AM, Julien Grall wrote:
> > > Hi Manish,
> > Hi Julien,
> > Thanks for reviewing the patch.
> >
> > > I sent the previous e-mail too soon.
> > >
> > > On 02/01/18 09:27, manish.jaggi@xxxxxxxxxx wrote:
> > > > From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> > > >
> > > >   Public API to populate and query map between requester id and
> > > >   streamId/DeviceID. IORT is parsed one time (outside this patch)
> > > >   and two lists are created one for mapping between reuesterId and 
> > > > streamid
> > > >   and another between requesterID and deviceID.
> > > >
> > > >   These lists eliminate the need to reparse IORT for querying streamid
> > > >   or deviceid using requesterid.
> > > >
> > > >   Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> > > > ---
> > > >   xen/drivers/acpi/Makefile     |   1 +
> > > >   xen/drivers/acpi/arm/Makefile |   1 +
> > > We have a directory arch/arm/acpi/. So please move all your code there.
> > The current files in arch/arm/acpi hold only boot time/low level code.
> > IMHO creating drivers/acpi/arm makes more sense.
> > Linux also has iort code in drivers/acpi/arm.
> drivers/acpi mostly contain generic ACPI code. ridmap.c and iort.c is 
> AFAICT Arm specific. So arch/arm/acpi is a better place.
ok, I will move the code.
What about iort.c ?
Should it be in drivers/acpi/arm
> [...]
>
> > > > + *
> > > > + * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> > > > + * Copyright (c) 2018 Linaro.
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or 
> > > > modify
> > > > + * it under the terms of the GNU General Public License as 
> > > > published by
> > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > + * (at your option) any later version.
> > > Xen is GPLv2 only and hence the copyright wrong. You want to use:
> > >
> > > This program is free software; you can redistribute it and/or modify it
> > > under the terms and conditions of the GNU General Public License,
> > > version 2, as published by the Free Software Foundation.
> > I picked this copyright from xen/arch/arm/traps.c.
> >
> >   * xen/arch/arm/traps.c
> >   *
> >   * ARM Trap handlers
> >   *
> >   * Copyright (c) 2011 Citrix Systems.
> >   *
> >   * This program is free software; you can redistribute it and/or modify
> >   * it under the terms of the GNU General Public License as published by
> >   * the Free Software Foundation; either version 2 of the Licese, or
> >   * (at your option) any later version.
> >
> > So IIUYC, traps.c copyright is also wrong.
> > How do we plan to fix all other files in xen code which use the same 
> > copyright.
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02899.html 
I was not aware of this patch, will change the copyright.
Please add what copyright to be used in Xen coding guidelines document.
Moreover the patch to fix copyright is still not merged, any specific 
reason?
> > > > + *
> > > > + * This program is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > > + * GNU General Public License for more details.
> > > > + */
> > > > +
> > > > +#include <acpi/ridmap.h>
> > > > +#include <xen/iommu.h>
> > > > +#include <xen/kernel.h>
> > > > +#include <xen/list.h>
> > > > +#include <xen/pci.h>
> > > > +
> > > > +struct list_head rid_streamid_map_list;
> > > > +struct list_head rid_deviceid_map_list;
> > > Please drop _list. This is pointless to know that when you can 
> > > discover it.
> > I think it is not pointless.
> > There is a point here. :)
> >   _list is added to show that it is a list to make it more verbose.
> > Without _list the name could mean a single mapping as well.
> When I read, rid_streamid_map. I understand it is a map of 
> rid/streamid. Not a single mapping. But ...
But In code the rid_streamid_map is used for a single mapping, so there 
has to be a proper naming.
Thats why I used a _list.

range_t and range_list. Same logic here.
> > If you care to see
> > xen/common/rangeset.c:27:    struct list_head range_list;
> >
> > I hope you can appreciate the point.
> ... look at the name length here, 10 characters. Yours is 22 
> characters. This is 1/4 of a line. That's just stupid.
So your reservation is not against _list but 22 characters ?
I dont consider this stupid at all. The list is accessed in few 
functions only and it makes code more readable.
Anyway, how about rid_sid_map_list? or rid_sid_list ? Which one you prefer.
> > > Also, can you explain the rationale of using an unsorted list over 
> > > another structure? 
> > Since rid - streamId mapping also requires pcirc_node so it would 
> > require two level of sorting.
> > First sort based on pcirc_node and next on basis of rid.
> > Does it makes sense to have all that complex code here ?
> >   as this API will be used only once per pci device
> > > Along that please give an idea how often and where the query API 
> > > will be used.
> > ok
> > BTW, this is called from pci_for_each_dma_alias code flow.
> The document the rationale.
The/Then?
Yes I will do in v2.
> > > > +
> > > > +void init_ridmaps(void)
> > > This likely need to be __init.
> > ok.
> > > > +{
> > > > +    INIT_LIST_HEAD(&rid_deviceid_map_list);
> > > > +    INIT_LIST_HEAD(&rid_streamid_map_list);
> > > > +}
> > > This function is not necessary. Declaring 
> > > LIST_HEAD(rid_streamid_map_list) will do the trick.
> > ok.
> > > > +
> > > > +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
> > > Ditto.
> > Ditto for? Sorry didnt catch your point here.
> __init.
_init might not be valid, I will add more documentation and you can see 
the rationale.
> > > > +                         struct acpi_iort_node *smmu_node,
> > > > +                         u32 input_base, u32 output_base, u32 
> > > > id_count)
> > > u32 & co should not be used in new code (unless imported from 
> > > Linux). Please use uint32_t & co. /.
> > I couldn't find this in xen coding style document.
> > Could you please point to the section which says u32 should not be used.
> It is not in the coding style but Xen is phasing out from u*. What's 
> the problem?
There is no problem, but It is not written anywhere so I was not sure of 
your comment.
I suggest it should be _mentioned_ in xen coding guidelines.
This will avoid all unwritten standards not being followed issues.
> > > > +{
> > > > +    struct rid_streamid_map *rid_map;
> > > Newline here as it should be between after declarations.
> > ok
> > > > +    rid_map = xzalloc(struct rid_streamid_map);
> > > > +
> > > > +    if (!rid_map)
> > > This should be ( ... ).
> > >
> > > > +        return -ENOMEM;
> > > You either return -ENOMEM or 0 in this function. It sounds like to 
> > > me that bool would be the best.
> > I think ENOMEM should be used here. The error code is designed 
> > specifically for this purpose.
> Fair enough.
Thanks it is one of rare instances where you agree to my point.
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, 
> > > > u32 *streamid,
> > > s/u*/uint_/
> > >
> > > But how come the rid is 16-bit here when Linux is using 32-bit?
> > IIUC rid is 16bit only. Dont know why linux is using 32bit.
> >
> > rid = bus - 8bits , devfn 8bits.
> >
> >  From PCI Express specification
> > The Requester ID is a 16-bit value that is unique for every PCI 
> > Express Function within a Hierarchy..
> > If you think it is a 32bit value please let me know how to use upper 
> > 16 bits.
> Well AFAICT, the IORT stores 32-bit. So it is probably best to stick 
> with it.
IORT uses 32bit and refers as input_base.
That is generic in acpi_iort_id_mapping.
Does IORT spec uses term requesterID and defines its width as 32bit?

I can keep 32bit anyways.
> > > Also, I am a bit puzzled how the caller is expected to use it. 
> > I thought it would be self explanatory query streamid based on rid.
> > But if it is not verbose enough for you, I will add this explicitly.
> Not at all. More than this function is returning void. A query 
> function is usually return a bool/int.
ok will change.
> > > From the name I would expect the function to return whether a 
> > > translation was found. But it returns void.
> > > IHMO, this is a pretty bad idea and make more expectation on the 
> > > value for the caller.
> > > Lastly, I would appreciate documentation on at least the function 
> > > exported.
> > ok.
> > > > +                    struct acpi_iort_node **smmu_node)
> > > > +{
> > > > +    struct rid_streamid_map *rmap;
> > > > +
> > > > +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
> > > > +    {
> > > > +        if (rmap->pcirc_node == pcirc_node)
> > > Coding style.
> > Can we have a checkpatch.pl for xen.
> > This would help in cases when code has a mix of files with linux 
> > coding style and xen coding style.
> This is been worked on.
Great!
> [...]
>
> > > I am not a big fan of exporting those 2 maps. But I will see how you 
> > > use it before commenting.
> > Lot of xen code using the same way. Is that all wrong?
> Second sentence: "I will see how you use it before commenting".
In that case, the comment was unwanted..
> Cheers,

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