linux-kernel - Re: [PATCH v6 4/5] PCI: Extend isolated function probing to s390

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ec7b4f2ad9f1cfb6ad47e9476b11127ecb24a9f7.camel@linux.ibm.com>
Date:   Fri, 01 Jul 2022 16:42:38 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Pierre Morel <pmorel@...ux.ibm.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Cc:     Jan Kiszka <jan.kiszka@...mens.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/5] PCI: Extend isolated function probing to s390

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@...mens.com>
> > Signed-off-by: Niklas Schnelle <schnelle@...ux.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@...ux.ibm.com>

Thanks for taking a look!

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