Re: [Xen-devel] [RFC v2 5/7] acpi:arm64: Add support for parsing IORT table

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Sameer,

On 19/10/17 16:21, Goel, Sameer wrote:
> On 10/12/2017 8:06 AM, Julien Grall wrote:
> > > +
> > > +/* Redefine WARN macros */
> > > +#undef WARN
> > > +#undef WARN_ON
> > > +#define WARN(condition, format...) ({                    \
> > > +    int __ret_warn_on = !!(condition);                \
> > > +    if (unlikely(__ret_warn_on))                    \
> > > +        printk(format);                        \
> > > +    unlikely(__ret_warn_on);                    \
> > > +})
> > Again, you should at least try to modify the common code version before 
> > deciding to redefine it here.
> The xen macro seems such that it explicitly tries to block a return by wrapping 
> this macro in a loop. I had changed the common function in the last iteration 
> and there seemed to be a pushback.
I don't think there was any pushback on changing the common code. Jan 
Beulich validly requested to move that change in a separate patch.
> > > +#define WARN_TAINT(cond, taint, format...) WARN(cond, format)
> > > +#define WARN_ON(cond)                      (!!cond)
> > >      #define IORT_TYPE_MASK(type)    (1 << (type))
> > >    #define IORT_MSI_TYPE        (1 << ACPI_IORT_NODE_ITS_GROUP)
> > > @@ -256,6 +286,13 @@ 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;
> > > +/*
> > > + * We need the namespace object name from dsdt to match the iort node, this
> > Please add a "Xen: TODO:" in front.
> Ok.
> > > + * will need additions to the kernel xen bus notifiers.
> > > + * So, disabling the named node code till a proposal is approved.
> > > + */
> > > +#if 0
> > >            struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> > >            struct acpi_device *adev = to_acpi_device_node(dev->fwnode);
> > >            struct acpi_iort_named_component *ncomp;
> > > @@ -275,11 +312,12 @@ static acpi_status iort_match_node_callback(struct 
> > > acpi_iort_node *node,
> > >            status = !strcmp(ncomp->device_name, buf.pointer) ?
> > >                                AE_OK : AE_NOT_FOUND;
> > >            acpi_os_free(buf.pointer);
> > > +#endif
> > >        } else if (node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
> > >            struct acpi_iort_root_complex *pci_rc;
> > > -        struct pci_bus *bus;
> > > +        struct pci_dev *pci_dev;
> > Do you really need to modify the code? Wouldn't it be possible to do
> >
> > #define pci_bus pci_dev
> The pci_dev is the container for the generic device. I wanted to call this out 
> explicitly here. We can do the above if you insist :).
I agree this can be confusing to read. But that change is not justified 
for the goal you want to achieve. E.g importing the code from Linux and 
keep in sync in the future.
With a comment on top of the definitions, you could explain when pci_bus 
= pci_dev at the moment.
[...]

> > > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > index 362d578..ad956d5 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -181,6 +181,7 @@ static void __iomem *devm_ioremap_resource(struct device 
> > > *dev,
> > >     * Xen: PCI functions
> > >     * TODO: It should be implemented when PCI will be supported
> > >     */
> > > +#undef to_pci_dev
> > Why this change?
> I had redefine the to_pci_dev to get the actual pci_dev struct. smmu driver 
> does not use pci yet.
That should go in a separate commit in that case with probably all stub 
for PCI you added (see the pci.h).
The reason behind is those changes are not directly related to this 
patch. Patch should be kept simple and do one thing only. This makes 
easier to review.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel