[PATCH for-7.0 4/4] hw/nvme: add support for zoned random write area
From: Klaus Jensen
Add support for TP 4076 ("Zoned Random Write Area"), v2021.08.23
("Ratified").
This adds three new namespace parameters: "zoned.numzrwa" (number of
zrwa resources, i.e. number of zones that can have a zrwa),
"zoned.zrwas" (zrwa size in LBAs), "zoned.zrwafg" (granularity in LBAs
for flushes).
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 171 ++-
hw/nvme/ns.c | 58 +++
hw/nvme/nvme.h | 10 +++
hw/nvme/trace-events | 1 +
include/block/nvme.h | 17 -
5 files changed, 237 insertions(+), 20 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 7ac6ec50a0d1..4c9b303dfdca 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -299,26 +299,37 @@ static void nvme_assign_zone_state(NvmeNamespace *ns,
NvmeZone *zone,
}
}
-/*
- * Check if we can open a zone without exceeding open/active limits.
- * AOR stands for "Active and Open Resources" (see TP 4053 section 2.5).
- */
-static int nvme_aor_check(NvmeNamespace *ns, uint32_t act, uint32_t opn)
+static uint16_t nvme_zns_check_resources(NvmeNamespace *ns, uint32_t act,
+ uint32_t opn, uint32_t zrwa)
{
if (ns->params.max_active_zones != 0 &&
ns->nr_active_zones + act > ns->params.max_active_zones) {
trace_pci_nvme_err_insuff_active_res(ns->params.max_active_zones);
return NVME_ZONE_TOO_MANY_ACTIVE | NVME_DNR;
}
+
if (ns->params.max_open_zones != 0 &&
ns->nr_open_zones + opn > ns->params.max_open_zones) {
trace_pci_nvme_err_insuff_open_res(ns->params.max_open_zones);
return NVME_ZONE_TOO_MANY_OPEN | NVME_DNR;
}
+if (zrwa > ns->zns.numzrwa) {
+return NVME_NOZRWA | NVME_DNR;
+}
+
return NVME_SUCCESS;
}
+/*
+ * Check if we can open a zone without exceeding open/active limits.
+ * AOR stands for "Active and Open Resources" (see TP 4053 section 2.5).
+ */
+static uint16_t nvme_aor_check(NvmeNamespace *ns, uint32_t act, uint32_t opn)
+{
+return nvme_zns_check_resources(ns, act, opn, 0);
+}
+
static bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
{
hwaddr hi, lo;
@@ -1605,9 +1616,19 @@ static uint16_t nvme_check_zone_write(NvmeNamespace *ns,
NvmeZone *zone,
return status;
}
-if (unlikely(slba != zone->w_ptr)) {
-trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba, zone->w_ptr);
-return NVME_ZONE_INVALID_WRITE;
+if (zone->d.za & NVME_ZA_ZRWA_VALID) {
+uint64_t ezrwa = zone->w_ptr + 2 * ns->zns.zrwas;
+
+if (slba < zone->w_ptr || slba + nlb > ezrwa) {
+trace_pci_nvme_err_zone_invalid_write(slba, zone->w_ptr);
+return NVME_ZONE_INVALID_WRITE;
+}
+} else {
+if (unlikely(slba != zone->w_ptr)) {
+trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
+ zone->w_ptr);
+return NVME_ZONE_INVALID_WRITE;
+}
}
if (unlikely((slba + nlb) > zcap)) {
@@ -1687,6 +1708,14 @@ static uint16_t nvme_zrm_finish(NvmeNamespace *ns,
NvmeZone *zone)
/* fallthrough */
case NVME_ZONE_STATE_CLOSED:
nvme_aor_dec_active(ns);
+
+if (zone->d.za & NVME_ZA_ZRWA_VALID) {
+NVME_ZA_CLEAR(zone->d.za, NVME_ZA_ZRWA_VALID);
+if (ns->params.numzrwa) {
+ns->zns.numzrwa++;
+}
+}
+
/* fallthrough */
case NVME_ZONE_STATE_EMPTY:
nvme_assign_zone_state(ns, zone, NVME_ZONE_STATE_FULL);
@@ -1722,6 +1751,13 @@ static uint16_t nvme_zrm_reset(NvmeNamespace *ns,
NvmeZone *zone)
/* fallthrough */
case NVME_ZONE_STATE_CLOSED:
nvme_aor_dec_active(ns);
+
+if (zone->d.za & NVME_ZA_ZRWA_VALID) {
+if (ns->params.numzrwa) {
+ns->zns.numzrwa++;
+}
+}
+
/* fallthrough */
case NVME_ZONE_STATE_FULL:
zone->w_ptr = zone->d.zslba;
@@ -1755,6 +1791,7 @@ static void nvme_zrm_auto_transition_zone(NvmeNamespace
*ns)
enum {
NVME_ZRM_AUTO = 1 << 0,
+NVME_ZRM_ZRWA = 1 << 1,
};
static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
@@ -1773,7 +1810,8 @@ static uint16_t nvme_zrm_open_flags(NvmeCtrl *n,
NvmeNamespace *ns,
if (n->params.auto_transition_zones) {
nvme_zrm_auto_transition_zone(ns);
}
-status = nvme_aor_check(ns, act, 1);
+status = nvme_zns_check_resources(ns, act, 1,
+ (flags & NVME_ZRM_ZRWA) ? 1 : 0);
if (status) {
return status;
}
@@ -1801,6 +1839,12 @@ static uint16_t nvme_zrm_open_flags(NvmeCtrl *n,
NvmeNamespace *ns,
/* fallthrough */
case NVME_ZONE_STATE_EXPLICITLY_OPEN:
+if (flags & NVME_ZRM_ZRWA) {
+ns->zns.numzrwa--;
+
+NVME_ZA_SET(zone->d.za,
[PATCH for-7.0 3/4] hw/nvme: add ozcs enum
From: Klaus Jensen
Add enumeration for OZCS values.
Signed-off-by: Klaus Jensen
---
hw/nvme/ns.c | 3 ++-
include/block/nvme.h | 4
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 8b5f98c76180..356b6c1c2f14 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -266,7 +266,8 @@ static void nvme_ns_init_zoned(NvmeNamespace *ns)
id_ns_z->mar = cpu_to_le32(ns->params.max_active_zones - 1);
id_ns_z->mor = cpu_to_le32(ns->params.max_open_zones - 1);
id_ns_z->zoc = 0;
-id_ns_z->ozcs = ns->params.cross_zone_read ? 0x01 : 0x00;
+id_ns_z->ozcs = ns->params.cross_zone_read ?
+NVME_ID_NS_ZONED_OZCS_RAZB : 0x00;
for (i = 0; i <= ns->id_ns.nlbaf; i++) {
id_ns_z->lbafe[i].zsze = cpu_to_le64(ns->zone_size);
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 2b8b906466ab..d33ff2c184cf 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1351,6 +1351,10 @@ typedef struct QEMU_PACKED NvmeIdNsZoned {
uint8_t vs[256];
} NvmeIdNsZoned;
+enum NvmeIdNsZonedOzcs {
+NVME_ID_NS_ZONED_OZCS_RAZB= 1 << 0,
+};
+
/*Deallocate Logical Block Features*/
#define NVME_ID_NS_DLFEAT_GUARD_CRC(dlfeat) ((dlfeat) & 0x10)
#define NVME_ID_NS_DLFEAT_WRITE_ZEROES(dlfeat)((dlfeat) & 0x08)
--
2.34.0
[PATCH for-7.0 2/4] hw/nvme: add zone attribute get/set helpers
From: Klaus Jensen
Add some get/set helpers for zone attributes.
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 4 ++--
include/block/nvme.h | 4
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 489d586ab9d7..7ac6ec50a0d1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -295,7 +295,7 @@ static void nvme_assign_zone_state(NvmeNamespace *ns,
NvmeZone *zone,
case NVME_ZONE_STATE_READ_ONLY:
break;
default:
-zone->d.za = 0;
+NVME_ZA_CLEAR_ALL(zone->d.za);
}
}
@@ -3356,7 +3356,7 @@ static uint16_t nvme_set_zd_ext(NvmeNamespace *ns,
NvmeZone *zone)
return status;
}
nvme_aor_inc_active(ns);
-zone->d.za |= NVME_ZA_ZD_EXT_VALID;
+NVME_ZA_SET(zone->d.za, NVME_ZA_ZD_EXT_VALID);
nvme_assign_zone_state(ns, zone, NVME_ZONE_STATE_CLOSED);
return NVME_SUCCESS;
}
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 2ee227760265..2b8b906466ab 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1407,6 +1407,10 @@ enum NvmeZoneAttr {
NVME_ZA_ZD_EXT_VALID = 1 << 7,
};
+#define NVME_ZA_SET(za, attrs) ((za) |= (attrs))
+#define NVME_ZA_CLEAR(za, attrs) ((za) &= ~(attrs))
+#define NVME_ZA_CLEAR_ALL(za)((za) = 0x0)
+
typedef struct QEMU_PACKED NvmeZoneReportHeader {
uint64_tnr_zones;
uint8_t rsvd[56];
--
2.34.0
[PATCH for-7.0 1/4] hw/nvme: add struct for zone management send
From: Klaus Jensen
Add struct for Zone Management Send in preparation for more zone send
flags.
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 10 --
include/block/nvme.h | 18 ++
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 5f573c417b3d..489d586ab9d7 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3593,26 +3593,24 @@ done:
static uint16_t nvme_zone_mgmt_send(NvmeCtrl *n, NvmeRequest *req)
{
-NvmeCmd *cmd = (NvmeCmd *)>cmd;
+NvmeZoneSendCmd *cmd = (NvmeZoneSendCmd *)>cmd;
NvmeNamespace *ns = req->ns;
NvmeZone *zone;
NvmeZoneResetAIOCB *iocb;
uint8_t *zd_ext;
-uint32_t dw13 = le32_to_cpu(cmd->cdw13);
uint64_t slba = 0;
uint32_t zone_idx = 0;
uint16_t status;
-uint8_t action;
+uint8_t action = cmd->zsa;
bool all;
enum NvmeZoneProcessingMask proc_mask = NVME_PROC_CURRENT_ZONE;
-action = dw13 & 0xff;
-all = !!(dw13 & 0x100);
+all = cmd->zsflags[0] & NVME_ZSFLAG_SELECT_ALL;
req->status = NVME_SUCCESS;
if (!all) {
-status = nvme_get_mgmt_zone_slba_idx(ns, cmd, , _idx);
+status = nvme_get_mgmt_zone_slba_idx(ns, >cmd, , _idx);
if (status) {
return status;
}
diff --git a/include/block/nvme.h b/include/block/nvme.h
index e3bd47bf76ab..2ee227760265 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1433,6 +1433,20 @@ enum NvmeZoneType {
NVME_ZONE_TYPE_SEQ_WRITE = 0x02,
};
+typedef struct QEMU_PACKED NvmeZoneSendCmd {
+uint8_t opcode;
+uint8_t flags;
+uint16_tcid;
+uint32_tnsid;
+uint32_trsvd2[4];
+NvmeCmdDptr dptr;
+uint64_tslba;
+uint32_trsvd12;
+uint8_t zsa;
+uint8_t zsflags[3];
+uint32_trsvd14[2];
+} NvmeZoneSendCmd;
+
enum NvmeZoneSendAction {
NVME_ZONE_ACTION_RSD = 0x00,
NVME_ZONE_ACTION_CLOSE = 0x01,
@@ -1443,6 +1457,10 @@ enum NvmeZoneSendAction {
NVME_ZONE_ACTION_SET_ZD_EXT = 0x10,
};
+enum {
+NVME_ZSFLAG_SELECT_ALL = 1 << 0,
+};
+
typedef struct QEMU_PACKED NvmeZoneDescr {
uint8_t zt;
uint8_t zs;
--
2.34.0
[PATCH for-7.0 0/4] hw/nvme: zoned random write area
From: Klaus Jensen
This series adds support for a zoned random write area as standardized
in TP 4076 ("Zoned Random Write Area").
Klaus Jensen (4):
hw/nvme: add struct for zone management send
hw/nvme: add zone attribute get/set helpers
hw/nvme: add ozcs enum
hw/nvme: add support for zoned random write area
hw/nvme/ctrl.c | 185 ---
hw/nvme/ns.c | 61 +-
hw/nvme/nvme.h | 10 +++
hw/nvme/trace-events | 1 +
include/block/nvme.h | 43 +-
5 files changed, 271 insertions(+), 29 deletions(-)
--
2.34.0
Re: [PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
On Tue, Nov 23, 2021 at 11:08 AM Hanna Reitz wrote:
> On 18.11.21 12:57, Philippe Mathieu-Daudé wrote:
> > Add the reproducer from
> https://gitlab.com/qemu-project/qemu/-/issues/339
> >
> > Without the previous commit, when running 'make check-qtest-i386'
> > with QEMU configured with '--enable-sanitizers' we get:
> >
> >==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x61962a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
> >READ of size 786432 at 0x61962a00 thread T0
> >#0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
> >#1 0x5626d1c023cc in flatview_write_continue
> softmmu/physmem.c:2787:13
> >#2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
> >#3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
> >#4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
> >#5 0x5626d1bf14c8 in cpu_physical_memory_rw
> softmmu/physmem.c:2933:5
> >#6 0x5626d0bd5649 in cpu_physical_memory_write
> include/exec/cpu-common.h:82:5
> >#7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
> >#8 0x5626d09f825d in fdctrl_transfer_handler
> hw/block/fdc.c:1616:13
> >#9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
> >#10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
> >#11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
> >#12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17
> >
> >0x61962a00 is located 0 bytes to the right of 512-byte region
> [0x61962800,0x61962a00)
> >allocated by thread T0 here:
> >#0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
> >#1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
> >#2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
> >#3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
> >#4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
> >#5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13
> >
> >SUMMARY: AddressSanitizer: heap-buffer-overflow
> (qemu-system-i386+0x1e65919) in __asan_memcpy
> >Shadow bytes around the buggy address:
> > 0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >=>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> > 0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> >Shadow byte legend (one shadow byte represents 8 application bytes):
> > Addressable: 00
> > Heap left redzone: fa
> > Freed heap region: fd
> >==4028352==ABORTING
> >
> > Reported-by: Alexander Bulekov
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > tests/qtest/fdc-test.c | 20
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> > index 26b69f7c5cd..f164d972d10 100644
> > --- a/tests/qtest/fdc-test.c
> > +++ b/tests/qtest/fdc-test.c
> > @@ -546,6 +546,25 @@ static void fuzz_registers(void)
> > }
> > }
> >
> > +static void test_cve_2021_3507(void)
> > +{
> > +QTestState *s;
> > +
> > +s = qtest_initf("-nographic -m 32M -nodefaults "
> > +"-drive file=%s,format=raw,if=floppy", test_image);
> > +qtest_outl(s, 0x9, 0x0a0206);
> > +qtest_outw(s, 0x3f4, 0x1600);
> > +qtest_outw(s, 0x3f4, 0x);
> > +qtest_outw(s, 0x3f4, 0x);
> > +qtest_outw(s, 0x3f4, 0x);
> > +qtest_outw(s, 0x3f4, 0x0200);
> > +qtest_outw(s, 0x3f4, 0x0200);
> > +qtest_outw(s, 0x3f4, 0x);
> > +qtest_outw(s, 0x3f4, 0x);
> > +qtest_outw(s, 0x3f4, 0x);
>
> No idea what this does (looks like a 256-byte sector read read from
> sector 2 with EOT=0), but hits the problem and reproduces for me.
>
> Unfortunately, I have the exact same problem with test_image as I did in
> the other series.
>
> Hanna
>
> > +qtest_quit(s);
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > int fd;
> > @@ -576,6 +595,7 @@ int main(int argc, char **argv)
> > qtest_add_func("/fdc/read_no_dma_18", test_read_no_dma_18);
> > qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
> > qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
> > +
Re: [PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196
On Wed, Nov 24, 2021 at 11:15 AM Philippe Mathieu-Daudé wrote: > Since v3: > - Preliminary extract blk_create_empty_drive() > - qtest checks qtest_check_clang_sanitizer() enabled > - qtest uses null-co:// driver instead of file > > Philippe Mathieu-Daudé (3): > hw/block/fdc: Extract blk_create_empty_drive() > hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 > tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 > > hw/block/fdc.c | 23 --- > tests/qtest/fdc-test.c | 38 ++ > 2 files changed, 58 insertions(+), 3 deletions(-) > > -- > 2.33.1 > > I'm testing this now. I'm going to take your word for it. If Hanna is fine with the block-layer components of the fix, I'll probably take it, but I will be sending a patch to remove myself as maintainer in the process, since I don't have the time to do the "proper fix" for these devices, and haven't for quite some time. --js
[PATCH 20/23] python/aqmp: fully separate from qmp.QEMUMonitorProtocol
After this patch, qemu.aqmp.legacy.QEMUMonitorProtocol no longer
inherits from qemu.qmp.QEMUMonitorProtocol. To do this, several
inherited methods need to be explicitly re-defined.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 38 --
1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index c8183de19a..71f3d378db 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -5,18 +5,18 @@
"""
import asyncio
+from types import TracebackType
from typing import (
Any,
Awaitable,
Dict,
List,
Optional,
+Type,
TypeVar,
Union,
)
-import qemu.qmp
-
from .error import AQMPError
from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
@@ -48,9 +48,9 @@ class QMPBadPortError(AQMPError):
"""
-class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+class QEMUMonitorProtocol:
def __init__(self, address: SocketAddrT,
- server: bool = False,
+ server: bool = False, # pylint: disable=unused-argument
nickname: Optional[str] = None):
# pylint: disable=super-init-not-called
@@ -74,7 +74,18 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return self._aqmp.greeting._asdict()
return None
-# __enter__ and __exit__ need no changes
+def __enter__(self: _T) -> _T:
+# Implement context manager enter function.
+return self
+
+def __exit__(self,
+ # pylint: disable=duplicate-code
+ # see https://github.com/PyCQA/pylint/issues/3619
+ exc_type: Optional[Type[BaseException]],
+ exc_val: Optional[BaseException],
+ exc_tb: Optional[TracebackType]) -> None:
+# Implement context manager exit function.
+self.close()
@classmethod
def parse_address(cls, address: str) -> SocketAddrT:
@@ -131,7 +142,22 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
)
)
-# Default impl of cmd() delegates to cmd_obj
+def cmd(self, name: str,
+args: Optional[Dict[str, object]] = None,
+cmd_id: Optional[object] = None) -> QMPMessage:
+"""
+Build a QMP command and send it to the QMP Monitor.
+
+@param name: command name (string)
+@param args: command arguments (dict)
+@param cmd_id: command id (dict, list, string or int)
+"""
+qmp_cmd: QMPMessage = {'execute': name}
+if args:
+qmp_cmd['arguments'] = args
+if cmd_id:
+qmp_cmd['id'] = cmd_id
+return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
return self._sync(
--
2.31.1
[PATCH 22/23] python: remove the old QMP package
Thank you for your service!
Signed-off-by: John Snow
---
python/PACKAGE.rst | 4 +-
python/README.rst | 2 +-
python/qemu/qmp/README.rst | 9 -
python/qemu/qmp/__init__.py | 396
python/qemu/qmp/py.typed| 0
python/setup.cfg| 3 +-
6 files changed, 4 insertions(+), 410 deletions(-)
delete mode 100644 python/qemu/qmp/README.rst
delete mode 100644 python/qemu/qmp/__init__.py
delete mode 100644 python/qemu/qmp/py.typed
diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
index b0b86cc4c3..ddfa9ba3f5 100644
--- a/python/PACKAGE.rst
+++ b/python/PACKAGE.rst
@@ -8,11 +8,11 @@ to change at any time.
Usage
-
-The ``qemu.qmp`` subpackage provides a library for communicating with
+The ``qemu.aqmp`` subpackage provides a library for communicating with
QMP servers. The ``qemu.machine`` subpackage offers rudimentary
facilities for launching and managing QEMU processes. Refer to each
package's documentation
-(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+(``>>> help(qemu.aqmp)``, ``>>> help(qemu.machine)``)
for more information.
Contributing
diff --git a/python/README.rst b/python/README.rst
index fcf74f69ea..eb5213337d 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -3,7 +3,7 @@ QEMU Python Tooling
This directory houses Python tooling used by the QEMU project to build,
configure, and test QEMU. It is organized by namespace (``qemu``), and
-then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
+then by package (e.g. ``qemu/machine``, ``qemu/aqmp``, etc).
``setup.py`` is used by ``pip`` to install this tooling to the current
environment. ``setup.cfg`` provides the packaging configuration used by
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
deleted file mode 100644
index 5bfb82535f..00
--- a/python/qemu/qmp/README.rst
+++ /dev/null
@@ -1,9 +0,0 @@
-qemu.qmp package
-
-
-This package provides a library used for connecting to and communicating
-with QMP servers. It is used extensively by iotests, vm tests,
-avocado tests, and other utilities in the ./scripts directory. It is
-not a fully-fledged SDK and is subject to change at any time.
-
-See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
deleted file mode 100644
index 4e08641154..00
--- a/python/qemu/qmp/__init__.py
+++ /dev/null
@@ -1,396 +0,0 @@
-"""
-QEMU Monitor Protocol (QMP) development library & tooling.
-
-This package provides a fairly low-level class for communicating to QMP
-protocol servers, as implemented by QEMU, the QEMU Guest Agent, and the
-QEMU Storage Daemon. This library is not intended for production use.
-
-`QEMUMonitorProtocol` is the primary class of interest, and all errors
-raised derive from `QMPError`.
-"""
-
-# Copyright (C) 2009, 2010 Red Hat Inc.
-#
-# Authors:
-# Luiz Capitulino
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import errno
-import json
-import logging
-import socket
-import struct
-from types import TracebackType
-from typing import (
-Any,
-Dict,
-List,
-Optional,
-TextIO,
-Tuple,
-Type,
-TypeVar,
-Union,
-cast,
-)
-
-
-#: QMPMessage is an entire QMP message of any kind.
-QMPMessage = Dict[str, Any]
-
-#: QMPReturnValue is the 'return' value of a command.
-QMPReturnValue = object
-
-#: QMPObject is any object in a QMP message.
-QMPObject = Dict[str, object]
-
-# QMPMessage can be outgoing commands or incoming events/returns.
-# QMPReturnValue is usually a dict/json object, but due to QAPI's
-# 'returns-whitelist', it can actually be anything.
-#
-# {'return': {}} is a QMPMessage,
-# {} is the QMPReturnValue.
-
-
-InternetAddrT = Tuple[str, int]
-UnixAddrT = str
-SocketAddrT = Union[InternetAddrT, UnixAddrT]
-
-
-class QMPError(Exception):
-"""
-QMP base exception
-"""
-
-
-class QMPConnectError(QMPError):
-"""
-QMP connection exception
-"""
-
-
-class QMPCapabilitiesError(QMPError):
-"""
-QMP negotiate capabilities exception
-"""
-
-
-class QMPTimeoutError(QMPError):
-"""
-QMP timeout exception
-"""
-
-
-class QMPProtocolError(QMPError):
-"""
-QMP protocol error; unexpected response
-"""
-
-
-class QMPResponseError(QMPError):
-"""
-Represents erroneous QMP monitor reply
-"""
-def __init__(self, reply: QMPMessage):
-try:
-desc = reply['error']['desc']
-except KeyError:
-desc = reply
-super().__init__(desc)
-self.reply = reply
-
-
-class QEMUMonitorProtocol:
-"""
-Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
-allow to handle commands and events.
-"""
-
-#: Logger object for debugging messages
-logger = logging.getLogger('QMP')
-
-def __init__(self,
[PATCH 13/23] scripts/cpu-x86-uarch-abi: switch to AQMP
Signed-off-by: John Snow
---
scripts/cpu-x86-uarch-abi.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 8963d90f0b..c262d2f027 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -6,7 +6,7 @@
# compatibility levels for each CPU model.
#
-from qemu import qmp
+from qemu.aqmp.legacy import QEMUMonitorProtocol
import sys
if len(sys.argv) != 2:
@@ -66,7 +66,7 @@
sock = sys.argv[1]
-shell = qmp.QEMUMonitorProtocol(sock)
+shell = QEMUMonitorProtocol(sock)
shell.connect()
models = shell.cmd("query-cpu-definitions")
--
2.31.1
[PATCH 12/23] scripts/cpu-x86-uarch-abi: fix CLI parsing
Signed-off-by: John Snow
---
scripts/cpu-x86-uarch-abi.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/cpu-x86-uarch-abi.py b/scripts/cpu-x86-uarch-abi.py
index 08acc52a81..8963d90f0b 100644
--- a/scripts/cpu-x86-uarch-abi.py
+++ b/scripts/cpu-x86-uarch-abi.py
@@ -9,7 +9,7 @@
from qemu import qmp
import sys
-if len(sys.argv) != 1:
+if len(sys.argv) != 2:
print("syntax: %s QMP-SOCK\n\n" % __file__ +
"Where QMP-SOCK points to a QEMU process such as\n\n" +
" # qemu-system-x86_64 -qmp unix:/tmp/qmp,server,nowait " +
@@ -66,7 +66,6 @@
sock = sys.argv[1]
-cmd = sys.argv[2]
shell = qmp.QEMUMonitorProtocol(sock)
shell.connect()
--
2.31.1
[PATCH 18/23] python: temporarily silence pylint duplicate-code warnings
The next several commits copy some code from qemu.qmp to qemu.aqmp, then delete qemu.qmp. In the interim, to prevent test failures, the duplicate code detection needs to be silenced to prevent bisect problems with CI testing. Signed-off-by: John Snow --- python/setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/python/setup.cfg b/python/setup.cfg index 168a79c867..510df23698 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -115,6 +115,7 @@ ignore_missing_imports = True disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. no-member, # mypy also handles this better. +duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.31.1
[PATCH 19/23] python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
Shift these definitions over from the qmp package to the async qmp
package.
Signed-off-by: John Snow
---
python/qemu/aqmp/aqmp_tui.py | 2 +-
python/qemu/aqmp/legacy.py | 30 ++
python/qemu/qmp/__init__.py | 26 --
3 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index a2929f771c..184a3e4690 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -35,8 +35,8 @@
import urwid
import urwid_readline
-from ..qmp import QEMUMonitorProtocol, QMPBadPortError
from .error import ProtocolError
+from .legacy import QEMUMonitorProtocol, QMPBadPortError
from .message import DeserializationError, Message, UnexpectedTypeError
from .protocol import ConnectError, Runstate
from .qmp_client import ExecInterruptedError, QMPClient
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 5d358d63db..c8183de19a 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -22,9 +22,6 @@
from .qmp_client import QMPClient
-# (Temporarily) Re-export QMPBadPortError
-QMPBadPortError = qemu.qmp.QMPBadPortError
-
#: QMPMessage is an entire QMP message of any kind.
QMPMessage = Dict[str, Any]
@@ -45,6 +42,12 @@
# pylint: disable=missing-docstring
+class QMPBadPortError(AQMPError):
+"""
+Unable to parse socket address: Port was non-numerical.
+"""
+
+
class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
def __init__(self, address: SocketAddrT,
server: bool = False,
@@ -72,7 +75,26 @@ def _get_greeting(self) -> Optional[QMPMessage]:
return None
# __enter__ and __exit__ need no changes
-# parse_address needs no changes
+
+@classmethod
+def parse_address(cls, address: str) -> SocketAddrT:
+"""
+Parse a string into a QMP address.
+
+Figure out if the argument is in the port:host form.
+If it's not, it's probably a file path.
+"""
+components = address.split(':')
+if len(components) == 2:
+try:
+port = int(components[1])
+except ValueError:
+msg = f"Bad port: '{components[1]}' in '{address}'."
+raise QMPBadPortError(msg) from None
+return (components[0], port)
+
+# Treat as filepath.
+return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
self._aqmp.await_greeting = negotiate
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 358c0971d0..4e08641154 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -102,12 +102,6 @@ def __init__(self, reply: QMPMessage):
self.reply = reply
-class QMPBadPortError(QMPError):
-"""
-Unable to parse socket address: Port was non-numerical.
-"""
-
-
class QEMUMonitorProtocol:
"""
Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -237,26 +231,6 @@ def __exit__(self,
# Implement context manager exit function.
self.close()
-@classmethod
-def parse_address(cls, address: str) -> SocketAddrT:
-"""
-Parse a string into a QMP address.
-
-Figure out if the argument is in the port:host form.
-If it's not, it's probably a file path.
-"""
-components = address.split(':')
-if len(components) == 2:
-try:
-port = int(components[1])
-except ValueError:
-msg = f"Bad port: '{components[1]}' in '{address}'."
-raise QMPBadPortError(msg) from None
-return (components[0], port)
-
-# Treat as filepath.
-return address
-
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
"""
Connect to the QMP Monitor and perform capabilities negotiation.
--
2.31.1
[PATCH 23/23] python: re-enable pylint duplicate-code warnings
With the old library gone, there's nothing duplicated in the tree, so the warning suppression can be removed. Signed-off-by: John Snow --- python/setup.cfg | 1 - 1 file changed, 1 deletion(-) diff --git a/python/setup.cfg b/python/setup.cfg index 5140a5b322..c341e922c2 100644 --- a/python/setup.cfg +++ b/python/setup.cfg @@ -114,7 +114,6 @@ ignore_missing_imports = True disable=consider-using-f-string, too-many-function-args, # mypy handles this with less false positives. no-member, # mypy also handles this better. -duplicate-code, # To be removed by the end of this patch series. [pylint.basic] # Good variable names which should always be accepted, separated by a comma. -- 2.31.1
[PATCH 21/23] python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
Copy the docstrings out of qemu.qmp, adjusting them as necessary to
more accurately reflect the current state of this class.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 110 ++---
1 file changed, 102 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 71f3d378db..5f7955ac5d 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -1,9 +1,23 @@
"""
-Sync QMP Wrapper
+(Legacy) Sync QMP Wrapper
-This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+This module provides the `QEMUMonitorProtocol` class, which is a
+synchronous wrapper around `QMPClient`.
+
+Its design closely resembles that of the original QEMUMonitorProtocol
+class, originally written by Luiz Capitulino.
"""
+# Copyright (C) 2009, 2010, 2021 Red Hat Inc.
+#
+# Authors:
+# Luiz Capitulino
+# John Snow
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+
import asyncio
from types import TracebackType
from typing import (
@@ -39,9 +53,6 @@
# {} is the QMPReturnValue.
-# pylint: disable=missing-docstring
-
-
class QMPBadPortError(AQMPError):
"""
Unable to parse socket address: Port was non-numerical.
@@ -49,6 +60,21 @@ class QMPBadPortError(AQMPError):
class QEMUMonitorProtocol:
+"""
+Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP)
+and then allow to handle commands and events.
+
+:param address: QEMU address, can be either a unix socket path (string)
+ or a tuple in the form ( address, port ) for a TCP
+ connection
+:param server: Deprecated, ignored. (See 'accept')
+:param nickname: Optional nickname used for logging.
+
+..note::
+No connection is established during `__init__`, this is done by
+the `connect()` or `accept()` methods.
+"""
+
def __init__(self, address: SocketAddrT,
server: bool = False, # pylint: disable=unused-argument
nickname: Optional[str] = None):
@@ -108,6 +134,12 @@ def parse_address(cls, address: str) -> SocketAddrT:
return address
def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+"""
+Connect to the QMP Monitor and perform capabilities negotiation.
+
+:return: QMP greeting dict, or None if negotiate is false
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = negotiate
self._aqmp.negotiate = negotiate
@@ -117,6 +149,16 @@ def connect(self, negotiate: bool = True) ->
Optional[QMPMessage]:
return self._get_greeting()
def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+"""
+Await connection from QMP Monitor and perform capabilities negotiation.
+
+:param timeout:
+timeout in seconds (nonnegative float number, or None).
+If None, there is no timeout, and this may block forever.
+
+:return: QMP greeting dict
+:raise ConnectError: on connection errors
+"""
self._aqmp.await_greeting = True
self._aqmp.negotiate = True
@@ -130,6 +172,12 @@ def accept(self, timeout: Optional[float] = 15.0) ->
QMPMessage:
return ret
def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+"""
+Send a QMP command to the QMP Monitor.
+
+:param qmp_cmd: QMP command to be sent as a Python dict
+:return: QMP response as a Python dict
+"""
return dict(
self._sync(
# pylint: disable=protected-access
@@ -148,9 +196,9 @@ def cmd(self, name: str,
"""
Build a QMP command and send it to the QMP Monitor.
-@param name: command name (string)
-@param args: command arguments (dict)
-@param cmd_id: command id (dict, list, string or int)
+:param name: command name (string)
+:param args: command arguments (dict)
+:param cmd_id: command id (dict, list, string or int)
"""
qmp_cmd: QMPMessage = {'execute': name}
if args:
@@ -160,6 +208,9 @@ def cmd(self, name: str,
return self.cmd_obj(qmp_cmd)
def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+"""
+Build and send a QMP command to the monitor, report errors if any
+"""
return self._sync(
self._aqmp.execute(cmd, kwds),
self._timeout
@@ -167,6 +218,19 @@ def command(self, cmd: str, **kwds: object) ->
QMPReturnValue:
def pull_event(self,
wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+"""
+Pulls a single event.
+
+:param wait:
+If False or 0, do not wait. Return None if no events ready.
+If True, wait forever until the next event.
+Otherwise,
[PATCH 14/23] scripts/render-block-graph: switch to AQMP
Creating an instance of qemu.aqmp.ExecuteError is too involved here, so just drop the specificity down to a generic AQMPError. Signed-off-by: John Snow --- scripts/render_block_graph.py | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py index da6acf050d..d2f3a72348 100755 --- a/scripts/render_block_graph.py +++ b/scripts/render_block_graph.py @@ -25,10 +25,8 @@ from graphviz import Digraph sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python')) -from qemu.qmp import ( -QEMUMonitorProtocol, -QMPResponseError, -) +from qemu.aqmp import AQMPError +from qemu.aqmp.legacy import QEMUMonitorProtocol def perm(arr): @@ -105,7 +103,7 @@ def command(self, cmd): reply = json.loads(subprocess.check_output(ar)) if 'error' in reply: -raise QMPResponseError(reply) +raise AQMPError(reply) return reply['return'] -- 2.31.1
[PATCH 15/23] scripts/bench-block-job: switch to AQMP
For this commit, we only need to remove accommodations for the
synchronous QMP library.
Signed-off-by: John Snow
---
scripts/simplebench/bench_block_job.py | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index a403c35b08..af9d1646a4 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -27,7 +27,6 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
-from qemu.qmp import QMPConnectError
from qemu.aqmp import ConnectError
@@ -50,7 +49,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.launch()
except OSError as e:
return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, ConnectError, socket.timeout):
+except (ConnectError, socket.timeout):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
--
2.31.1
[PATCH 17/23] iotests: switch to AQMP
Simply import the type defition from the new location.
Signed-off-by: John Snow
---
tests/qemu-iotests/iotests.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 83bfedb902..cb21aebe36 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,7 +37,7 @@
from contextlib import contextmanager
from qemu.machine import qtest
-from qemu.qmp import QMPMessage
+from qemu.aqmp.legacy import QMPMessage
# Use this logger for logging messages directly from the iotests module
logger = logging.getLogger('qemu.iotests')
--
2.31.1
[PATCH 16/23] iotests/mirror-top-perms: switch to AQMP
Signed-off-by: John Snow
---
Note: I still need to adjust the logging. The problem now is that the
logging messages include the PID of the test process, so they need to be
filtered out. I'll investigate that for a follow-up, or for v2.
I could just add yet another filtering function somewhere, but I think
it's getting out of hand with how many filters and loggers there are, so
I want to give it a slightly more serious treatment instead of a
hackjob.
Signed-off-by: John Snow
---
tests/qemu-iotests/tests/mirror-top-perms | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/mirror-top-perms
b/tests/qemu-iotests/tests/mirror-top-perms
index 0a51a613f3..f394931a00 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -23,7 +23,6 @@ import os
from qemu.aqmp import ConnectError
from qemu.machine import machine
-from qemu.qmp import QMPConnectError
import iotests
from iotests import change_log_level, qemu_img
@@ -101,13 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
try:
# Silence AQMP errors temporarily.
-# TODO: Remove this and just allow the errors to be logged when
-# AQMP fully replaces QMP.
+# TODO: Remove change_log_level and allow the errors to be logged.
+# This necessitates a PID filter on *all* logging output.
with change_log_level('qemu.aqmp'):
self.vm_b.launch()
print('ERROR: VM B launched successfully, '
'this should not have happened')
-except (QMPConnectError, ConnectError):
+except ConnectError:
assert 'Is another process using the image' in self.vm_b.get_log()
result = self.vm.qmp('block-job-cancel',
--
2.31.1
[PATCH 10/23] python: move qmp-shell under the AQMP package
Signed-off-by: John Snow
---
python/README.rst | 2 +-
python/qemu/{qmp => aqmp}/qmp_shell.py | 0
python/setup.cfg | 2 +-
scripts/qmp/qmp-shell | 2 +-
4 files changed, 3 insertions(+), 3 deletions(-)
rename python/qemu/{qmp => aqmp}/qmp_shell.py (100%)
diff --git a/python/README.rst b/python/README.rst
index 9c1fceaee7..fcf74f69ea 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -59,7 +59,7 @@ Package installation also normally provides executable
console scripts,
so that tools like ``qmp-shell`` are always available via $PATH. To
invoke them without installation, you can invoke e.g.:
-``> PYTHONPATH=~/src/qemu/python python3 -m qemu.qmp.qmp_shell``
+``> PYTHONPATH=~/src/qemu/python python3 -m qemu.aqmp.qmp_shell``
The mappings between console script name and python module path can be
found in ``setup.cfg``.
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
similarity index 100%
rename from python/qemu/qmp/qmp_shell.py
rename to python/qemu/aqmp/qmp_shell.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 78421411d2..168a79c867 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -67,7 +67,7 @@ console_scripts =
qom-tree = qemu.utils.qom:QOMTree.entry_point
qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
qemu-ga-client = qemu.utils.qemu_ga_client:main
-qmp-shell = qemu.qmp.qmp_shell:main
+qmp-shell = qemu.aqmp.qmp_shell:main
aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
[flake8]
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 4a20f97db7..31b19d73e2 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import qmp_shell
+from qemu.aqmp import qmp_shell
if __name__ == '__main__':
--
2.31.1
[PATCH 11/23] python/machine: permanently switch to AQMP
Remove the QEMU_PYTHON_LEGACY_QMP environment variable, making the
switch permanent. Update Exceptions and import paths as necessary.
Signed-off-by: John Snow
---
python/qemu/machine/machine.py | 18 +++---
python/qemu/machine/qtest.py | 2 +-
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 67ab06ca2b..21fb4a4f30 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -40,21 +40,16 @@
TypeVar,
)
-from qemu.qmp import ( # pylint: disable=import-error
+from qemu.aqmp import SocketAddrT
+from qemu.aqmp.legacy import (
+QEMUMonitorProtocol,
QMPMessage,
QMPReturnValue,
-SocketAddrT,
)
from . import console_socket
-if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
-from qemu.qmp import QEMUMonitorProtocol
-else:
-from qemu.aqmp.legacy import QEMUMonitorProtocol
-
-
LOG = logging.getLogger(__name__)
@@ -710,8 +705,9 @@ def events_wait(self,
:param timeout: Optional timeout, in seconds.
See QEMUMonitorProtocol.pull_event.
-:raise QMPTimeoutError: If timeout was non-zero and no matching events
-were found.
+:raise asyncio.TimeoutError:
+If timeout was non-zero and no matching events were found.
+
:return: A QMP event matching the filter criteria.
If timeout was 0 and no event matched, None.
"""
@@ -734,7 +730,7 @@ def _match(event: QMPMessage) -> bool:
event = self._qmp.pull_event(wait=timeout)
if event is None:
# NB: None is only returned when timeout is false-ish.
-# Timeouts raise QMPTimeoutError instead!
+# Timeouts raise asyncio.TimeoutError instead!
break
if _match(event):
return event
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index f2f9aaa5e5..817c8a5425 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
TextIO,
)
-from qemu.qmp import SocketAddrT # pylint: disable=import-error
+from qemu.aqmp.protocol import SocketAddrT
from .machine import QEMUMachine
--
2.31.1
[PATCH 00/23] Python: delete qemu.qmp package
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qmp-legacy-switch
CI: https://gitlab.com/jsnow/qemu/-/pipelines/415801786
NOT intended for 6.2.
This series swaps out qemu.qmp for qemu.aqmp permanently, instead of
hiding it behind an environment variable toggle. This leaves us with
just one QMP library to worry about.
The series is organized like this:
- 01-02: Fixes and improvements to Async QMP
- 03-11: Switch python/* users to use AQMP exclusively
- 12-17: Switch other users to use AQMP exclusively
- 18-23: Finalize the switchover, delete python/qemu/qmp.
Optional notes about the broader process of moving Python infrastructure
onto PyPI are below, though it isn't required reading for reviewing this
series. Consider it a newsletter from the Python dungeon:
I was asked what the timeline for actually uploading anything to PyPI
was. This series is part of my answer, but the steps look like this:
Phase I:
- Refactor everything in-tree to be a bona-fide python package [Done]
- Add unit testing and CI for all QEMU python packages [Done]
- Develop a version of QMP intended for public support via PyPI [Done]
Phase II:
- Switch machine.py and iotests to using async QMP by default [Done]
- Fix bugs in qemu.aqmp discovered during RC testing [Ongoing]
- Remove qemu.qmp in favor of qemu.aqmp[This Series]
- Rename qemu.aqmp back to qemu.qmp
- Add a proper "sync" version of qemu.aqmp.QMPClient [In Progress]
designed to be more supportable via PyPI
current status: it's functional, but there are some FIXMEs.
- Pivot in-tree users of qemu.(a)qmp.legacy to qemu.qmp.sync,
-OR- move the "legacy" wrapper outside of the qmp package and into utils.
(The goal is simply to avoid uploading the legacy wrapper to PyPI.)
Phase III:
- Fork python/qemu/qmp into its own git repo, with its own pkg version
- Add sphinx doc generation to qemu.qmp repo and add readthedocs integration
[Doc generation is 95% done on a branch, needs polish. RTD is untouched.]
- Convert in-tree users of qemu.qmp to pull the dependency from either
PyPI or a git URL. I think I'd like to avoid using git submodules ...
That's broadly it. There's some code to do for the sync bridge to make
the design tidier, but the goal there is to move a lot of the QMP event
wrangling functions we have scattered across qmp, machine, and even
iotests into a more central location with much stronger support.
A lot of this will hopefully move pretty fast once the tree re-opens.
One of the remaining skeletons in the closet that I have not yet fully
addressed is how I will be moving remaining in-tree users of the QMP
package onto a PyPI dependency. That's probably where most of the work
will actually be; adding a python virtual environment to iotests et al.
John Snow (23):
python/aqmp: add __del__ method to legacy interface
python/aqmp: handle asyncio.TimeoutError on execute()
python/aqmp: copy type definitions from qmp
python/aqmp: add SocketAddrT to package root
python/qemu-ga-client: update instructions to newer CLI syntax
python/qmp: switch qemu-ga-client to AQMP
python/qmp: switch qom tools to AQMP
python/qmp: switch qmp-shell to AQMP
python: move qmp utilities to python/qemu/utils
python: move qmp-shell under the AQMP package
python/machine: permanently switch to AQMP
scripts/cpu-x86-uarch-abi: fix CLI parsing
scripts/cpu-x86-uarch-abi: switch to AQMP
scripts/render-block-graph: switch to AQMP
scripts/bench-block-job: switch to AQMP
iotests/mirror-top-perms: switch to AQMP
iotests: switch to AQMP
python: temporarily silence pylint duplicate-code warnings
python/aqmp: take QMPBadPortError and parse_address from qemu.qmp
python/aqmp: fully separate from qmp.QEMUMonitorProtocol
python/aqmp: copy qmp docstrings to qemu.aqmp.legacy
python: remove the old QMP package
python: re-enable pylint duplicate-code warnings
python/PACKAGE.rst | 4 +-
python/README.rst| 4 +-
python/qemu/qmp/README.rst | 9 -
python/qemu/aqmp/__init__.py | 10 +-
python/qemu/aqmp/aqmp_tui.py | 2 +-
python/qemu/aqmp/legacy.py | 203 -
python/qemu/aqmp/protocol.py | 16 +-
python/qemu/aqmp/qmp_client.py | 8 +-
python/qemu/{qmp => aqmp}/qmp_shell.py | 31 +-
python/qemu/machine/machine.py | 18 +-
python/qemu/machine/qtest.py | 2 +-
python/qemu/qmp/__init__.py | 422 ---
python/qemu/qmp/py.typed | 0
python/qemu/{qmp => utils}/qemu_ga_client.py | 24 +-
python/qemu/{qmp => utils}/qom.py| 5 +-
python/qemu/{qmp => utils}/qom_common.py | 7 +-
python/qemu/{qmp => utils}/qom_fuse.py | 11 +-
python/setup.cfg | 21 +-
scripts/cpu-x86-uarch-abi.py | 7 +-
[PATCH 06/23] python/qmp: switch qemu-ga-client to AQMP
Signed-off-by: John Snow
---
python/qemu/qmp/qemu_ga_client.py | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/python/qemu/qmp/qemu_ga_client.py
b/python/qemu/qmp/qemu_ga_client.py
index b3e1d98c9e..15ed430c61 100644
--- a/python/qemu/qmp/qemu_ga_client.py
+++ b/python/qemu/qmp/qemu_ga_client.py
@@ -37,8 +37,8 @@
# the COPYING file in the top-level directory.
import argparse
+import asyncio
import base64
-import errno
import os
import random
import sys
@@ -50,8 +50,8 @@
Sequence,
)
-from qemu import qmp
-from qemu.qmp import SocketAddrT
+from qemu.aqmp import ConnectError, SocketAddrT
+from qemu.aqmp.legacy import QEMUMonitorProtocol
# This script has not seen many patches or careful attention in quite
@@ -61,7 +61,7 @@
# pylint: disable=missing-docstring
-class QemuGuestAgent(qmp.QEMUMonitorProtocol):
+class QemuGuestAgent(QEMUMonitorProtocol):
def __getattr__(self, name: str) -> Callable[..., Any]:
def wrapper(**kwds: object) -> object:
return self.command('guest-' + name.replace('_', '-'), **kwds)
@@ -149,7 +149,7 @@ def ping(self, timeout: Optional[float]) -> bool:
self.qga.settimeout(timeout)
try:
self.qga.ping()
-except TimeoutError:
+except asyncio.TimeoutError:
return False
return True
@@ -172,7 +172,7 @@ def suspend(self, mode: str) -> None:
try:
getattr(self.qga, 'suspend' + '_' + mode)()
# On error exception will raise
-except TimeoutError:
+except asyncio.TimeoutError:
# On success command will timed out
return
@@ -182,7 +182,7 @@ def shutdown(self, mode: str = 'powerdown') -> None:
try:
self.qga.shutdown(mode=mode)
-except TimeoutError:
+except asyncio.TimeoutError:
pass
@@ -277,7 +277,7 @@ def _cmd_reboot(client: QemuGuestAgentClient, args:
Sequence[str]) -> None:
def send_command(address: str, cmd: str, args: Sequence[str]) -> None:
if not os.path.exists(address):
-print('%s not found' % address)
+print(f"'{address}' not found. (Is QEMU running?)")
sys.exit(1)
if cmd not in commands:
@@ -287,10 +287,10 @@ def send_command(address: str, cmd: str, args:
Sequence[str]) -> None:
try:
client = QemuGuestAgentClient(address)
-except OSError as err:
+except ConnectError as err:
print(err)
-if err.errno == errno.ECONNREFUSED:
-print('Hint: qemu is not running?')
+if isinstance(err.exc, ConnectionError):
+print('(Is QEMU running?)')
sys.exit(1)
if cmd == 'fsfreeze' and args[0] == 'freeze':
--
2.31.1
[PATCH 07/23] python/qmp: switch qom tools to AQMP
Signed-off-by: John Snow
---
python/qemu/qmp/qom.py| 5 +++--
python/qemu/qmp/qom_common.py | 7 ---
python/qemu/qmp/qom_fuse.py | 11 ++-
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/python/qemu/qmp/qom.py b/python/qemu/qmp/qom.py
index 8ff28a8343..bb5d1a78f5 100644
--- a/python/qemu/qmp/qom.py
+++ b/python/qemu/qmp/qom.py
@@ -32,7 +32,8 @@
import argparse
-from . import QMPResponseError
+from qemu.aqmp import ExecuteError
+
from .qom_common import QOMCommand
@@ -233,7 +234,7 @@ def _list_node(self, path: str) -> None:
rsp = self.qmp.command('qom-get', path=path,
property=item.name)
print(f" {item.name}: {rsp} ({item.type})")
-except QMPResponseError as err:
+except ExecuteError as err:
print(f" {item.name}: ({item.type})")
print('')
for item in items:
diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/qmp/qom_common.py
index a59ae1a2a1..b145157258 100644
--- a/python/qemu/qmp/qom_common.py
+++ b/python/qemu/qmp/qom_common.py
@@ -27,7 +27,8 @@
TypeVar,
)
-from . import QEMUMonitorProtocol, QMPError
+from qemu.aqmp import AQMPError
+from qemu.aqmp.legacy import QEMUMonitorProtocol
# The following is needed only for a type alias.
@@ -82,7 +83,7 @@ class QOMCommand:
def __init__(self, args: argparse.Namespace):
if args.socket is None:
-raise QMPError("No QMP socket path or address given")
+raise AQMPError("No QMP socket path or address given")
self.qmp = QEMUMonitorProtocol(
QEMUMonitorProtocol.parse_address(args.socket)
)
@@ -161,7 +162,7 @@ def command_runner(
try:
cmd = cls(args)
return cmd.run()
-except QMPError as err:
+except AQMPError as err:
print(f"{type(err).__name__}: {err!s}", file=sys.stderr)
return -1
diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/qmp/qom_fuse.py
index 43f4671fdb..653a76b93b 100644
--- a/python/qemu/qmp/qom_fuse.py
+++ b/python/qemu/qmp/qom_fuse.py
@@ -48,7 +48,8 @@
import fuse
from fuse import FUSE, FuseOSError, Operations
-from . import QMPResponseError
+from qemu.aqmp import ExecuteError
+
from .qom_common import QOMCommand
@@ -99,7 +100,7 @@ def is_object(self, path: str) -> bool:
try:
self.qom_list(path)
return True
-except QMPResponseError:
+except ExecuteError:
return False
def is_property(self, path: str) -> bool:
@@ -112,7 +113,7 @@ def is_property(self, path: str) -> bool:
if item.name == prop:
return True
return False
-except QMPResponseError:
+except ExecuteError:
return False
def is_link(self, path: str) -> bool:
@@ -125,7 +126,7 @@ def is_link(self, path: str) -> bool:
if item.name == prop and item.link:
return True
return False
-except QMPResponseError:
+except ExecuteError:
return False
def read(self, path: str, size: int, offset: int, fh: IO[bytes]) -> bytes:
@@ -138,7 +139,7 @@ def read(self, path: str, size: int, offset: int, fh:
IO[bytes]) -> bytes:
try:
data = str(self.qmp.command('qom-get', path=path, property=prop))
data += '\n' # make values shell friendly
-except QMPResponseError as err:
+except ExecuteError as err:
raise FuseOSError(EPERM) from err
if offset > len(data):
--
2.31.1
[PATCH 08/23] python/qmp: switch qmp-shell to AQMP
We have a replacement for async QMP, but it doesn't have feature parity
yet. For now, then, port the old tool onto the new backend.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 3 +++
python/qemu/qmp/qmp_shell.py | 31 +--
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 9431fe9330..5d358d63db 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -22,6 +22,9 @@
from .qmp_client import QMPClient
+# (Temporarily) Re-export QMPBadPortError
+QMPBadPortError = qemu.qmp.QMPBadPortError
+
#: QMPMessage is an entire QMP message of any kind.
QMPMessage = Dict[str, Any]
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index e7d7eb18f1..2260ae016e 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -95,8 +95,13 @@
Sequence,
)
-from qemu import qmp
-from qemu.qmp import QMPMessage
+from qemu.aqmp import AQMPError, ConnectError, SocketAddrT
+from qemu.aqmp.legacy import (
+QEMUMonitorProtocol,
+QMPBadPortError,
+QMPMessage,
+QMPObject,
+)
LOG = logging.getLogger(__name__)
@@ -125,7 +130,7 @@ def complete(self, text: str, state: int) -> Optional[str]:
return None
-class QMPShellError(qmp.QMPError):
+class QMPShellError(AQMPError):
"""
QMP Shell Base error class.
"""
@@ -153,7 +158,7 @@ def visit_Name(cls, # pylint: disable=invalid-name
return node
-class QMPShell(qmp.QEMUMonitorProtocol):
+class QMPShell(QEMUMonitorProtocol):
"""
QMPShell provides a basic readline-based QMP shell.
@@ -161,7 +166,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
:param pretty: Pretty-print QMP messages.
:param verbose: Echo outgoing QMP messages to console.
"""
-def __init__(self, address: qmp.SocketAddrT,
+def __init__(self, address: SocketAddrT,
pretty: bool = False, verbose: bool = False):
super().__init__(address)
self._greeting: Optional[QMPMessage] = None
@@ -237,7 +242,7 @@ def _parse_value(cls, val: str) -> object:
def _cli_expr(self,
tokens: Sequence[str],
- parent: qmp.QMPObject) -> None:
+ parent: QMPObject) -> None:
for arg in tokens:
(key, sep, val) = arg.partition('=')
if sep != '=':
@@ -403,7 +408,7 @@ class HMPShell(QMPShell):
:param pretty: Pretty-print QMP messages.
:param verbose: Echo outgoing QMP messages to console.
"""
-def __init__(self, address: qmp.SocketAddrT,
+def __init__(self, address: SocketAddrT,
pretty: bool = False, verbose: bool = False):
super().__init__(address, pretty, verbose)
self._cpu_index = 0
@@ -512,19 +517,17 @@ def main() -> None:
try:
address = shell_class.parse_address(args.qmp_server)
-except qmp.QMPBadPortError:
+except QMPBadPortError:
parser.error(f"Bad port number: {args.qmp_server}")
return # pycharm doesn't know error() is noreturn
with shell_class(address, args.pretty, args.verbose) as qemu:
try:
qemu.connect(negotiate=not args.skip_negotiation)
-except qmp.QMPConnectError:
-die("Didn't get QMP greeting message")
-except qmp.QMPCapabilitiesError:
-die("Couldn't negotiate capabilities")
-except OSError as err:
-die(f"Couldn't connect to {args.qmp_server}: {err!s}")
+except ConnectError as err:
+if isinstance(err.exc, OSError):
+die(f"Couldn't connect to {args.qmp_server}: {err!s}")
+die(str(err))
for _ in qemu.repl():
pass
--
2.31.1
[PATCH 09/23] python: move qmp utilities to python/qemu/utils
In order to upload a QMP package to PyPI, I want to remove any scripts
that I am not 100% confident I want to support upstream, beyond our
castle walls.
Move most of our QMP utilities into the utils package so we can split
them out from the PyPI upload.
Signed-off-by: John Snow
---
python/qemu/{qmp => utils}/qemu_ga_client.py | 0
python/qemu/{qmp => utils}/qom.py| 0
python/qemu/{qmp => utils}/qom_common.py | 0
python/qemu/{qmp => utils}/qom_fuse.py | 0
python/setup.cfg | 16
scripts/qmp/qemu-ga-client | 2 +-
scripts/qmp/qom-fuse | 2 +-
scripts/qmp/qom-get | 2 +-
scripts/qmp/qom-list | 2 +-
scripts/qmp/qom-set | 2 +-
scripts/qmp/qom-tree | 2 +-
11 files changed, 14 insertions(+), 14 deletions(-)
rename python/qemu/{qmp => utils}/qemu_ga_client.py (100%)
rename python/qemu/{qmp => utils}/qom.py (100%)
rename python/qemu/{qmp => utils}/qom_common.py (100%)
rename python/qemu/{qmp => utils}/qom_fuse.py (100%)
diff --git a/python/qemu/qmp/qemu_ga_client.py
b/python/qemu/utils/qemu_ga_client.py
similarity index 100%
rename from python/qemu/qmp/qemu_ga_client.py
rename to python/qemu/utils/qemu_ga_client.py
diff --git a/python/qemu/qmp/qom.py b/python/qemu/utils/qom.py
similarity index 100%
rename from python/qemu/qmp/qom.py
rename to python/qemu/utils/qom.py
diff --git a/python/qemu/qmp/qom_common.py b/python/qemu/utils/qom_common.py
similarity index 100%
rename from python/qemu/qmp/qom_common.py
rename to python/qemu/utils/qom_common.py
diff --git a/python/qemu/qmp/qom_fuse.py b/python/qemu/utils/qom_fuse.py
similarity index 100%
rename from python/qemu/qmp/qom_fuse.py
rename to python/qemu/utils/qom_fuse.py
diff --git a/python/setup.cfg b/python/setup.cfg
index 417e937839..78421411d2 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -60,13 +60,13 @@ tui =
[options.entry_points]
console_scripts =
-qom = qemu.qmp.qom:main
-qom-set = qemu.qmp.qom:QOMSet.entry_point
-qom-get = qemu.qmp.qom:QOMGet.entry_point
-qom-list = qemu.qmp.qom:QOMList.entry_point
-qom-tree = qemu.qmp.qom:QOMTree.entry_point
-qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
-qemu-ga-client = qemu.qmp.qemu_ga_client:main
+qom = qemu.utils.qom:main
+qom-set = qemu.utils.qom:QOMSet.entry_point
+qom-get = qemu.utils.qom:QOMGet.entry_point
+qom-list = qemu.utils.qom:QOMList.entry_point
+qom-tree = qemu.utils.qom:QOMTree.entry_point
+qom-fuse = qemu.utils.qom_fuse:QOMFuse.entry_point [fuse]
+qemu-ga-client = qemu.utils.qemu_ga_client:main
qmp-shell = qemu.qmp.qmp_shell:main
aqmp-tui = qemu.aqmp.aqmp_tui:main [tui]
@@ -80,7 +80,7 @@ python_version = 3.6
warn_unused_configs = True
namespace_packages = True
-[mypy-qemu.qmp.qom_fuse]
+[mypy-qemu.utils.qom_fuse]
# fusepy has no type stubs:
allow_subclassing_any = True
diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index 102fd2cad9..56edd0234a 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp import qemu_ga_client
+from qemu.utils import qemu_ga_client
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index a58c8ef979..d453807b27 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom_fuse import QOMFuse
+from qemu.utils.qom_fuse import QOMFuse
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index e4f3e0c013..04ebe052e8 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMGet
+from qemu.utils.qom import QOMGet
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 7a071a54e1..853b85a8d3 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMList
+from qemu.utils.qom import QOMList
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index 9ca9e2ba10..06820feec4 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -4,7 +4,7 @@ import os
import sys
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.qmp.qom import QOMSet
+from qemu.utils.qom import QOMSet
if __name__ == '__main__':
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index
[PATCH 03/23] python/aqmp: copy type definitions from qmp
Copy the remaining type definitions from QMP into the qemu.aqmp.legacy
module. Now, most users don't need to import anything else but
qemu.aqmp.legacy.
Signed-off-by: John Snow
---
python/qemu/aqmp/legacy.py | 22 --
python/qemu/aqmp/protocol.py | 16 ++--
2 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 2ccb136b02..9431fe9330 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -6,7 +6,9 @@
import asyncio
from typing import (
+Any,
Awaitable,
+Dict,
List,
Optional,
TypeVar,
@@ -14,13 +16,29 @@
)
import qemu.qmp
-from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
from .error import AQMPError
-from .protocol import Runstate
+from .protocol import Runstate, SocketAddrT
from .qmp_client import QMPClient
+#: QMPMessage is an entire QMP message of any kind.
+QMPMessage = Dict[str, Any]
+
+#: QMPReturnValue is the 'return' value of a command.
+QMPReturnValue = object
+
+#: QMPObject is any object in a QMP message.
+QMPObject = Dict[str, object]
+
+# QMPMessage can be outgoing commands or incoming events/returns.
+# QMPReturnValue is usually a dict/json object, but due to QAPI's
+# 'returns-whitelist', it can actually be anything.
+#
+# {'return': {}} is a QMPMessage,
+# {} is the QMPReturnValue.
+
+
# pylint: disable=missing-docstring
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 5190b33b13..42a897e2fe 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -46,6 +46,10 @@
_TaskFN = Callable[[], Awaitable[None]] # aka ``async def func() -> None``
_FutureT = TypeVar('_FutureT', bound=Optional['asyncio.Future[Any]'])
+InternetAddrT = Tuple[str, int]
+UnixAddrT = str
+SocketAddrT = Union[UnixAddrT, InternetAddrT]
+
class Runstate(Enum):
"""Protocol session runstate."""
@@ -257,7 +261,7 @@ async def runstate_changed(self) -> Runstate:
@upper_half
@require(Runstate.IDLE)
-async def accept(self, address: Union[str, Tuple[str, int]],
+async def accept(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Accept a connection and begin processing message queues.
@@ -275,7 +279,7 @@ async def accept(self, address: Union[str, Tuple[str, int]],
@upper_half
@require(Runstate.IDLE)
-async def connect(self, address: Union[str, Tuple[str, int]],
+async def connect(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Connect to the server and begin processing message queues.
@@ -337,7 +341,7 @@ def _set_state(self, state: Runstate) -> None:
@upper_half
async def _new_session(self,
- address: Union[str, Tuple[str, int]],
+ address: SocketAddrT,
ssl: Optional[SSLContext] = None,
accept: bool = False) -> None:
"""
@@ -397,7 +401,7 @@ async def _new_session(self,
@upper_half
async def _establish_connection(
self,
-address: Union[str, Tuple[str, int]],
+address: SocketAddrT,
ssl: Optional[SSLContext] = None,
accept: bool = False
) -> None:
@@ -424,7 +428,7 @@ async def _establish_connection(
await self._do_connect(address, ssl)
@upper_half
-async def _do_accept(self, address: Union[str, Tuple[str, int]],
+async def _do_accept(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Acting as the transport server, accept a single connection.
@@ -482,7 +486,7 @@ async def _client_connected_cb(reader: asyncio.StreamReader,
self.logger.debug("Connection accepted.")
@upper_half
-async def _do_connect(self, address: Union[str, Tuple[str, int]],
+async def _do_connect(self, address: SocketAddrT,
ssl: Optional[SSLContext] = None) -> None:
"""
Acting as the transport client, initiate a connection to a server.
--
2.31.1
[PATCH 02/23] python/aqmp: handle asyncio.TimeoutError on execute()
This exception can be injected into any await statement. If we are canceled via timeout, we want to clear the pending execution record on our way out. Signed-off-by: John Snow --- python/qemu/aqmp/qmp_client.py | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py index 8105e29fa8..6a985ffe30 100644 --- a/python/qemu/aqmp/qmp_client.py +++ b/python/qemu/aqmp/qmp_client.py @@ -435,7 +435,11 @@ async def _issue(self, msg: Message) -> Union[None, str]: msg_id = msg['id'] self._pending[msg_id] = asyncio.Queue(maxsize=1) -await self._outgoing.put(msg) +try: +await self._outgoing.put(msg) +except: +del self._pending[msg_id] +raise return msg_id @@ -452,9 +456,9 @@ async def _reply(self, msg_id: Union[str, None]) -> Message: was lost, or some other problem. """ queue = self._pending[msg_id] -result = await queue.get() try: +result = await queue.get() if isinstance(result, ExecInterruptedError): raise result return result -- 2.31.1
[PATCH 05/23] python/qemu-ga-client: update instructions to newer CLI syntax
Signed-off-by: John Snow --- python/qemu/qmp/qemu_ga_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/qemu/qmp/qemu_ga_client.py b/python/qemu/qmp/qemu_ga_client.py index 67ac0b4211..b3e1d98c9e 100644 --- a/python/qemu/qmp/qemu_ga_client.py +++ b/python/qemu/qmp/qemu_ga_client.py @@ -5,7 +5,7 @@ Start QEMU with: -# qemu [...] -chardev socket,path=/tmp/qga.sock,server,wait=off,id=qga0 \ +# qemu [...] -chardev socket,path=/tmp/qga.sock,server=on,wait=off,id=qga0 \ -device virtio-serial \ -device virtserialport,chardev=qga0,name=org.qemu.guest_agent.0 -- 2.31.1
[PATCH 04/23] python/aqmp: add SocketAddrT to package root
It's a commonly needed definition, it can be re-exported by the root. Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index 880d5b6fa7..c6fa2dda58 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -26,7 +26,12 @@ from .error import AQMPError from .events import EventListener from .message import Message -from .protocol import ConnectError, Runstate, StateError +from .protocol import ( +ConnectError, +Runstate, +SocketAddrT, +StateError, +) from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient @@ -48,4 +53,7 @@ 'ConnectError', 'ExecuteError', 'ExecInterruptedError', + +# Type aliases +'SocketAddrT', ) -- 2.31.1
[PATCH 01/23] python/aqmp: add __del__ method to legacy interface
asyncio can complain *very* loudly if you forget to back out of things gracefully before the garbage collector starts destroying objects that contain live references to asyncio Tasks. The usual fix is just to remember to call aqmp.disconnect(), but for the sake of the legacy wrapper and quick, one-off scripts where a graceful shutdown is not necessarily of paramount imporance, add a courtesy cleanup that will trigger prior to seeing screenfuls of confusing asyncio tracebacks. Note that we can't *always* save you from yourself; depending on when the GC runs, you might just seriously be out of luck. The best we can do in this case is to gently remind you to clean up after yourself. (Still much better than multiple pages of incomprehensible python warnings for the crime of forgetting to put your toys away.) Signed-off-by: John Snow --- python/qemu/aqmp/legacy.py | 18 ++ 1 file changed, 18 insertions(+) diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py index 9e7b9fb80b..2ccb136b02 100644 --- a/python/qemu/aqmp/legacy.py +++ b/python/qemu/aqmp/legacy.py @@ -16,6 +16,8 @@ import qemu.qmp from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT +from .error import AQMPError +from .protocol import Runstate from .qmp_client import QMPClient @@ -136,3 +138,19 @@ def settimeout(self, timeout: Optional[float]) -> None: def send_fd_scm(self, fd: int) -> None: self._aqmp.send_fd_scm(fd) + +def __del__(self) -> None: +if self._aqmp.runstate == Runstate.IDLE: +return + +if not self._aloop.is_running(): +self.close() +else: +# Garbage collection ran while the event loop was running. +# Nothing we can do about it now, but if we don't raise our +# own error, the user will be treated to a lot of traceback +# they might not understand. +raise AQMPError( +"QEMUMonitorProtocol.close()" +" was not called before object was garbage collected" +) -- 2.31.1
Re: [RFC PATCH v3] hw/nvme:Adding Support for namespace management
On Tue, Nov 23, 2021 at 11:11:37AM +0100, Lukasz Maniak wrote:
> On Wed, Nov 10, 2021 at 04:56:29PM +0530, Naveen wrote:
> > From: Naveen Nagar
> >
> > This patch supports namespace management : create and delete operations
> > This patch has been tested with the following command and size of image
> > file for unallocated namespaces is taken as 0GB. ns_create will look into
> > the list of unallocated namespaces and it will initialize the same and
> > return the nsid of the same. A new mandatory field has been added called
> > tnvmcap and we have ensured that the total capacity of namespace created
> > does not exceed tnvmcap
> >
> > -device nvme-subsys,id=subsys0,tnvmcap=8
> > -device nvme,serial=foo,id=nvme0,subsys=subsys0
> > -device nvme,serial=bar,id=nvme1,subsys=subsys0
> >
> > -drive id=ns1,file=ns1.img,if=none
> > -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,zoned=false,shared=true
> > -drive id=ns2,file=ns2.img,if=none
> > -device nvme-ns,drive=ns2,bus=nvme0,nsid=2,zoned=false,shared=true
> > -drive id=ns3,file=ns3.img,if=none
> > -device nvme-ns,drive=ns3,bus=nvme0,nsid=3,zoned=false,shared=true
> > -drive id=ns4,file=ns4.img,if=none
> > -device nvme-ns,drive=ns4,bus=nvme0,nsid=4,zoned=false,shared=true
> >
> > Please review and suggest if any changes are required.
> >
> > Signed-off-by: Naveen Nagar
> >
> > Since v2:
> > -Lukasz Maniak found a bug in namespace attachment and proposed
> > solution is added
> >
>
> Hi Naveen,
>
> The current implementation is inconsistent and thus has a bug related to
> unvmcap support.
>
> Namespaces are pre-allocated after boot, and the initial namespace size
> is the size of the associated blockdev. If the blockdevs are non-zero
> sized then the first deletion of the namespaces associated with them
> will increment unvmcap by their size. This will make unvmcap greater
> than tnvmcap.
>
> While the easiest way would be to prohibit the use of non-zero sized
> blockdev with namespace management, doing so would limit the
> functionality of the namespaces itself, which we would like to avoid.
>
> This fix below addresses issues related to unvmcap and non-zero block
> devices. The unvmcap value will be properly updated on both the first
> and subsequent controllers added to the subsystem regardless of the
> order in which nvme-ns is defined on the command line before or after
> the controller definition. Additionally, if the block device size of any
> namespace causes the unvmcap to be exceeded, an error will be returned
> at the namespace definition point.
>
> The fix is based on v3 based on v6.1.0, as v3 does not apply to master.
>
> ---
> hw/nvme/ctrl.c | 7 +++
> hw/nvme/ns.c | 23 ---
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 63ea2fcb14..dc0ad4155b 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6594,6 +6594,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
> *pci_dev)
> NvmeIdCtrl *id = >id_ctrl;
> uint8_t *pci_conf = pci_dev->config;
> uint64_t cap = ldq_le_p(>bar.cap);
> +int i;
>
> id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
> id->ssvid = cpu_to_le16(pci_get_word(pci_conf +
> PCI_SUBSYSTEM_VENDOR_ID));
> @@ -6672,6 +6673,12 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice
> *pci_dev)
> id->cmic |= NVME_CMIC_MULTI_CTRL;
> id->tnvmcap = n->subsys->params.tnvmcap * GiB;
> id->unvmcap = n->subsys->params.tnvmcap * GiB;
> +
> +for (i = 0; i < ARRAY_SIZE(n->subsys->namespaces); i++) {
> +if (n->subsys->namespaces[i]) {
> +id->unvmcap -= le64_to_cpu(n->subsys->namespaces[i]->size);
> +}
> +}
> }
>
> NVME_CAP_SET_MQES(cap, 0x7ff);
> diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> index f62a695132..c87d7f5bd6 100644
> --- a/hw/nvme/ns.c
> +++ b/hw/nvme/ns.c
> @@ -140,9 +140,12 @@ lbaf_found:
> return 0;
> }
>
> -static int nvme_ns_init_blk(NvmeNamespace *ns, Error **errp)
> +static int nvme_ns_init_blk(NvmeNamespace *ns, NvmeSubsystem *subsys,
> +Error **errp)
> {
> bool read_only;
> +NvmeCtrl *ctrl;
> +int i;
>
> if (!blkconf_blocksizes(>blkconf, errp)) {
> return -1;
> @@ -164,6 +167,21 @@ static int nvme_ns_init_blk(NvmeNamespace *ns, Error
> **errp)
> return -1;
> }
>
> +if (subsys) {
> +for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
> +ctrl = nvme_subsys_ctrl(subsys, i);
> +
> +if (ctrl) {
> +if (ctrl->id_ctrl.unvmcap < le64_to_cpu(ns->size)) {
> +error_setg(errp, "blockdev size %ld exceeds subsystem's "
> + "unallocated capacity", ns->size);
> +} else {
> +ctrl->id_ctrl.unvmcap -= le64_to_cpu(ns->size);
> +}
> +}
> +}
> +}
[PATCH v4 2/3] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
Guest might select another drive on the bus by setting the
DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR).
The current controller model doesn't expect a BlockBackend
to be NULL. A simple way to fix CVE-2021-20196 is to create
an empty BlockBackend when it is missing. All further
accesses will be safely handled, and the controller state
machines keep behaving correctly.
Cc: qemu-sta...@nongnu.org
Fixes: CVE-2021-20196
Reported-by: Gaoning Pan (Ant Security Light-Year Lab)
BugLink: https://bugs.launchpad.net/qemu/+bug/1912780
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338
Reviewed-by: Darren Kenny
Reviewed-by: Hanna Reitz
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/fdc.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 1dbf3f6028f..21d18ac2e36 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1166,7 +1166,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit)
static FDrive *get_cur_drv(FDCtrl *fdctrl)
{
-return get_drv(fdctrl, fdctrl->cur_drv);
+FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv);
+
+if (!cur_drv->blk) {
+/*
+ * Kludge: empty drive line selected. Create an anonymous
+ * BlockBackend to avoid NULL deref with various BlockBackend
+ * API calls within this model (CVE-2021-20196).
+ * Due to the controller QOM model limitations, we don't
+ * attach the created to the controller device.
+ */
+cur_drv->blk = blk_create_empty_drive();
+}
+return cur_drv;
}
/* Status A register : 0x00 (read-only) */
--
2.33.1
[PATCH v4 3/3] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
Without the previous commit, when running 'make check-qtest-i386'
with QEMU configured with '--enable-sanitizers' we get:
AddressSanitizer:DEADLYSIGNAL
=
==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x0344
==287878==The signal is caused by a WRITE memory access.
==287878==Hint: address points to the zero page.
#0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5
#1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5
#2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11
#3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17
#4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9
#5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9
Add the reproducer for CVE-2021-20196.
Suggested-by: Alexander Bulekov
Reviewed-by: Darren Kenny
Signed-off-by: Philippe Mathieu-Daudé
---
tests/qtest/fdc-test.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 26b69f7c5cd..8f6eee84a47 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -32,6 +32,9 @@
/* TODO actually test the results and get rid of this */
#define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__))
+#define DRIVE_FLOPPY_BLANK \
+"-drive
if=floppy,file=null-co://,file.read-zeroes=on,format=raw,size=1440k"
+
#define TEST_IMAGE_SIZE 1440 * 1024
#define FLOPPY_BASE 0x3f0
@@ -546,6 +549,40 @@ static void fuzz_registers(void)
}
}
+static bool qtest_check_clang_sanitizer(void)
+{
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+return true;
+#else
+g_test_skip("QEMU not configured using --enable-sanitizers");
+return false;
+#endif
+}
+static void test_cve_2021_20196(void)
+{
+QTestState *s;
+
+if (!qtest_check_clang_sanitizer()) {
+return;
+}
+
+s = qtest_initf("-nographic -m 32M -nodefaults " DRIVE_FLOPPY_BLANK);
+
+qtest_outw(s, 0x3f4, 0x0500);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outw(s, 0x3f1, 0x0400);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outw(s, 0x3f4, 0x);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_outb(s, 0x3f5, 0x01);
+qtest_outw(s, 0x3f1, 0x0500);
+qtest_outb(s, 0x3f5, 0x00);
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
int fd;
@@ -576,6 +613,7 @@ int main(int argc, char **argv)
qtest_add_func("/fdc/read_no_dma_18", test_read_no_dma_18);
qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
+qtest_add_func("/fdc/fuzz/cve_2021_20196", test_cve_2021_20196);
ret = g_test_run();
--
2.33.1
[PATCH v4 1/3] hw/block/fdc: Extract blk_create_empty_drive()
We are going to re-use this code in the next commit,
so extract it as a new blk_create_empty_drive() function.
Inspired-by: Hanna Reitz
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/fdc.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index fa933cd3263..1dbf3f6028f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -61,6 +61,12 @@
} while (0)
+/* Anonymous BlockBackend for empty drive */
+static BlockBackend *blk_create_empty_drive(void)
+{
+return blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
+}
+
//
/* qdev floppy bus */
@@ -486,8 +492,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error
**errp)
}
if (!dev->conf.blk) {
-/* Anonymous BlockBackend for an empty drive */
-dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
+dev->conf.blk = blk_create_empty_drive();
ret = blk_attach_dev(dev->conf.blk, qdev);
assert(ret == 0);
--
2.33.1
[PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196
Since v3: - Preliminary extract blk_create_empty_drive() - qtest checks qtest_check_clang_sanitizer() enabled - qtest uses null-co:// driver instead of file Philippe Mathieu-Daudé (3): hw/block/fdc: Extract blk_create_empty_drive() hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 hw/block/fdc.c | 23 --- tests/qtest/fdc-test.c | 38 ++ 2 files changed, 58 insertions(+), 3 deletions(-) -- 2.33.1
Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
On 11/24/21 15:00, Hanna Reitz wrote: > On 24.11.21 13:50, Philippe Mathieu-Daudé wrote: >> On 11/23/21 15:14, Hanna Reitz wrote: >>> On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: On 11/23/21 14:42, Hanna Reitz wrote: > On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >> From: Alexander Bulekov >> >> Without the previous commit, when running 'make check-qtest-i386' >> with QEMU configured with '--enable-sanitizers' we get: >> >> AddressSanitizer:DEADLYSIGNAL >> >> = >> ==287878==ERROR: AddressSanitizer: SEGV on unknown address >> 0x0344 >> ==287878==The signal is caused by a WRITE memory access. >> ==287878==Hint: address points to the zero page. >> #0 0x564b2e5bac27 in blk_inc_in_flight >> block/block-backend.c:1346:5 >> #1 0x564b2e5bb228 in blk_pwritev_part >> block/block-backend.c:1317:5 >> #2 0x564b2e5bcd57 in blk_pwrite >> block/block-backend.c:1498:11 >> #3 0x564b2ca1cdd3 in fdctrl_write_data >> hw/block/fdc.c:2221:17 >> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 >> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 >> >> Add the reproducer for CVE-2021-20196. >> >> Signed-off-by: Alexander Bulekov >> Message-Id: <20210319050906.14875-2-alx...@bu.edu> >> [PMD: Rebased, use global test_image] >> Reviewed-by: Darren Kenny >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> tests/qtest/fdc-test.c | 21 + >> 1 file changed, 21 insertions(+) > Not sure if I’m doing something wrong, but: > > Using the global test_image brings a problem, namely that this test > fails unconditionally (for me at least...?), with the reason being > that > the global QEMU instance (launched by qtest_start(), quit by > qtest_end()) still has that image open, so by launching a second > instance concurrently, I get this: > > qemu-system-x86_64: Failed to get "write" lock > Is another process using the image [/tmp/qtest.xV4IxX]? Hmm I had too many odd problems running qtests in parallel so I switched to 'make check-qtest -j1' more than 1 year ago, which is probably why I haven't noticed that issue. >>> I’ve run the test with >>> >>> QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test >>> >>> so there definitely weren’t any other tests running at the same time. I >>> don’t know why you don’t encounter this problem, but it’s caused by the >>> concurrent QEMU instance launched in the very same test (qtest_start() >>> in main(), and cleaned up by qtest_end() after g_test_run()). >> I run all my qtests on top of this patch, otherwise I can't >> get any coredump: >> https://lore.kernel.org/qemu-devel/20200707031920.17428-1-f4...@amsat.org/ >> >> But I don't think it mattered here... > > I can give that a try, but since I use coredumpctl, I generally don’t > have a problem with one coredump overwriting another (only that I need > to give a PID to `coredumpctl gdb` to load not the latest coredump (the > qtest) but the one before (qemu)). I'm not a Fedora expert and use the default, for some reason only the last coredump is available (which in this case is tests/qtest/fdc-test, not useful at all). > Hm, perhaps the problem is that I never applied the other series before > this one. Also one thing that remains to be tested... Don't waste time now, wait for v4 ;)
Re: [PATCH v2 12/15] hw/nvme: Initialize capability structures for primary/secondary controllers
On Wed, Nov 24, 2021 at 09:04:31AM +0100, Klaus Jensen wrote:
> On Nov 16 16:34, Łukasz Gieryk wrote:
> > With four new properties:
> > - sriov_v{i,q}_flexible,
> > - sriov_max_v{i,q}_per_vf,
> > one can configure the number of available flexible resources, as well as
> > the limits. The primary and secondary controller capability structures
> > are initialized accordingly.
> >
> > Since the number of available queues (interrupts) now varies between
> > VF/PF, BAR size calculation is also adjusted.
> >
> > Signed-off-by: Łukasz Gieryk
> > ---
> > hw/nvme/ctrl.c | 138 ---
> > hw/nvme/nvme.h | 4 ++
> > include/block/nvme.h | 5 ++
> > 3 files changed, 140 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index f8f5dfe204..f589ffde59 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -6358,13 +6444,40 @@ static void nvme_init_state(NvmeCtrl *n)
> > n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> > n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
> >
> > -list->numcntl = cpu_to_le16(n->params.sriov_max_vfs);
> > -for (i = 0; i < n->params.sriov_max_vfs; i++) {
> > +list->numcntl = cpu_to_le16(max_vfs);
> > +for (i = 0; i < max_vfs; i++) {
> > sctrl = >sec[i];
> > sctrl->pcid = cpu_to_le16(n->cntlid);
> > }
> >
> > cap->cntlid = cpu_to_le16(n->cntlid);
> > +cap->crt = NVME_CRT_VQ | NVME_CRT_VI;
> > +
> > +if (pci_is_vf(>parent_obj)) {
> > +cap->vqprt = cpu_to_le16(1 + n->conf_ioqpairs);
> > +} else {
> > +cap->vqprt = cpu_to_le16(1 + n->params.max_ioqpairs -
> > + n->params.sriov_vq_flexible);
> > +cap->vqfrt = cpu_to_le32(n->params.sriov_vq_flexible);
> > +cap->vqrfap = cap->vqfrt;
> > +cap->vqgran = cpu_to_le16(NVME_VF_RES_GRANULARITY);
> > +cap->vqfrsm = n->params.sriov_max_vq_per_vf ?
> > +cpu_to_le16(n->params.sriov_max_vq_per_vf) :
> > +cap->vqprt;
>
> That this defaults to VQPRT doesn't seem right. It should default to
> VQFRT. Does not make sense to report a maximum number of assignable
> flexible resources that are bigger than the number of flexible resources
> available.
I’ve explained in on of v1 threads why I think using the current default
is better than VQPRT.
What you’ve noticed is indeed an inconvenience, but it’s – at least in
my opinion – part of the design. What matters is the current number of
unassigned flexible resources. It may be lower than VQFRSM due to
multiple reasons:
1) resources are bound to PF,
2) resources are bound to other VFs,
3) resources simply don’t exist (not baked in silicone: VQFRT < VQFRSM).
If 1) and 2) are allowed to happen, and the user must be aware of that,
then why 3) shouldn’t?
Re: [PATCH v2 13/15] hw/nvme: Add support for the Virtualization Management command
On Wed, Nov 24, 2021 at 09:06:23AM +0100, Klaus Jensen wrote:
> On Nov 16 16:34, Łukasz Gieryk wrote:
> > With the new Virtualization Management command one can:
> > - assign flexible resources (queues, interrupts) to primary and
> >secondary controllers,
> > - toggle the online/offline state of given controller.
> >
> > Signed-off-by: Łukasz Gieryk
> > ---
> > hw/nvme/ctrl.c | 204 +++
> > hw/nvme/nvme.h | 16
> > hw/nvme/trace-events | 3 +
> > include/block/nvme.h | 17
> > 4 files changed, 240 insertions(+)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index f589ffde59..9d0432a2e5 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
>
> [... snip]
>
> > +static uint16_t nvme_assign_virt_res_to_sec(NvmeCtrl *n, NvmeRequest *req,
> > +uint16_t cntlid, uint8_t rt,
> > int nr)
> > +{
> > +int limit = rt ? n->params.sriov_max_vi_per_vf :
> > + n->params.sriov_max_vq_per_vf;
>
> If these parameters are left at the default, limit is 0 and the check
> below fails.
>
> [... snip]
>
> > +if (nr > limit) {
> > +return NVME_INVALID_NUM_RESOURCES | NVME_DNR;
> > +}
Indeed, my bad.
Al the tests I have at hand set the parameters explicitly, so the
problem has slipped through. I’ve manually tested only the negative
scenarios, where Qemu refuses to start.
[RFC for-6.2] block/nbd: forbid incompatible change of server options on reconnect
Reconnect feature was never prepared to handle server options changed
on reconnect. Let's be stricter and check what exactly is changed. If
server capabilities just got richer don't worry. Otherwise fail and
drop the established connection.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
Hi all! The patch is probably good for 6.2. It's an RFC because I didn't
test it yet) But I want to early send, so that my proposed design be
available for discussion.
include/block/nbd.h | 9 +
nbd/client-connection.c | 86 +
2 files changed, 95 insertions(+)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 78d101b774..3d379b5539 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -157,6 +157,10 @@ enum {
#define NBD_FLAG_SEND_RESIZE (1 << NBD_FLAG_SEND_RESIZE_BIT)
#define NBD_FLAG_SEND_CACHE(1 << NBD_FLAG_SEND_CACHE_BIT)
#define NBD_FLAG_SEND_FAST_ZERO(1 << NBD_FLAG_SEND_FAST_ZERO_BIT)
+/*
+ * If you add any new NBD_FLAG_ flag, check that logic in
+ * nbd_is_new_info_compatible() is still good about handling flags.
+ */
/* New-style handshake (global) flags, sent from server to client, and
control what will happen during handshake phase. */
@@ -305,6 +309,11 @@ struct NBDExportInfo {
uint32_t context_id;
+/*
+ * WARNING! when add any new field to the structure, don't forget to check
+ * and updated nbd_is_new_info_compatible() function.
+ */
+
/* Set by server results during nbd_receive_export_list() */
char *description;
int n_contexts;
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 695f855754..2d66993632 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -37,6 +37,10 @@ struct NBDClientConnection {
bool do_negotiation;
bool do_retry;
+/* Used only by connection thread, no need in locking the mutex */
+bool has_prev_info;
+NBDExportInfo prev_info;
+
QemuMutex mutex;
/*
@@ -160,6 +164,67 @@ static int nbd_connect(QIOChannelSocket *sioc,
SocketAddress *addr,
return 0;
}
+static bool nbd_is_new_info_compatible(NBDExportInfo *old, NBDExportInfo *new,
+ Error **errp)
+{
+uint32_t dropped_flags;
+
+if (old->structured_reply && !new->structured_reply) {
+error_setg(errp, "Server options degrade after reconnect: "
+ "structured_reply is not supported anymore");
+return false;
+}
+
+if (old->base_allocation && !new->base_allocation) {
+error_setg(errp, "Server options degrade after reconnect: "
+ "base_allocation is not supported anymore");
+return false;
+}
+
+if (old->size != new->size) {
+error_setg(errp, "NBD export size changed after reconnect");
+return false;
+}
+
+/*
+ * No worry if rotational status changed. But other flags are feature
flags,
+ * they should not degrade.
+ */
+dropped_flags = (old->flags & ~new->flags) & ~NBD_FLAG_ROTATIONAL;
+if (dropped_flags) {
+error_setg(errp, "Server options degrade after reconnect: flags 0x%"
+ PRIx32 " are not reported anymore", dropped_flags);
+return false;
+}
+
+if (new->min_block > old->min_block) {
+error_setg(errp, "Server requires more strict min_block after "
+ "reconnect: %" PRIu32 " instead of %" PRIu32,
+ new->min_block, old->min_block);
+return false;
+}
+if (new->min_block && (old->min_block % new->min_block)) {
+error_setg(errp, "Server requires new min_block %" PRIu32
+ " after reconnect, incompatible with old one %" PRIu32,
+ new->min_block, old->min_block);
+return false;
+}
+
+if (new->max_block < old->max_block) {
+error_setg(errp, "Server requires more strict max_block after "
+ "reconnect: %" PRIu32 " instead of %" PRIu32,
+ new->max_block, old->max_block);
+return false;
+}
+
+if (old->context_id != new->context_id) {
+error_setg(errp, "Meta context id changed after reconnect");
+return false;
+}
+
+return true;
+}
+
static void *connect_thread_func(void *opaque)
{
NBDClientConnection *conn = opaque;
@@ -183,6 +248,27 @@ static void *connect_thread_func(void *opaque)
conn->do_negotiation ? >updated_info : NULL,
conn->tlscreds, >ioc, >err);
+if (ret == 0) {
+if (conn->has_prev_info &&
+!nbd_is_new_info_compatible(>prev_info,
+>updated_info, >err))
+{
+NBDRequest request = { .type = NBD_CMD_DISC };
+QIOChannel *ioc = conn->ioc ?: QIO_CHANNEL(conn->sioc);
+
+nbd_send_request(ioc, );
+
Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
On 24.11.21 13:50, Philippe Mathieu-Daudé wrote: On 11/23/21 15:14, Hanna Reitz wrote: On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: On 11/23/21 14:42, Hanna Reitz wrote: On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: From: Alexander Bulekov Without the previous commit, when running 'make check-qtest-i386' with QEMU configured with '--enable-sanitizers' we get: AddressSanitizer:DEADLYSIGNAL = ==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x0344 ==287878==The signal is caused by a WRITE memory access. ==287878==Hint: address points to the zero page. #0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5 #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 Add the reproducer for CVE-2021-20196. Signed-off-by: Alexander Bulekov Message-Id: <20210319050906.14875-2-alx...@bu.edu> [PMD: Rebased, use global test_image] Reviewed-by: Darren Kenny Signed-off-by: Philippe Mathieu-Daudé --- tests/qtest/fdc-test.c | 21 + 1 file changed, 21 insertions(+) Not sure if I’m doing something wrong, but: Using the global test_image brings a problem, namely that this test fails unconditionally (for me at least...?), with the reason being that the global QEMU instance (launched by qtest_start(), quit by qtest_end()) still has that image open, so by launching a second instance concurrently, I get this: qemu-system-x86_64: Failed to get "write" lock Is another process using the image [/tmp/qtest.xV4IxX]? Hmm I had too many odd problems running qtests in parallel so I switched to 'make check-qtest -j1' more than 1 year ago, which is probably why I haven't noticed that issue. I’ve run the test with QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test so there definitely weren’t any other tests running at the same time. I don’t know why you don’t encounter this problem, but it’s caused by the concurrent QEMU instance launched in the very same test (qtest_start() in main(), and cleaned up by qtest_end() after g_test_run()). I run all my qtests on top of this patch, otherwise I can't get any coredump: https://lore.kernel.org/qemu-devel/20200707031920.17428-1-f4...@amsat.org/ But I don't think it mattered here... I can give that a try, but since I use coredumpctl, I generally don’t have a problem with one coredump overwriting another (only that I need to give a PID to `coredumpctl gdb` to load not the latest coredump (the qtest) but the one before (qemu)). Hm, perhaps the problem is that I never applied the other series before this one. Also one thing that remains to be tested... Hanna
Questions about losing the write lock of raw-format disks after migration
Hi folks,
When we test migration with raw-format disk, we found that the QEMU
process in the dst will lose the write lock after migration. However,
the QEMU process in the dst will still hold the write lock for
qcow2-format disk.
After reading some block layer's code, I found that the first
blk_set_perm in blk_root_activate will set blk->shared_perm to
BLK_PERM_ALL (disable all shared permissions?). Then in
blk_vm_state_changed, blk_set_perm will set shared_perm to
blk->shared_perm, which is BLK_PERM_ALL. And it makes
raw_handle_perm_lock not to get the write lock.
So I try the following patch and it will fix the problem:
diff --git a/block/block-backend.c b/block/block-backend.c
index 12ef80ea17..96518fd1f0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -197,13 +197,6 @@ static void blk_root_activate(BdrvChild *child,
Error **errp)
blk->disable_perm = false;
-blk_set_perm(blk, blk->perm, BLK_PERM_ALL, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-blk->disable_perm = true;
-return;
-}
-
if (runstate_check(RUN_STATE_INMIGRATE)) {
/* Activation can happen when migration process is still
active, for
* example when nbd_server_add is called during non-shared storage
I'm new to the block layer and I'm not sure that it's a right fix to the
problem. Any idea about the problem and the patch?
Thanks,
Peng
Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
On 11/23/21 15:14, Hanna Reitz wrote: > On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: >> On 11/23/21 14:42, Hanna Reitz wrote: >>> On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: From: Alexander Bulekov Without the previous commit, when running 'make check-qtest-i386' with QEMU configured with '--enable-sanitizers' we get: AddressSanitizer:DEADLYSIGNAL = ==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x0344 ==287878==The signal is caused by a WRITE memory access. ==287878==Hint: address points to the zero page. #0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5 #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 Add the reproducer for CVE-2021-20196. Signed-off-by: Alexander Bulekov Message-Id: <20210319050906.14875-2-alx...@bu.edu> [PMD: Rebased, use global test_image] Reviewed-by: Darren Kenny Signed-off-by: Philippe Mathieu-Daudé --- tests/qtest/fdc-test.c | 21 + 1 file changed, 21 insertions(+) >>> Not sure if I’m doing something wrong, but: >>> >>> Using the global test_image brings a problem, namely that this test >>> fails unconditionally (for me at least...?), with the reason being that >>> the global QEMU instance (launched by qtest_start(), quit by >>> qtest_end()) still has that image open, so by launching a second >>> instance concurrently, I get this: >>> >>> qemu-system-x86_64: Failed to get "write" lock >>> Is another process using the image [/tmp/qtest.xV4IxX]? >> Hmm I had too many odd problems running qtests in parallel so I >> switched to 'make check-qtest -j1' more than 1 year ago, which >> is probably why I haven't noticed that issue. > > I’ve run the test with > > QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test > > so there definitely weren’t any other tests running at the same time. I > don’t know why you don’t encounter this problem, but it’s caused by the > concurrent QEMU instance launched in the very same test (qtest_start() > in main(), and cleaned up by qtest_end() after g_test_run()). I run all my qtests on top of this patch, otherwise I can't get any coredump: https://lore.kernel.org/qemu-devel/20200707031920.17428-1-f4...@amsat.org/ But I don't think it mattered here...
Re: [PATCH v2 13/15] hw/nvme: Add support for the Virtualization Management command
On Nov 16 16:34, Łukasz Gieryk wrote:
> With the new Virtualization Management command one can:
> - assign flexible resources (queues, interrupts) to primary and
>secondary controllers,
> - toggle the online/offline state of given controller.
>
> Signed-off-by: Łukasz Gieryk
> ---
> hw/nvme/ctrl.c | 204 +++
> hw/nvme/nvme.h | 16
> hw/nvme/trace-events | 3 +
> include/block/nvme.h | 17
> 4 files changed, 240 insertions(+)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f589ffde59..9d0432a2e5 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
[... snip]
> +static uint16_t nvme_assign_virt_res_to_sec(NvmeCtrl *n, NvmeRequest *req,
> +uint16_t cntlid, uint8_t rt, int
> nr)
> +{
> +int limit = rt ? n->params.sriov_max_vi_per_vf :
> + n->params.sriov_max_vq_per_vf;
If these parameters are left at the default, limit is 0 and the check
below fails.
[... snip]
> +if (nr > limit) {
> +return NVME_INVALID_NUM_RESOURCES | NVME_DNR;
> +}
signature.asc
Description: PGP signature
Re: [PATCH v2 12/15] hw/nvme: Initialize capability structures for primary/secondary controllers
On Nov 16 16:34, Łukasz Gieryk wrote:
> With four new properties:
> - sriov_v{i,q}_flexible,
> - sriov_max_v{i,q}_per_vf,
> one can configure the number of available flexible resources, as well as
> the limits. The primary and secondary controller capability structures
> are initialized accordingly.
>
> Since the number of available queues (interrupts) now varies between
> VF/PF, BAR size calculation is also adjusted.
>
> Signed-off-by: Łukasz Gieryk
> ---
> hw/nvme/ctrl.c | 138 ---
> hw/nvme/nvme.h | 4 ++
> include/block/nvme.h | 5 ++
> 3 files changed, 140 insertions(+), 7 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f8f5dfe204..f589ffde59 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6358,13 +6444,40 @@ static void nvme_init_state(NvmeCtrl *n)
> n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
>
> -list->numcntl = cpu_to_le16(n->params.sriov_max_vfs);
> -for (i = 0; i < n->params.sriov_max_vfs; i++) {
> +list->numcntl = cpu_to_le16(max_vfs);
> +for (i = 0; i < max_vfs; i++) {
> sctrl = >sec[i];
> sctrl->pcid = cpu_to_le16(n->cntlid);
> }
>
> cap->cntlid = cpu_to_le16(n->cntlid);
> +cap->crt = NVME_CRT_VQ | NVME_CRT_VI;
> +
> +if (pci_is_vf(>parent_obj)) {
> +cap->vqprt = cpu_to_le16(1 + n->conf_ioqpairs);
> +} else {
> +cap->vqprt = cpu_to_le16(1 + n->params.max_ioqpairs -
> + n->params.sriov_vq_flexible);
> +cap->vqfrt = cpu_to_le32(n->params.sriov_vq_flexible);
> +cap->vqrfap = cap->vqfrt;
> +cap->vqgran = cpu_to_le16(NVME_VF_RES_GRANULARITY);
> +cap->vqfrsm = n->params.sriov_max_vq_per_vf ?
> +cpu_to_le16(n->params.sriov_max_vq_per_vf) :
> +cap->vqprt;
That this defaults to VQPRT doesn't seem right. It should default to
VQFRT. Does not make sense to report a maximum number of assignable
flexible resources that are bigger than the number of flexible resources
available.
signature.asc
Description: PGP signature
Re: [PATCH 4/4] include/sysemu/blockdev.h: remove drive_get_max_devs
On 11/24/21 07:36, Emanuele Giuseppe Esposito wrote:
> Remove drive_get_max_devs, as it is not used by anyone.
Maybe complete:
Last use was removed in commit 8f2d75e81d5
("hw: Drop superfluous special checks for orphaned -drive").
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> include/sysemu/blockdev.h | 1 -
> blockdev.c| 17 -
> 2 files changed, 18 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 3/4] include/sysemu/blockdev.h: move drive_add and inline drive_def
On 11/24/21 07:36, Emanuele Giuseppe Esposito wrote:
> drive_add is only used in softmmu/vl.c, so it can be a static
> function there, and drive_def is only a particular use case of
> qemu_opts_parse_noisily, so it can be inlined.
>
> Also remove drive_mark_claimed_by_board, as it is only defined
> but not implemented (nor used) anywhere.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> include/sysemu/blockdev.h | 6 ++
> block/monitor/block-hmp-cmds.c | 2 +-
> blockdev.c | 27 +--
> softmmu/vl.c | 25 -
> 4 files changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 32c2d6023c..aacc587a33 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -27,6 +27,8 @@ typedef enum {
> IF_COUNT
> } BlockInterfaceType;
>
> +extern const char *const block_if_name[];
Maybe a cleaner alternative is to ignore the previous patch,
and add a new public method:
const char *block_if_name(BlockInterfaceType iface);
Re: [PATCH v2 00/15] hw/nvme: SR-IOV with Virtualization Enhancements
Hi Lukasz, I've been through this. I have a couple of review comments, but overall looks good for inclusion in nvme-next. Would be nice to get this in early in the cycle so it can mature there for 7.0. I'd like that we mark this support experimental, so we can easily do some changes to how parameters work since I'm not sure we completely agree on that yet. By the way, in the future, please add me and Keith as CCs on the entire series so we get CC'ed on replies to the cover-letter ;) On Nov 16 16:34, Łukasz Gieryk wrote: > This is the updated version of SR-IOV support for the NVMe device. > > Changes since v1: > - Dropped the "pcie: Set default and supported MaxReadReq to 512" patch. >The original author agrees it may be no longer needed for recent >kernels. > - Dropped the "pcie: Add callback preceding SR-IOV VFs update" patch. >A customized pc->config_write callback is used instead. > - Split the "hw/nvme: Calculate BAR attributes in a function” patch into >cleanup and update parts. > - Reworked the configuration options related to SR-IOV. > - Fixed nvme_update_msixcap_ts() for platform without MSIX support. > - Updated handling of SUBSYS_SLOT_RSVD values in nvme_subsys_ctrl(). > - Updated error codes returned from the Virtualization Management >command (DNR is set). > - Updated typedef/enum names mismatch. > - Few other minor tweaks and improvements. > > List of known gaps and nice-to-haves: > > 1) Interaction of secondary controllers with namespaces is not 100% > following the spec > > The limitation: VF has to be visible on the PCI bus first, and only then > such VF can have a namespace attached. > Looking at the spec I'm not even sure what the expected behavior is supposed to be, can you elaborate? I rebased this on latest, and with Hannes changes, shared namespaces will be attached by default, which seems to be reasonable. > The problem is that the mapping between controller and attached > namespaces is stored in the NvmeCtrl object, and unrealized VF doesn’t > have this object allocated. There are multiple ways to address the > issue, but none of those we’ve considered so far is elegant. The fact, > that the namespace-related code area is busy (pending patches, some > rework?), doesn’t help either. > > > 2) VFs report and support the same features as the parent PF > > Due to security reasons, user of a VF should be not allowed to interact > with other VFs. Essentially, VFs should not announce and support: > Namespace Management, Attachment, corresponding Identify commands, and > maybe other features as well. > This can be relatively easily fixed I think. I have a patch that already does this for Administrative controller types and it should be applicable here as well. I can follow up with that. > > 3) PMR and CMB must be disabled when SR-IOV is active > > The main concern is whether PMR/CMB should be even implemented for a > device that supports SR-IOV. If the answer is yes, then another question > arises: how the feature should behave? Simulation of different devices > may require different implementation. > > It's too early to get into such details, so we’ve decided to disallow > both features altogether if SR-IOV is enabled. > Reasonable enough for now. > > 4) The Private Resources Mode > > The NVMe Spec defines Flexible Resources as an optional feature. VFs can > alternatively support a fixed number of queues/interrupts. > > This SR-IOV implementation supports Flexible Resources with the > Virtualization Management command, and a fallback to Private Resources > is not available. Support for such fallback, if there’s demand, can be > implemented later. > Acceptable. > > 5) Support for Namespace Management command > > Device with virtualization enhancements must support the Namespace > Management command. The command is not absolutely necessary to use > SR-IOV, but for a more complete set of features you may want to > cherry-pick this patch: > https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg03107.html > together with this fix: > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg06734.html > I think we can live with just Namespace Attachment suppoort for now, until the above patches are accepted. signature.asc Description: PGP signature
