Re: [Xen-devel] [PATCH 1/5] IOMMU: iommu_intremap is x86-only

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 02/03/2020 10:57, Jan Beulich wrote:
> On 02.03.2020 11:44, Julien Grall wrote:
> > On 02/03/2020 10:07, Jan Beulich wrote:
> > > On 28.02.2020 21:16, Andrew Cooper wrote:
> > > > On 28/02/2020 12:26, Jan Beulich wrote:
> > > > > Provide a #define for other cases; it didn't seem worthwhile to me to
> > > > > introduce an IOMMU_INTREMAP Kconfig option at this point.
> > > > >
> > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > >
> > > > > --- a/docs/misc/xen-command-line.pandoc
> > > > > +++ b/docs/misc/xen-command-line.pandoc
> > > > > @@ -1299,6 +1299,8 @@ boolean (e.g. `iommu=no`) can override t
> > > > >        generation of IOMMUs only supported DMA remapping, and Interrupt 
> > > > > Remapping
> > > > >        appeared in the second generation.
> > > > >    
> > > > > +    This option is not valid on Arm.
> > > > The longevity of this comment would be greater if it were phrased as "is
> > > > only valid on x86", especially given the RFC RISCV series on list.
> > > How do we know how intremap is going to work on future ports?
> > We don't  know. But, for a reviewer, it is going to be much easier to
> > notice a command line option is going to be used as the patch would
> > modify a caller.
> I'm struggling with understanding this (not seeing the connection
> between "command line option" and "caller"), but ...
"caller" might have been the wrong word here. Let me expand it. The 
patch you sent contains an #ifdef CONFIG_X86 protecting the declaration 
of iommu_intremap:
+#ifdef CONFIG_X86
 extern enum __packed iommu_intremap {
    /*
     * In order to allow traditional boolean uses of the iommu_intremap
     * variable, the "off" value has to come first (yielding a value of 
zero).
     */
    iommu_intremap_off,
-#ifdef CONFIG_X86
    iommu_intremap_restricted,
-#endif
    iommu_intremap_full,
 } iommu_intremap;
+#else
+# define iommu_intremap false
+#endif

Someone wanted to use iommu_intremap (and by extent the command line 
option) in a new arch would have to modify the declaration for it to 
work. A commit message would also likely to contain "Implement the 
command line option ...". So a reviewer can spot the change and ask to 
update the documentation if this wasn't yet done.
At the inverse, if the new arch is not using iommu_intremap then there 
will be no modification in the code. Therefore, it may be more difficult 
for a reviewer to notice that the documentation needs to be updated.
Cheers,

--
Julien Grall

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