Documentation/arch/s390/vfio-ap.rst | 30 ++++ drivers/s390/crypto/ap_bus.c | 13 +- drivers/s390/crypto/ap_bus.h | 22 +++ drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++-- drivers/s390/crypto/vfio_ap_private.h | 6 +- 5 files changed, 262 insertions(+), 29 deletions(-)
Mdevctl requires a way to atomically query and atomically update a vfio-ap
mdev's current state. This patch set creates the ap_config sysfs attribute.
This new attribute allows reading and writing an mdev's entire state in one go.
If a newly written state is invalid for any reason the entire state is rejected
and the target mdev remains unchanged.
Changelog
==========
v3
- Optimization: hot unplug functions skip apcb update if nothing was actually
unplugged.
- Hot unplug functions modified to zero bitmaps before use.
- Rename ap_matrix_length_check to ap_matrix_overflow_check
- Fixed omissions and errors in several commit messages and the docs.
- Added Tested-by tags.
v2
- Rebased patched on top of latest master
- Reworked code to fit changes introduced by f848cba767e59
s390/vfio-ap: reset queues filtered from the guest's AP config
- Moved docs changes to separate patch
Jason J. Herne (5):
s390/ap: Externalize AP bus specific bitmap reading function
s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state
s390/vfio-ap: Ignore duplicate link requests in
vfio_ap_mdev_link_queue
s390/vfio-ap: Add write support to sysfs attr ap_config
docs: Update s390 vfio-ap doc for ap_config sysfs attribute
Documentation/arch/s390/vfio-ap.rst | 30 ++++
drivers/s390/crypto/ap_bus.c | 13 +-
drivers/s390/crypto/ap_bus.h | 22 +++
drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++--
drivers/s390/crypto/vfio_ap_private.h | 6 +-
5 files changed, 262 insertions(+), 29 deletions(-)
--
2.41.0
Unless someone else chooses to review these, my opinion is that they are good to go. On 3/13/24 4:58 PM, Jason J. Herne wrote: > Mdevctl requires a way to atomically query and atomically update a vfio-ap > mdev's current state. This patch set creates the ap_config sysfs attribute. > This new attribute allows reading and writing an mdev's entire state in one go. > If a newly written state is invalid for any reason the entire state is rejected > and the target mdev remains unchanged. > > Changelog > ========== > v3 > - Optimization: hot unplug functions skip apcb update if nothing was actually > unplugged. > - Hot unplug functions modified to zero bitmaps before use. > - Rename ap_matrix_length_check to ap_matrix_overflow_check > - Fixed omissions and errors in several commit messages and the docs. > - Added Tested-by tags. > v2 > - Rebased patched on top of latest master > - Reworked code to fit changes introduced by f848cba767e59 > s390/vfio-ap: reset queues filtered from the guest's AP config > - Moved docs changes to separate patch > > > Jason J. Herne (5): > s390/ap: Externalize AP bus specific bitmap reading function > s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state > s390/vfio-ap: Ignore duplicate link requests in > vfio_ap_mdev_link_queue > s390/vfio-ap: Add write support to sysfs attr ap_config > docs: Update s390 vfio-ap doc for ap_config sysfs attribute > > Documentation/arch/s390/vfio-ap.rst | 30 ++++ > drivers/s390/crypto/ap_bus.c | 13 +- > drivers/s390/crypto/ap_bus.h | 22 +++ > drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++-- > drivers/s390/crypto/vfio_ap_private.h | 6 +- > 5 files changed, 262 insertions(+), 29 deletions(-) >
On Fri, Mar 15, 2024 at 10:48:44AM -0400, Anthony Krowiak wrote:
> Unless someone else chooses to review these, my opinion is that they are
> good to go.
Please don't top-post.
> On 3/13/24 4:58 PM, Jason J. Herne wrote:
> > Mdevctl requires a way to atomically query and atomically update a vfio-ap
> > mdev's current state. This patch set creates the ap_config sysfs attribute.
> > This new attribute allows reading and writing an mdev's entire state in one go.
> > If a newly written state is invalid for any reason the entire state is rejected
> > and the target mdev remains unchanged.
...
> > Jason J. Herne (5):
> > s390/ap: Externalize AP bus specific bitmap reading function
> > s390/vfio-ap: Add sysfs attr, ap_config, to export mdev state
> > s390/vfio-ap: Ignore duplicate link requests in
> > vfio_ap_mdev_link_queue
> > s390/vfio-ap: Add write support to sysfs attr ap_config
> > docs: Update s390 vfio-ap doc for ap_config sysfs attribute
> >
> > Documentation/arch/s390/vfio-ap.rst | 30 ++++
> > drivers/s390/crypto/ap_bus.c | 13 +-
> > drivers/s390/crypto/ap_bus.h | 22 +++
> > drivers/s390/crypto/vfio_ap_ops.c | 220 ++++++++++++++++++++++++--
> > drivers/s390/crypto/vfio_ap_private.h | 6 +-
> > 5 files changed, 262 insertions(+), 29 deletions(-)
With gcc gcc 13.2.0 / binutils 2.40.90.20230730 I get this (defconfig):
CC [M] drivers/s390/crypto/vfio_ap_ops.o
In file included from ./include/linux/cpumask.h:13,
from ./include/linux/smp.h:13,
from ./include/linux/lockdep.h:14,
from ./include/linux/spinlock.h:63,
from ./include/linux/mmzone.h:8,
from ./include/linux/gfp.h:7,
from ./include/linux/mm.h:7,
from ./include/linux/scatterlist.h:8,
from ./include/linux/iommu.h:10,
from ./include/linux/vfio.h:12,
from drivers/s390/crypto/vfio_ap_ops.c:12:
In function ‘bitmap_copy’,
inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1672:2,
inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
253 | memcpy(dst, src, len);
| ^~~~~~~~~~~~~~~~~~~~~
In function ‘ap_config_store’:
cc1: note: source object is likely at address zero
In function ‘bitmap_copy’,
inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1673:2,
inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
253 | memcpy(dst, src, len);
| ^~~~~~~~~~~~~~~~~~~~~
In function ‘ap_config_store’:
cc1: note: source object is likely at address zero
In function ‘bitmap_copy’,
inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1674:2,
inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
253 | memcpy(dst, src, len);
| ^~~~~~~~~~~~~~~~~~~~~
In function ‘ap_config_store’:
cc1: note: source object is likely at address zero
On 3/19/24 7:12 AM, Heiko Carstens wrote:
> With gcc gcc 13.2.0 / binutils 2.40.90.20230730 I get this (defconfig):
>
> CC [M] drivers/s390/crypto/vfio_ap_ops.o
> In file included from ./include/linux/cpumask.h:13,
> from ./include/linux/smp.h:13,
> from ./include/linux/lockdep.h:14,
> from ./include/linux/spinlock.h:63,
> from ./include/linux/mmzone.h:8,
> from ./include/linux/gfp.h:7,
> from ./include/linux/mm.h:7,
> from ./include/linux/scatterlist.h:8,
> from ./include/linux/iommu.h:10,
> from ./include/linux/vfio.h:12,
> from drivers/s390/crypto/vfio_ap_ops.c:12:
> In function ‘bitmap_copy’,
> inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1672:2,
> inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> 253 | memcpy(dst, src, len);
> | ^~~~~~~~~~~~~~~~~~~~~
> In function ‘ap_config_store’:
> cc1: note: source object is likely at address zero
> In function ‘bitmap_copy’,
> inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1673:2,
> inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> 253 | memcpy(dst, src, len);
> | ^~~~~~~~~~~~~~~~~~~~~
> In function ‘ap_config_store’:
> cc1: note: source object is likely at address zero
> In function ‘bitmap_copy’,
> inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1674:2,
> inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2:
> ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread]
> 253 | memcpy(dst, src, len);
> | ^~~~~~~~~~~~~~~~~~~~~
> In function ‘ap_config_store’:
> cc1: note: source object is likely at address zero
I believe that this is a bogus compiler warning. I cannot reproduce it,
fwiw.
gcc: gcc (GCC) 13.2.1 20231205 (Red Hat 13.2.1-6)
binutls binutils-2.40-14.fc39
make W=1 modules
Here is the supposedly offending code.
drivers/s390/crypto/vfio_ap_ops.c:
1670 static void ap_matrix_copy(struct ap_matrix *dst, struct ap_matrix
*src)
1671 {
1672 bitmap_copy(dst->apm, src->apm, AP_DEVICES);
1673 bitmap_copy(dst->aqm, src->aqm, AP_DOMAINS);
1674 bitmap_copy(dst->adm, src->adm, AP_DOMAINS);
1675 }
called from drivers/s390/crypto/vfio_ap_ops.c:
1695 /* Save old state */
1696 ap_matrix_copy(&m_old, &matrix_mdev->matrix);
Definition of struct in drivers/s390/crypto/vfio_ap_private.h:
113 struct ap_matrix_mdev {
114 struct vfio_device vdev;
115 struct list_head node;
116 struct ap_matrix matrix;
117 struct ap_matrix shadow_apcb;
118 struct kvm *kvm;
119 crypto_hook pqap_hook;
120 struct mdev_device *mdev;
121 struct ap_queue_table qtable;
122 struct eventfd_ctx *req_trigger;
123 DECLARE_BITMAP(apm_add, AP_DEVICES);
124 DECLARE_BITMAP(aqm_add, AP_DOMAINS);
125 DECLARE_BITMAP(adm_add, AP_DOMAINS);
126 };
drivers/s390/crypto/vfio_ap_private.h:
76 struct ap_matrix {
77 unsigned long apm_max;
78 DECLARE_BITMAP(apm, AP_DEVICES);
79 unsigned long aqm_max;
80 DECLARE_BITMAP(aqm, AP_DOMAINS);
81 unsigned long adm_max;
82 DECLARE_BITMAP(adm, AP_DOMAINS);
83 };
drivers/s390/crypto/ap_bus.h:
22 #define AP_DEVICES 256 /* Number of AP devices. */
23 #define AP_DOMAINS 256 /* Number of AP domains. */
The source object seems to have a well defined size.
A quick web search seems to indicate gcc throws quite a few
Wstringop-overread warnings for valid code. I suspect this is
another example of that.
Hi Jason, > > In function ‘bitmap_copy’, > > inlined from ‘ap_matrix_copy’ at drivers/s390/crypto/vfio_ap_ops.c:1674:2, > > inlined from ‘ap_config_store’ at drivers/s390/crypto/vfio_ap_ops.c:1696:2: > > ./include/linux/bitmap.h:253:17: warning: ‘memcpy’ reading 32 bytes from a region of size 0 [-Wstringop-overread] > > 253 | memcpy(dst, src, len); > > | ^~~~~~~~~~~~~~~~~~~~~ > > In function ‘ap_config_store’: > > cc1: note: source object is likely at address zero > > I believe that this is a bogus compiler warning. I cannot reproduce it, > fwiw. > > gcc: gcc (GCC) 13.2.1 20231205 (Red Hat 13.2.1-6) > binutls binutils-2.40-14.fc39 ... > A quick web search seems to indicate gcc throws quite a few > Wstringop-overread warnings for valid code. I suspect this is > another example of that. This might be the case, however the code has to compile without warnings also with plain gcc 13.2.0 (built from source).
© 2016 - 2026 Red Hat, Inc.