Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ec7b4f2ad9f1cfb6ad47e9476b11127ecb24a9f7.camel@linux.ibm.com>
Subject: Re: [PATCH v6 4/5] PCI: Extend isolated function probing to s390
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Pierre Morel <pmorel@linux.ibm.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Cc:     Jan Kiszka <jan.kiszka@siemens.com>,
        Matthew Rosato <mjrosato@linux.ibm.com>,
        linux-s390@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Fri, 01 Jul 2022 16:42:38 +0200
In-Reply-To: <90996285-9ae3-0030-a5e3-a3f1bfa23088@linux.ibm.com>
References: <20220628143100.3228092-1-schnelle@linux.ibm.com>
         <20220628143100.3228092-5-schnelle@linux.ibm.com>
         <90996285-9ae3-0030-a5e3-a3f1bfa23088@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2022-06-30 at 14:45 +0200, Pierre Morel wrote:
> 
> On 6/28/22 16:30, Niklas Schnelle wrote:
> > Like the jailhouse hypervisor s390's PCI architecture allows passing
> > isolated PCI functions to an OS instance. As of now this is was not
> > utilized even with multi-function support as the s390 PCI code makes
> > sure that only virtual PCI busses including a function with devfn 0 are
> > presented to the PCI subsystem. A subsequent change will remove this
> > restriction.
> > 
> > Allow probing such functions by replacing the existing check for
> > jailhouse_paravirt() with a new hypervisor_isolated_pci_functions()
> > helper.
> > 
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >   drivers/pci/probe.c        | 2 +-
> >   include/linux/hypervisor.h | 8 ++++++++
> >   2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index a18e07e6a7df..156dd13594b8 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2667,7 +2667,7 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
> >   			 * a hypervisor which passes through individual PCI
> >   			 * functions.
> >   			 */
> > -			if (!jailhouse_paravirt())
> > +			if (!hypervisor_isolated_pci_functions())
> >   				break;
> >   		}
> >   		fn = next_fn(bus, dev, fn);
> > diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
> > index fc08b433c856..33b1c0482aac 100644
> > --- a/include/linux/hypervisor.h
> > +++ b/include/linux/hypervisor.h
> > @@ -32,4 +32,12 @@ static inline bool jailhouse_paravirt(void)
> >   
> >   #endif /* !CONFIG_X86 */
> >   
> > +static inline bool hypervisor_isolated_pci_functions(void)
> > +{
> > +	if (IS_ENABLED(CONFIG_S390))
> > +		return true;
> > +	else
> > +		return jailhouse_paravirt();
> 
> I would spare the else,

I don't have a preference for either style so sure.

> 
> Another remark, shouldn't it be the last patch?

Either way should work. Without the last patch we don't try to probe
and without this patch the probing wouldn't find the function. I think
I'll keep the order to keep the PCI subsystem changes together and
because I feel trying to probe without that working is worse than not
probing.

> 
> otherwise LGTM
> 
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>

Thanks for taking a look!

> 
> 
> > +}
> > +
> >   #endif /* __LINUX_HYPEVISOR_H */
> >