Re: [PATCH 10/12] xen-pcifront: this module is PV-only

From: Jan Beulich <jbeulich@suse.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Juergen Gross <jgross@suse.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH 10/12] xen-pcifront: this module is PV-only
Date: Tue, 7 Sep 2021 18:14:16 +0200	[thread overview]
Message-ID: <82a99bb4-85bd-61cc-85de-f8dd9d1f98ed@suse.com> (raw)
In-Reply-To: <20210907153017.GA741607@bjorn-Precision-5520>

On 07.09.2021 17:30, Bjorn Helgaas wrote:
> Update subject to follow conventions (use "git log --oneline
> drivers/pci/Kconfig").  Should say what this patch does.

I can change that; I don't think it'll carry any different information.

> Commit log below should also say what this patch does.  Currently it's
> part of the rationale for the change, but doesn't say what the patch
> does.

"There's no point building ..." to me is as good as "Don't build ...".
But oh well, I can adjust ...

> On Tue, Sep 07, 2021 at 02:10:41PM +0200, Jan Beulich wrote:
>> It's module init function does a xen_pv_domain() check first thing.
>> Hence there's no point building it in non-PV configurations.
> 
> s/It's/<name of function that calls xen_pv_domain()/   # pcifront_init()?

I don't understand this - how is "module init function" not clear
enough?

> s/building it/building <name of module>/               # xen-pcifront.o?

The driver name is already part of the subject; I didn't think I
need to repeat that one here.

> I see that CONFIG_XEN_PV is only mentioned in arch/x86, so
> CONFIG_XEN_PV=y cannot be set on other arches.  Is the current
> "depends on X86" just a reflection of that, or is it because of some
> other x86 dependency in the code?
> 
> The connection between xen_pv_domain() and CONFIG_XEN_PV is not
> completely obvious.
> 
> If you only build xen-pcifront.o when CONFIG_XEN_PV=y, and
> xen_pv_domain() is true if and only if CONFIG_XEN_PV=y, why bother
> calling xen_pv_domain() at all?

Because XEN_PV=y only _enables_ the kernel to run in PV mode. It
may be enabled to also run in HVM and/or PVH modes. And it may
then _run_ in any of the enabled modes. IOW xen_pv_domain() will
always return false when !XEN_PV; no other implication is valid.
I don't think this basic concept needs explaining in a simple
patch like this. Instead I think the config option in question,
despite living in drivers/pci/Kconfig, should be under "XEN
HYPERVISOR INTERFACE" maintainership. I realize that's not even
expressable in ./MAINTAINERS. I wonder why the option was put
there in the first place, rather than in drivers/xen/Kconfig.

Jan