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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 22/01/18 05:07, Manish Jaggi wrote:
> 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
I think they should be both in arch/arm/acpi. Stefano do you have any 
opinion?
> > [...]
> >
> > > > > + *
> > > > > + * 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.
Patch are welcomed for coding style improvement.

> Moreover the patch to fix copyright is still not merged, any specific 
> reason?
Because it is unclear how you can modify the header in current code. 
Whether you need an ack from everyone who has touched the code. See the 
thread.
New code should use the GPLv2 copyright to avoid such dilemma.

> > > > > + *
> > > > > + * 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.
I prefer rid_sid_list.

> > > > 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 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.
Do you mind giving the rationale now? Why would you call this function 
after boot?
> > > > > +                         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.
Patch for coding style improvement are welcomed.

> > > > > +    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?
The IORT spec does not seem to say anything about the width of 
requesterID. So I prefer to stay on the safe side on use 32-bit.
[...]

> > > > 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..
Probably less than "Thanks it is one of rare instances where you agree 
to my point." ;).
Cheers,

--
Julien Grall

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