Re: [PATCH 01/10] s390/cio: Export information about Endpoint-Security Capability

From: Cornelia Huck <cohuck@redhat.com>
To: Stefan Haberland <sth@linux.ibm.com>
Cc: axboe@kernel.dk, linux-block@vger.kernel.org,
	hoeppner@linux.ibm.com, linux-s390@vger.kernel.org,
	heiko.carstens@de.ibm.com, gor@linux.ibm.com,
	borntraeger@de.ibm.com, vneethv@linux.vnet.ibm.com
Subject: Re: [PATCH 01/10] s390/cio: Export information about Endpoint-Security Capability
Date: Tue, 6 Oct 2020 16:37:07 +0200	[thread overview]
Message-ID: <20201006163707.0bd1b90a.cohuck@redhat.com> (raw)
In-Reply-To: <153dde83-0f87-69d4-df65-a43342f5fc8e@linux.ibm.com>

On Tue, 6 Oct 2020 16:23:36 +0200
Stefan Haberland <sth@linux.ibm.com> wrote:

> Hi,
> 
> talked to Vineeth, here is his answer...
> 
> Am 06.10.20 um 11:46 schrieb Cornelia Huck:
> > On Fri,  2 Oct 2020 21:39:31 +0200
> > Stefan Haberland <sth@linux.ibm.com> wrote:
> >  
> >> From: Sebastian Ott <sebott@linux.ibm.com>
> >>
> >> Add a new sysfs attribute 'esc' per chpid. This new attribute exports
> >> the Endpoint-Security-Capability byte of channel-path description block,
> >> which could be 0-None, 1-Authentication, 2 and 3-Encryption.
> >>
> >> For example:
> >> $ cat /sys/devices/css0/chp0.34/esc
> >> 0
> >>
> >> Reference-ID: IO1812
> >> Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
> >> [vneethv@linux.ibm.com: cleaned-up & modified description]
> >> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> >> Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
> >> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> >> Acked-by: Vasily Gorbik <gor@linux.ibm.com>
> >> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> >> ---
> >>  drivers/s390/cio/chp.c  | 15 +++++++++++++++
> >>  drivers/s390/cio/chsc.h |  3 ++-
> >>  2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> >> index dfcbe54591fb..8d0de6adcad0 100644
> >> --- a/drivers/s390/cio/chp.c
> >> +++ b/drivers/s390/cio/chp.c
> >> @@ -384,6 +384,20 @@ static ssize_t chp_chid_external_show(struct device *dev,
> >>  }
> >>  static DEVICE_ATTR(chid_external, 0444, chp_chid_external_show, NULL);
> >>  
> >> +static ssize_t chp_esc_show(struct device *dev,
> >> +			    struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct channel_path *chp = to_channelpath(dev);
> >> +	ssize_t rc;
> >> +
> >> +	mutex_lock(&chp->lock);
> >> +	rc = sprintf(buf, "%x\n", chp->desc_fmt1.esc);  
> > I'm wondering: Do we need to distinguish between '0' == 'no esc, and
> > the hardware says so' and '0' == 'the chsc to get that information is
> > not supported'? I see that for the chid the code checks for a flag in
> > desc_fmt1, and I indeed see that nothing is displayed for
> > chid/chid_external when I run under QEMU.  
> 
> ESC==0 due to 'missing support for the required CHSC information' is
> just another symptom of "unsupported" because the CSS firmware code
> doesn't bring the required support.
> Also, not sure if there is any flag/value which provide this
> distinction. So we think having 2 different values "Unknown" and
> "Unsupported" is not required in this scenario.
> 
> So, we kept a single "ESC==0" which indicates "Unsupported", but as you
> mentioned, in different levels.

Ok, that makes sense, also considering how this is used later on.

> 
> >> +	mutex_unlock(&chp->lock);
> >> +
> >> +	return rc;
> >> +}
> >> +static DEVICE_ATTR(esc, 0444, chp_esc_show, NULL);
> >> +
> >>  static ssize_t util_string_read(struct file *filp, struct kobject *kobj,
> >>  				struct bin_attribute *attr, char *buf,
> >>  				loff_t off, size_t count)  
> > (...)
> >  
> 
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>