Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification
> Hi @jrreinh...@google.com,
> Thank you very much for your helpful comments.
>
> I missed the delay and cs_change_delay parameters. I will add both
> of them, although cs_change_delay can not be set from userspace, but can
> be set in kernel space.
>
>
> For CS delays such as PRE_DELAY/POST_DELAY, it seems that they are
> defined in structure spi_device:
>
> @cs_setup: delay to be introduced by the controller after CS is
> asserted.
>
> @cs_hold: delay to be introduced by the controller before CS is
> deasserted.
>
> @cs_inactive: delay to be introduced by the controller after CS is
> deasserted. If @cs_change_delay is used from @spi_transfer, then the
> two delays will be added up.
>
> They show the SPI controller ability to control the CS
> assertion/deassertion timing and should not be changed for each transfer
> (because thay can be updated by setting structure spi_transfer or
> structure spi_ioc_transfer). I think it better to define these parameter
> in host OS rather than in guest OS since it's the host OS to operate the
> hardware SPI controller directly. Besides, it can also avoid passing the
> same values from guest to host time after time.
>
> What's your opinion on this topic? Again, thank you very much.
Hi Haixu,
I took another look at the Linux structures and attempted to map up the
delay parameters to the various points in a SPI sequence. Please correct
me if I'm wrong about any of this.
Consider this diagram where CS# is an active-low chip select, and SCLK
is the serial clock:
___. ._.
CS#|___| |___
. . .
. . .
SCLK _________________
. .. . . . . . .
Delays: +-A-++B+ +--C--+ +-D-+--E--+
. .. . . . . . .
0 12 3 4 5 6 7 8
. .. . . . .
Terms: . +Word+ . . . .
. . . . . .
. +---Frame--+ +---Frame--+ .
. .
+-Transfer--+
Using NXP terminology:
From 1 to 2 is a "word" (with e.g. 8 bits).
From 1 to 4 is a "frame"(with 3 words).
From 1 to 6 is a "transfer" (with 3 frames).
Linux does not have the concept of a frame, only a series of transfers
(a spi_message), each of which can specify whether CS changes or not.
I've identified these various timing points and attempted to match them
to the Linux spi_device and spi_transfer parameters:
A - CS Setup: Delay after CS asserted before clock starts.
spi_device.cs_setup
B - Word Delay: Delay between words.
spi_transfer.word_delay
C - Frame Delay: Delay between frames.
(Linux does not have the concept of a SPI "frame".)
D - CS Hold: Delay after clock stops, and before CS deasserted.
spi_device.cs_hold (+ spi_transfer.delay ??)
E - CS Inactive: Delay after CS deasserted, before asserting again.
spi_device.cs_inactive + spi_transfer.cs_change_delay
While I agree with you that some of these timings are unlikely to change
from transfer-to-transfer, the subset of which should be specified
at the device level vs. per-transfer seems somewhat arbitrary. As you
can see there is some overlap between them.
If I understand correctly, it appears that the CS hold time *can* be
controlled on a per-transfer bases, using spi_transfer.delay
("after this transfer before changing the chipselect status").
That leaves CS setup as the only parameter that cannot be influenced on
a per-transfer basis.
Theoretically, one might require different CS setup/hold times,
depending on which slave_id they are talking to (on the same bus).
In that case, one must set those spi_device parameters to the worst-case
(maximum) values. However, this is already a Linux limitation, so I'm
not sure it needs to be improved here.
I think you've achieved parity with what the Linux kernel API allows,
and that's probably good enough. As you said, anything else can be
adjusted in the host OS -- I'm not sure how the guest would go about
achieving that, though. You're not proposing the use of configuration
space for virtio-spi, are you?
Regards,
Jonathon Reinhart
>
> Best Regards
> Haixu Cui
>
> On 6/28/2023 1:06 AM, jrreinh...@google.com wrote:
> > Hi Haixu,
> >
> >> +The \field{word_delay} defines how long to wait between words within
> >> one SPI transfer,
> >> +in ns unit.
> >
> > I'm a little surprised to see a word_delay but no frame_delay or
> > transfer_delay.
[virtio-dev] [RFC PATCH 6/7] virtio_rtc: Add PTP clocks
Expose the virtio_rtc clocks as PTP clocks to userspace, similar to
ptp_kvm. virtio_rtc can expose multiple clocks, e.g. a UTC clock and a
monotonic clock. Userspace should distinguish different clocks through the
name assigned by the driver. A udev rule such as the following can be used
to get a symlink /dev/ptp_virtio to the UTC clock:
SUBSYSTEM=="ptp", ATTR{clock_name}=="Virtio PTP UTC", SYMLINK +=
"ptp_virtio"
The preferred PTP clock reading method is ioctl PTP_SYS_OFFSET_PRECISE2,
through the ptp_clock_info.getcrosststamp() op. For now,
PTP_SYS_OFFSET_PRECISE2 will return -EOPNOTSUPP through a weak function.
PTP_SYS_OFFSET_PRECISE2 requires cross-timestamping support for specific
clocksources, which will be added in the following. If the clocksource
specific code is enabled, check that the Virtio RTC device supports the
respective HW counter before obtaining an actual cross-timestamp from the
Virtio device.
The Virtio RTC device response time may be higher than the timekeeper
seqcount increment interval. Therefore, obtain the cross-timestamp before
calling get_device_system_crosststamp().
As a fallback, support the ioctl PTP_SYS_OFFSET_EXTENDED2 for all
platforms.
Assume that concurrency issues during PTP clock removal are avoided by the
posix_clock framework.
Kconfig recursive dependencies prevent virtio_rtc from implicitly enabling
PTP_1588_CLOCK, therefore just warn the user if PTP_1588_CLOCK is not
available. Since virtio_rtc should in the future also expose clocks as RTC
class devices, it should make sense to not have VIRTIO_RTC depend on
PTP_1588_CLOCK.
Signed-off-by: Peter Hilber
---
drivers/virtio/Kconfig | 16 ++
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_rtc_driver.c | 111 +++-
drivers/virtio/virtio_rtc_internal.h | 62 +
drivers/virtio/virtio_rtc_ptp.c | 384 +++
5 files changed, 571 insertions(+), 3 deletions(-)
create mode 100644 drivers/virtio/virtio_rtc_ptp.c
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index e3dbf16fa977..7369ecd7dd01 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -187,4 +187,20 @@ config VIRTIO_RTC
If unsure, say M.
+comment "WARNING: The Virtio RTC driver is useless without VIRTIO_RTC_PTP."
+ depends on VIRTIO_RTC && !VIRTIO_RTC_PTP
+
+comment "Enable PTP_1588_CLOCK in order to enable VIRTIO_RTC_PTP."
+ depends on VIRTIO_RTC && PTP_1588_CLOCK=n
+
+config VIRTIO_RTC_PTP
+ bool "Virtio RTC PTP clocks"
+ default y
+ depends on VIRTIO_RTC && PTP_1588_CLOCK
+ help
+This exposes any Virtio RTC clocks as PTP Hardware Clocks (PHCs) to
+userspace.
+
+If unsure, say Y.
+
endif # VIRTIO_MENU
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index f760414ed6ab..4d48cbcae6bb 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_VIRTIO_MEM) += virtio_mem.o
obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
obj-$(CONFIG_VIRTIO_RTC) += virtio_rtc.o
virtio_rtc-y := virtio_rtc_driver.o
+virtio_rtc-$(CONFIG_VIRTIO_RTC_PTP) += virtio_rtc_ptp.o
diff --git a/drivers/virtio/virtio_rtc_driver.c
b/drivers/virtio/virtio_rtc_driver.c
index 424500d2c4f7..3c11fa95b9a7 100644
--- a/drivers/virtio/virtio_rtc_driver.c
+++ b/drivers/virtio/virtio_rtc_driver.c
@@ -36,11 +36,16 @@ struct viortc_vq {
* struct viortc_dev - virtio_rtc device data
* @vdev: virtio device
* @vqs: virtqueues
+ * @clocks_to_unregister: Clock references, which are only used during device
+ *removal.
+ * For other uses, there would be a race between device
+ * creation and setting the pointers here.
* @num_clocks: # of virtio_rtc clocks
*/
struct viortc_dev {
struct virtio_device *vdev;
struct viortc_vq vqs[VIORTC_MAX_NR_QUEUES];
+ struct viortc_ptp_clock **clocks_to_unregister;
u16 num_clocks;
};
@@ -588,6 +593,89 @@ int viortc_cross_cap(struct viortc_dev *viortc, u64
vio_clk_id, u16 hw_counter,
* init, deinit
*/
+/**
+ * viortc_init_clock() - init local representation of virtio_rtc clock (PHC)
+ * @viortc: device data
+ * @vio_clk_id: virtio_rtc clock id
+ *
+ * Context: Process context.
+ * Return: Zero on success, negative error code otherwise.
+ */
+static int viortc_init_clock(struct viortc_dev *viortc, u64 vio_clk_id)
+{
+ int ret;
+ u16 clock_type;
+ char ptp_clock_name[PTP_CLOCK_NAME_LEN];
+ const char *type_name;
+ /* fit prefix + u16 in decimal */
+ char type_name_buf[5 + 5 + 1];
+ bool has_xtstamp_feature;
+ struct viortc_ptp_clock *vio_ptp;
+ struct virtio_device *vdev = viortc->vdev;
+
+ ret = viortc_clock_cap(viortc, vio_clk_id, &clock_type);
+ if (ret)
+ return ret;
+
+ switch (clock_type) {
+ case VIRTIO_RTC_CLOCK_UTC:
+
[virtio-dev] [RFC PATCH 5/7] virtio_rtc: Add module and driver core
Add the virtio_rtc module and driver core. The virtio_rtc module implements
a driver compatible with the proposed Virtio RTC device specification [1].
The Virtio RTC (Real Time Clock) device provides information about current
time. The device can provide different clocks, e.g. for the UTC or TAI time
standards, or for physical time elapsed since some past epoch. The driver
can read the clocks with simple or more accurate methods.
Implement the core, which interacts with the Virtio RTC device. Apart from
this, the core does not expose functionality outside of the virtio_rtc
module. A follow-up patch will expose PTP clocks.
Provide synchronous messaging, which is enough for the expected time
synchronization use cases through PTP clocks (similar to ptp_kvm) or RTC
Class driver.
[1] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00592.html
Signed-off-by: Peter Hilber
---
MAINTAINERS | 7 +
drivers/virtio/Kconfig | 14 +
drivers/virtio/Makefile | 2 +
drivers/virtio/virtio_rtc_driver.c | 736 +++
drivers/virtio/virtio_rtc_internal.h | 23 +
include/uapi/linux/virtio_rtc.h | 159 ++
6 files changed, 941 insertions(+)
create mode 100644 drivers/virtio/virtio_rtc_driver.c
create mode 100644 drivers/virtio/virtio_rtc_internal.h
create mode 100644 include/uapi/linux/virtio_rtc.h
diff --git a/MAINTAINERS b/MAINTAINERS
index cd5388a33410..4dcdb98146be 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22661,6 +22661,13 @@ S: Maintained
F: drivers/nvdimm/nd_virtio.c
F: drivers/nvdimm/virtio_pmem.c
+VIRTIO RTC DRIVER
+M: Peter Hilber
+L: virtualizat...@lists.linux-foundation.org
+S: Maintained
+F: drivers/virtio/virtio_rtc_*
+F: include/uapi/linux/virtio_rtc.h
+
VIRTIO SOUND DRIVER
M: Anton Yakovlev
M: "Michael S. Tsirkin"
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 0a53a61231c2..e3dbf16fa977 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -173,4 +173,18 @@ config VIRTIO_DMA_SHARED_BUFFER
This option adds a flavor of dma buffers that are backed by
virtio resources.
+config VIRTIO_RTC
+ tristate "Virtio RTC driver"
+ depends on VIRTIO
+ depends on PTP_1588_CLOCK_OPTIONAL
+ help
+This driver provides current time from a Virtio RTC device. The driver
+provides the time through one or more clocks. The driver sub-option
+VIRTIO_RTC_PTP must be enabled to expose the clocks to userspace.
+
+To compile this code as a module, choose M here: the module will be
+called virtio_rtc.
+
+If unsure, say M.
+
endif # VIRTIO_MENU
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 8e98d24917cc..f760414ed6ab 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
obj-$(CONFIG_VIRTIO_VDPA) += virtio_vdpa.o
obj-$(CONFIG_VIRTIO_MEM) += virtio_mem.o
obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
+obj-$(CONFIG_VIRTIO_RTC) += virtio_rtc.o
+virtio_rtc-y := virtio_rtc_driver.o
diff --git a/drivers/virtio/virtio_rtc_driver.c
b/drivers/virtio/virtio_rtc_driver.c
new file mode 100644
index ..424500d2c4f7
--- /dev/null
+++ b/drivers/virtio/virtio_rtc_driver.c
@@ -0,0 +1,736 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * virtio_rtc driver core
+ *
+ * Copyright (C) 2022-2023 OpenSynergy GmbH
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "virtio_rtc_internal.h"
+
+/* virtqueue order */
+enum {
+ VIORTC_READQ,
+ VIORTC_CONTROLQ,
+ VIORTC_MAX_NR_QUEUES,
+};
+
+/**
+ * struct viortc_vq - virtqueue abstraction
+ * @vq: virtqueue
+ * @lock: protects access to vq
+ */
+struct viortc_vq {
+ struct virtqueue *vq;
+ spinlock_t lock;
+};
+
+/**
+ * struct viortc_dev - virtio_rtc device data
+ * @vdev: virtio device
+ * @vqs: virtqueues
+ * @num_clocks: # of virtio_rtc clocks
+ */
+struct viortc_dev {
+ struct virtio_device *vdev;
+ struct viortc_vq vqs[VIORTC_MAX_NR_QUEUES];
+ u16 num_clocks;
+};
+
+/**
+ * struct viortc_msg - Message requested by driver, responded by device.
+ * @viortc: device data
+ * @req: request buffer
+ * @resp: response buffer
+ * @responded: vqueue callback signals response reception
+ * @refcnt: Message reference count, message and buffers will be deallocated
+ * once 0. refcnt is decremented in the vqueue callback and in the
+ * thread waiting on the responded completion.
+ * If a message response wait function times out, the message will be
+ * freed upon late reception (refcnt will reach 0 in the callback), or
+ * device removal.
+ * @req_size: size of request in bytes
+ * @resp_cap: maximum size of response in bytes
+ * @resp_actual_size: actual size of response
+ */
+struct viort
[virtio-dev] [RFC PATCH 7/7] virtio_rtc: Add Arm Generic Timer cross-timestamping
Add PTP_SYS_OFFSET_PRECISE2 support on platforms using the Arm Generic
Timer, by forwarding the clocksource information from arm_arch_timer.
Support only the CP15 counter interfaces, since the memory-mapped
interfaces are not supported by the Virtio RTC draft spec [1].
[1] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00592.html
Signed-off-by: Peter Hilber
---
drivers/virtio/Kconfig | 13 ++
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_rtc_arm.c | 44 +
3 files changed, 58 insertions(+)
create mode 100644 drivers/virtio/virtio_rtc_arm.c
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 7369ecd7dd01..ed3f541032a0 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -203,4 +203,17 @@ config VIRTIO_RTC_PTP
If unsure, say Y.
+config VIRTIO_RTC_ARM
+ bool "Virtio RTC cross-timestamping using Arm Generic Timer"
+ default y
+ depends on VIRTIO_RTC_PTP && ARM_ARCH_TIMER
+ help
+This enables Virtio RTC cross-timestamping using the Arm Generic Timer.
+It only has an effect if the Virtio RTC device also supports this. The
+cross-timestamp is available through the PTP clock driver precise
+cross-timestamp ioctl (PTP_SYS_OFFSET_PRECISE2 or
+PTP_SYS_OFFSET_PRECISE).
+
+If unsure, say Y.
+
endif # VIRTIO_MENU
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 4d48cbcae6bb..781dff9f8822 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_VIRTIO_DMA_SHARED_BUFFER) += virtio_dma_buf.o
obj-$(CONFIG_VIRTIO_RTC) += virtio_rtc.o
virtio_rtc-y := virtio_rtc_driver.o
virtio_rtc-$(CONFIG_VIRTIO_RTC_PTP) += virtio_rtc_ptp.o
+virtio_rtc-$(CONFIG_VIRTIO_RTC_ARM) += virtio_rtc_arm.o
diff --git a/drivers/virtio/virtio_rtc_arm.c b/drivers/virtio/virtio_rtc_arm.c
new file mode 100644
index ..2367f054081c
--- /dev/null
+++ b/drivers/virtio/virtio_rtc_arm.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Provides cross-timestamp params for Arm.
+ *
+ * Copyright (C) 2022-2023 OpenSynergy GmbH
+ */
+
+#include
+#include
+
+#include
+
+#include "virtio_rtc_internal.h"
+
+static const u16 viortc_hw_counters[] = { VIRTIO_RTC_COUNTER_ARM_VIRT,
+ VIRTIO_RTC_COUNTER_ARM_PHYS };
+
+/* see header for doc */
+int viortc_hw_get_counters(const u16 **hw_counters, int *num_hw_counters)
+{
+ *hw_counters = viortc_hw_counters;
+ *num_hw_counters = ARRAY_SIZE(viortc_hw_counters);
+
+ return 0;
+}
+
+/* see header for doc */
+int viortc_hw_xtstamp_params(u16 *hw_counter, struct clocksource **cs)
+{
+ *cs = arch_timer_get_cs();
+
+ switch (arch_timer_counter_get_type()) {
+ case ARCH_COUNTER_CP15_VIRT:
+ *hw_counter = VIRTIO_RTC_COUNTER_ARM_VIRT;
+ break;
+ case ARCH_COUNTER_CP15_PHYS:
+ *hw_counter = VIRTIO_RTC_COUNTER_ARM_PHYS;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
--
2.39.2
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] [RFC PATCH 0/7] Add virtio_rtc module and related changes
This patch series adds the virtio_rtc module, and related bugfixes and
small interface extensions. The virtio_rtc module implements a driver
compatible with the proposed Virtio RTC device specification [1]. The
Virtio RTC (Real Time Clock) device provides information about current
time. The device can provide different clocks, e.g. for the UTC or TAI time
standards, or for physical time elapsed since some past epoch. The driver
can read the clocks with simple or more accurate methods.
This patch series depends, through its arm_arch_timer patch, on patch
"arm64/arch_timer: Provide noinstr sched_clock_read() functions" [3] which
seems not to be available with all development branches yet. Pull [2] for
an equivalent series on top of mst/linux-next, which should apply without
the above patch. Pull [5] for an equivalent series on top of
tip/timers/core. Pull [6] for an equivalent series on top of
tip/sched/core.
The series first fixes some bugs in the get_device_system_crosststamp()
interpolation code, which is required for reliable virtio_rtc operation.
Next, expose some Arm Generic Timer clocksource details for virtio_rtc.
Last, add the virtio_rtc implementation.
For the Virtio RTC device, there is currently a proprietary implementation,
which has been used for provisional testing.
virtio_rtc exposes clocks as PTP clocks to userspace, similar to ptp_kvm.
If both the Virtio RTC device and this driver have special support for the
current clocksource, time synchronization programs can use
cross-timestamping using ioctl PTP_SYS_OFFSET_PRECISE2 aka
PTP_SYS_OFFSET_PRECISE. Similar to ptp_kvm, system time synchronization
with single-digit ns precision is possible with a quiescent reference clock
(from the Virtio RTC device). This works even when the Virtio device
response is slow compared to ptp_kvm hypercalls.
The following illustrates a test using PTP_SYS_OFFSET_PRECISE, with
interspersed strace log and chrony [4] refclocks log, on arm64. In the
example, chrony tracks a virtio_rtc PTP clock ("PHCV", /dev/ptp0). The raw
offset between the virtio_rtc clock and CLOCK_REALTIME is 0 to 1 ns. At the
device side, the Virtio RTC device artificially delays both the clock read
request, and the response, by 50 ms. Cross-timestamp interpolation still
works with this delay. chrony also monitors a ptp_kvm clock ("PHCK",
/dev/ptp3) for comparison, which yields a similar offset.
ioctl(5, PTP_SYS_OFFSET_PRECISE, 0xe86691c8) = 0
<0.000329>
===
Date (UTC) Time Refid DP L P Raw offset Cooked offset
Disp.
===
2023-06-29 18:49:55.595742 PHCK0 N 0 1.00e-09 8.717931e-10
5.500e-08
2023-06-29 18:49:55.595742 PHCK- N - -8.717931e-10
5.500e-08
ioctl(6, PTP_SYS_OFFSET_PRECISE, 0xe86691c8) = 0
<0.101545>
2023-06-29 18:49:56.147766 PHCV0 N 0 1.00e-09 8.801870e-10
5.500e-08
2023-06-29 18:49:56.147766 PHCV- N - -8.801870e-10
5.500e-08
ioctl(5, PTP_SYS_OFFSET_PRECISE, 0xe86691c8) = 0
<0.000195>
2023-06-29 18:49:56.202446 PHCK0 N 0 1.00e-09 7.364180e-10
5.500e-08
2023-06-29 18:49:56.202446 PHCK- N - -7.364180e-10
5.500e-08
ioctl(6, PTP_SYS_OFFSET_PRECISE, 0xe86691c8) = 0
<0.101484>
2023-06-29 18:49:56.754641 PHCV0 N 0 0.00e+00 -2.617368e-10
5.500e-08
2023-06-29 18:49:56.754641 PHCV- N - - -2.617368e-10
5.500e-08
ioctl(5, PTP_SYS_OFFSET_PRECISE, 0xe86691c8) = 0
<0.000270>
2023-06-29 18:49:56.809282 PHCK0 N 0 1.00e-09 7.779321e-10
5.500e-08
2023-06-29 18:49:56.809282 PHCK- N - -7.779321e-10
5.500e-08
ioctl(6, PTP_SYS_OFFSET_PRECISE, 0xe86691c8) = 0
<0.101510>
2023-06-29 18:49:57.361376 PHCV0 N 0 0.00e+00 -2.198794e-10
5.500e-08
2023-06-29 18:49:57.361376 PHCV- N - - -2.198794e-10
5.500e-08
This patch series only adds special support for the Arm Generic Timer
clocksource. At the driver side, it should be easy to support more
clocksources.
Without special support for the current clocksource, time synchronization
programs can still use ioctl PTP_SYS_OFFSET_EXTENDED2 aka
PTP_SYS_OFFSET_EXTENDED. In this case, precision will generally be worse
and will depend on the Virtio device response characteristics.
The following illustrates a test using PTP_SYS_OFFSET_EXTENDED, with
interspersed strace log and chrony refclocks log, on x86-64 (with `ts'
values omitted):
ioctl(5, PTP_SYS_OFFSET_EXTENDED, {n_samples=10, ts=OMITTED}) = 0
===
Date (UTC) Time Refid DP L P Raw offset Cooked offset
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Sat, Jul 01, 2023 at 12:09:09AM +0800, Heng Qi wrote:
> On Fri, Jun 30, 2023 at 10:52:25AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
> > >
> > >
> > > 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > > > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > > > >
> > > > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > > > From: Heng Qi
> > > > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > > > >
> > > > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > > > > > > > > > From: Michael S. Tsirkin
> > > > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > > > +1.
> > > > > > > > > > > >
> > > > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine
> > > > > > > > > > > > to have its
> > > > > > > > > > > > own structure and commands.
> > > > > > > > > > > > There is no need to send additional RSS fields when we
> > > > > > > > > > > > configure
> > > > > > > > > > > > inner header hash.
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks.
> > > > > > > > > > > Not RSS, hash calculations. It's not critical, but I note
> > > > > > > > > > > that
> > > > > > > > > > > practically you said you will enable this with symmetric
> > > > > > > > > > > hash so it
> > > > > > > > > > > makes sense to me to send this in the same command with
> > > > > > > > > > > the key.
> > > > > > > > > > >
> > > > > > > > > > In the v19, we have,
> > > > > > > > > >
> > > > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires
> > > > > > > > > > VIRTIO_NET_F_CTRL_VQ
> > > > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > > > So it is done along with rss, so in same struct as rss
> > > > > > > > > > config is fine.
> > > > > > > > > Do you mean having both virtio_net_rss_config and
> > > > > > > > > virtio_net_hash_config
> > > > > > > > > have enabled_hash_types?
> > > > > > > > > Like this:
> > > > > > > > >
> > > > > > > > > struct virtio_net_rss_config {
> > > > > > > > > le32 hash_types;
> > > > > > > > > le16 indirection_table_mask;
> > > > > > > > > struct rss_rq_id unclassified_queue;
> > > > > > > > > struct rss_rq_id
> > > > > > > > > indirection_table[indirection_table_length];
> > > > > > > > > le16 max_tx_vq;
> > > > > > > > > u8 hash_key_length;
> > > > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > > > +le32 enabled_tunnel_types;
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > struct virtio_net_hash_config {
> > > > > > > > > le32 hash_types;
> > > > > > > > > -le16 reserved[4];
> > > > > > > > > +le32 enabled_tunnel_types;
> > > > > > > > > +le16 reserved[2];
> > > > > > > > > u8 hash_key_length;
> > > > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > > > };
> > > > > > Oh, I forgot that rss and hash had identical structures.
> > > > > > And we want to keep that I think.
> > > > > >
> > > > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > > > apply to both hash calculation and rss?
> > > > > Yes. What I'm trying to say is that making the inner header hash and
> > > > > RSS/hash calculation clear delimitation will make everything easier.
> > > > > The inner header hash just configures enabled_tunnel_types.
> > > > > The RSS/hash calculation is configured as before without modification.
> > > > >
> > > > > > I note we normally have separate configs for hash and rss.
> > > > > > So to keep it consistent what should we do? two set commands?
> > > > > As I explained above, like outer udp port hash/symmetric toeplitz
> > > > > hash,
> > > > > these fall under the umbrella of RSS/hash calculation.
> > > > Yes but note that symmetric toeplitz hash has to be configured
> > > > separately for RSS and for hashing.
> > >
> > > Yes, this requires a field like \field{mode}, with different values
> > > corresponding to different hashing algorithms, such as toeplitz or
> > > symmetric
> > > toeplitz.
> > > The outer port hash belongs to RSS/hash calculation.
> >
> > So there will be need for more fields.
>
> Yes, the mode field is outside of RSS/hash, as it should be at a higher
> level than RSS/hash.
>
> > To me this implies extending with struct virtio_net_rss_tunnel_config
> > is a better idea since we then have some reserved space to
> > put "mode" down the road (in the reserved[6] space below).
> >
> > No?
> >
> > > >
> > > > > Let's keep the inner header hash simple.
> > > > >
> > > > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > > > Certainly. See:
> > > > >
> > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > al
Re: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 11:38:49AM +, Parav Pandit wrote: > > > > From: virtio-dev@lists.oasis-open.org On > > Behalf Of Michael S. Tsirkin > > Sent: Thursday, June 29, 2023 3:07 AM > > > > > Well SHOULD also does not mean "ok to just ignore". > > > > > > > > This word, or the adjective "RECOMMENDED", mean that there > > > >may exist valid reasons in particular circumstances to > > > > ignore a > > > >particular item, but the full implications must be > > > > understood and > > > >carefully weighed before choosing a different course. > > > > > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is > > not good. > > > > You said this already. Let's not repeat ourselves please. > > > > It's a single word. We need to work on a patch, we can argue about small > > detail > > once it's posted. It's not in your list of plans so I am guessing we need > > someone > > on Red Hat side to work on this? > > Yes, I prefer to work together and before writing the patch. > As just DMA for config space is only point fix problem. > So lets discuss in new thread and improve this for config space and also > other limitations (like support multi-address VQ). > Interesting. What is multi-address VQ? -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev][PATCH 1/2] virtio-spi: define the DEVICE ID for virtio SPI master
On Mon, Apr 17, 2023 at 10:03:52PM +0800, Haixu Cui wrote:
> Define the DEVICE ID of virtio SPI master device as 45.
It does not looks like patch 2 will make it in time for 1.3
but should we vote on reserving device id maybe?
If yes pls create a github issue and request vote on list.
> ---
> content.tex | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index cff548a..29f2859 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -682,6 +682,8 @@ \chapter{Device Types}\label{sec:Device Types}
> \hline
> 44 & ISM device \\
> \hline
> +45 & SPI master \\
> +\hline
> \end{tabular}
>
> Some of the devices above are unspecified by this document,
> --
> 2.17.1
>
>
> -
> To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 10:04:08PM +0800, Heng Qi wrote:
>
>
> 在 2023/6/30 下午4:17, Michael S. Tsirkin 写道:
> > On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
> > >
> > > 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > > > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > > > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > > > From: Heng Qi
> > > > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > > > >
> > > > > > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > > > > > > > From: Michael S. Tsirkin
> > > > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > > > +1.
> > > > > > > > > >
> > > > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to
> > > > > > > > > > have its
> > > > > > > > > > own structure and commands.
> > > > > > > > > > There is no need to send additional RSS fields when we
> > > > > > > > > > configure
> > > > > > > > > > inner header hash.
> > > > > > > > > >
> > > > > > > > > > Thanks.
> > > > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > > > practically you said you will enable this with symmetric hash
> > > > > > > > > so it
> > > > > > > > > makes sense to me to send this in the same command with the
> > > > > > > > > key.
> > > > > > > > >
> > > > > > > > In the v19, we have,
> > > > > > > >
> > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > > > So it is done along with rss, so in same struct as rss config
> > > > > > > > is fine.
> > > > > > > Do you mean having both virtio_net_rss_config and
> > > > > > > virtio_net_hash_config
> > > > > > > have enabled_hash_types?
> > > > > > > Like this:
> > > > > > >
> > > > > > > struct virtio_net_rss_config {
> > > > > > > le32 hash_types;
> > > > > > > le16 indirection_table_mask;
> > > > > > > struct rss_rq_id unclassified_queue;
> > > > > > > struct rss_rq_id
> > > > > > > indirection_table[indirection_table_length];
> > > > > > > le16 max_tx_vq;
> > > > > > > u8 hash_key_length;
> > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > +le32 enabled_tunnel_types;
> > > > > > > };
> > > > > > >
> > > > > > > struct virtio_net_hash_config {
> > > > > > > le32 hash_types;
> > > > > > > -le16 reserved[4];
> > > > > > > +le32 enabled_tunnel_types;
> > > > > > > +le16 reserved[2];
> > > > > > > u8 hash_key_length;
> > > > > > > u8 hash_key_data[hash_key_length];
> > > > > > > };
> > > > Oh, I forgot that rss and hash had identical structures.
> > > > And we want to keep that I think.
> > > >
> > > > But now it's not clear to me: does the same enabled_tunnel_types
> > > > apply to both hash calculation and rss?
> > > Yes. What I'm trying to say is that making the inner header hash and
> > > RSS/hash calculation clear delimitation will make everything easier.
> > > The inner header hash just configures enabled_tunnel_types.
> > > The RSS/hash calculation is configured as before without modification.
> > >
> > > > I note we normally have separate configs for hash and rss.
> > > > So to keep it consistent what should we do? two set commands?
> > > As I explained above, like outer udp port hash/symmetric toeplitz hash,
> > > these fall under the umbrella of RSS/hash calculation.
> > Yes but note that symmetric toeplitz hash has to be configured
> > separately for RSS and for hashing.
>
> Yes, this requires a field like \field{mode}, with different values
> corresponding to different hashing algorithms, such as toeplitz or symmetric
> toeplitz.
> The outer port hash belongs to RSS/hash calculation.
So there will be need for more fields.
To me this implies extending with struct virtio_net_rss_tunnel_config
is a better idea since we then have some reserved space to
put "mode" down the road (in the reserved[6] space below).
No?
> >
> > > Let's keep the inner header hash simple.
> > >
> > > > Or does enabled_tunnel_types apply to both rss and hash?
> > > Certainly. See:
> > >
> > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> > > with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> >
> > It does not really say that.
> Oh! I understand now. I think a VIRTIO_NET_CTRL_HASH_TUNNEL_SET command is
> applied to hash and RSS. Yes.
> When one wants to configure inner header hash separately, use
> VIRTIO_NET_CTRL_HASH_TUNNEL_SET command to send enabled_tunnel_types
> separately.
> When one wants to configure both inner header hash and RSS/hash, use
> VIRTIO_NET_CTRL_HASH_TUNNEL_SET together with
> VIRTIO_NET_CTRL_MQ_RSS_CONFIG/VIRTIO_NET_CTRL_MQ_HASH_CONFIG.
> The inner header hash is decoupled from RSS/hash, and no extra fields will
> be sent every configuration.
But why is tunnel so different? Rest of things can
RE: [virtio-dev] Re: [PATCH v19] virtio-net: support inner header hash
> From: virtio-dev@lists.oasis-open.org On
> Behalf Of Jason Wang
> Sent: Wednesday, June 28, 2023 11:18 PM
>
> It's not the fault of config space itself, but the way to access the config
> space,
> more below.
>
Sure and it is linked to old and new entries into it. I explained in this
thread with discussion with Michael.
Please refer it.
> >
> > > struct virtio_net_config {
> > > u8 mac[6];
> > > le16 status;
> > > le16 max_virtqueue_pairs;
> > > le16 mtu;
> > > le32 speed;
> > > u8 duplex;
> > > u8 rss_max_key_size;
> > > le16 rss_max_indirection_table_length;
> > > le32 supported_hash_types;
> > > };
> > >
> > > Which of the above do you think can be accessed frequently and which
> > > part of the spec says it must be stored in the onchip area?
> > >
> > Most are not accessed frequently.
> > The fact that they are in MMIO a device needs to place in a memory with
> > tight
> latency budget.
>
> This really depends on the implementation and vendor architectures.
> For example,
>
> 1) MSI BAR might require much more resources than a simple device
> configuration space
Sure, that is also getting solved in parallel outside of virtio spec.
So MSI BAR saving is not replacement, it is different area that is getting
optimized.
> 2) I was told my some vendors that the virtqueue is much more valuable than
> MMIO
Not sure I understand "valuable".
> 3) We can introduce new ways to access the device configuration space
>
Yes, new way for existing config space and extended (new fields) of config
space.
I explained the reason to differentiate this few times in thread to Michael, so
I will omit repeating here.
> > Spec is not going to talk on onchip area, it is the reflection of spec that
> > forces
> certain inefficient implementation .
>
> Exactly, it's implementation specific, so config space is fine, we just need
> to
> invent new methods to access them that fit for your hardware.
Yes. Lets work on it in the new thread.
RE: [virtio-dev] Re: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
> From: virtio-dev@lists.oasis-open.org On > Behalf Of Michael S. Tsirkin > Sent: Thursday, June 29, 2023 3:07 AM > > > Well SHOULD also does not mean "ok to just ignore". > > > > > > This word, or the adjective "RECOMMENDED", mean that there > > > may exist valid reasons in particular circumstances to ignore a > > > particular item, but the full implications must be understood and > > > carefully weighed before choosing a different course. > > > > > RECOMMENDED and SHOULD forces the device to support MMIO, which is > not good. > > You said this already. Let's not repeat ourselves please. > > It's a single word. We need to work on a patch, we can argue about small > detail > once it's posted. It's not in your list of plans so I am guessing we need > someone > on Red Hat side to work on this? Yes, I prefer to work together and before writing the patch. As just DMA for config space is only point fix problem. So lets discuss in new thread and improve this for config space and also other limitations (like support multi-address VQ). - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] [RFC] virtio 1.3 schedule
> From: virtio-comm...@lists.oasis-open.org open.org> On Behalf Of Cornelia Huck > Sent: Friday, June 30, 2023 4:48 AM > For the inner header hash, I see there's already an issue (#173), so it's > good. I > have not followed the aq legacy access feature -- if everyone agrees that we > should be able to vote on it in July, we should have an issue for it as well > (I > cannot find one just by looking at > titles.) > For legacy registers access, it is https://github.com/oasis-tcs/virtio-spec/issues/167 - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] [VIRTIO GPU PATCH 1/1] virtio-gpu: Add new feature flag VIRTIO_GPU_F_FREEZING
When we suspend/resume guest on Xen, the display can't come back.
This is because when guest suspended, it called into Qemu. Then
Qemu destroyed all resources which is used for display. So that
guest's display can't come back to the time when it was suspended.
To solve above problem, I added a new mechanism that when guest is
suspending, it will notify Qemu, and then Qemu will not destroy
resources.
Due to that mechanism needs cooperation between guest and host,
I need to add a new feature flag, so that guest and host can
negotiate whenever freezing is supported or not.
Signed-off-by: Jiqian Chen
---
device-types/gpu/description.tex | 27 +++
1 file changed, 27 insertions(+)
diff --git a/device-types/gpu/description.tex b/device-types/gpu/description.tex
index 4435248..11e37bf 100644
--- a/device-types/gpu/description.tex
+++ b/device-types/gpu/description.tex
@@ -37,6 +37,8 @@ \subsection{Feature bits}\label{sec:Device Types / GPU Device
/ Feature bits}
resources is supported.
\item[VIRTIO_GPU_F_CONTEXT_INIT (4)] multiple context types and
synchronization timelines supported. Requires VIRTIO_GPU_F_VIRGL.
+\item[VIRTIO_GPU_F_FREEZING (5)] freezing virtio-gpu and keeping resources
+ alive is supported.
\end{description}
\subsection{Device configuration layout}\label{sec:Device Types / GPU Device /
Device configuration layout}
@@ -228,6 +230,9 @@ \subsubsection{Device Operation: Request
header}\label{sec:Device Types / GPU De
VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
VIRTIO_GPU_CMD_MOVE_CURSOR,
+/* status */
+VIRTIO_GPU_CMD_STATUS_FREEZING = 0x0400,
+
/* success responses */
VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
@@ -838,6 +843,28 @@ \subsubsection{Device Operation: cursorq}\label{sec:Device
Types / GPU Device /
\end{description}
+\subsubsection{Device Operation: status}\label{sec:Device Types / GPU Device /
Device Operation / Device Operation: cursorq}
+
+\begin{lstlisting}
+struct virtio_gpu_status_freezing {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __u32 freezing;
+};
+\end{lstlisting}
+
+\begin{description}
+
+\item[VIRTIO_GPU_CMD_STATUS_FREEZING]
+Notify freezing status through controlq.
+Request data is \field{struct virtio_gpu_status_freezing}.
+Response type is VIRTIO_GPU_RESP_OK_NODATA.
+
+Guest notifies QEMU if virtio-gpu is in freezing status or not in
\field{freezing}.
+zero means it is not in freezing status, none-zero is the opposite.
+When guest is in freezing status, QEMU will not destroy resources.
+
+\end{description}
+
\subsection{VGA Compatibility}\label{sec:Device Types / GPU Device / VGA
Compatibility}
Applies to Virtio Over PCI only. The GPU device can come with and
--
2.34.1
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] [VIRTIO GPU PATCH 0/1] Add new feature flag VIRTIO_GPU_F_FREEZING
Hi all, I am working to implement virtgpu S3 function on Xen. Currently on Xen, if we start a guest through Qemu with enabling virtgpu, and then suspend and s3resume guest. We can find that the guest kernel comes back, but the display doesn't. It just shown a black screen. That is because when guest was during suspending, it called into Qemu and Qemu destroyed all resources and reset renderer. This made the display gone after guest resumed. So, I add a mechanism that when guest is suspending, it will notify Qemu, and then Qemu will not destroy resources. That can help guest's display come back. As discussed and suggested by Robert Beckett and Gerd Hoffmann on v1 qemu's mailing list. Due to that mechanism needs cooperation between guest and host. What's more, as virtio drivers by design paravirt drivers, it is reasonable for guest to accept some cooperation with host to manage suspend/resume. So I request to add a new feature flag, so that guest and host can negotiate whenever freezing is supported or not. Attach the patches of Qemu and kernel: Qemu v1: https://lore.kernel.org/qemu-devel/20230608025655.1674357-2-jiqian.c...@amd.com/ v2: https://lore.kernel.org/qemu-devel/20230630070016.841459-1-jiqian.c...@amd.com/T/#t Kernel v1: https://lore.kernel.org/lkml/20230608063857.1677973-1-jiqian.c...@amd.com/ v2: https://lore.kernel.org/lkml/20230630073448.842767-1-jiqian.c...@amd.com/T/#t Best regards, Jiqian Chen. Jiqian Chen (1): virtio-gpu: Add new feature flag VIRTIO_GPU_F_FREEZING device-types/gpu/description.tex | 27 +++ 1 file changed, 27 insertions(+) -- 2.34.1 - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] [RFC] virtio 1.3 schedule
On Fri, Jun 30, 2023 at 10:47:33AM +0200, Cornelia Huck wrote: > On Thu, Jun 29 2023, "Michael S. Tsirkin" wrote: > > > On Thu, Jun 29, 2023 at 05:41:34PM +, Parav Pandit wrote: > >> Hi Cornelia, Michael, > >> > >> > From: virtio-comm...@lists.oasis-open.org >> > open.org> On Behalf Of Cornelia Huck > >> > Sent: Wednesday, April 12, 2023 12:17 PM > >> > >> > >> - July 1st 2023: enter feature freeze; all proposed changes must at > >> > >> least have an open github issue that refers to a proposal on-list > >> > >> - August 1st 2023: enter change freeze; everything needs to have been > >> > >> voted upon (i.e. we can start with preparations for a release like > >> > >> compiling the change log) > >> > >> - at the same time, fork virtio-next, which can be used for > >> > >> development > >> > >> while we're working on the release > >> > >> - August/September 2023: prepare draft, initiate initial voting etc. > >> > > > >> > > Hmm looks like you forgot a 30 day the public review period (or is > >> > > that included in the "etc"? ). During this period we will likely > >> > > receive review comments which we have to address. Should the changes > >> > > turn out to be material, another public review period will be required. > >> > > Let's spell it out please. > >> > > >> > Yeah, that's what I meant with "etc"... let's make this: > >> > > >> > - August 2023: prepare draft, aim to have it ready and voted upon by > >> > August 31 the latest (preferrably earlier) > >> > - September 2023: public review period (30 days) > >> > - October 2023: another public review period, if needed; otherwise, get > >> > comittee specification ready and voted upon > >> > - October (maybe November, if we're running late) 2023: virtio 1.3 > >> > released > >> > > >> > The most important dates are feature freeze on Jul 1 and change freeze > >> > on Aug > >> > 1, I guess; for the rest, we depend on whether people are out on PTO, > >> > which > >> > kind of feedback we get, etc. -- but as long as we release > >> > 1.3 in 2023, I'll be happy enough. > >> > >> We have two extension features that has gone through several reviews and > >> are in advance stage (and also late) for 1.3. :) > >> > >> 1. inner hash v19, rolling to v20 anytime this week. 1 Patch. > >> [1] > >> https://lists.oasis-open.org/archives/virtio-comment/202306/msg00490.html > >> > >> 2. first user of aq for legacy access at v7. > >> 2 patches. First 2 are Fixes. > >> [2] > >> https://lists.oasis-open.org/archives/virtio-comment/202306/msg00491.html > >> > >> Both need few more days of review and vote. > >> Shall we please have few days in July for them so that it can be part of > >> 1.3? > >> or whole July is needed to prepare the draft spec? > > > > I think it's not too bad, projects that do releases every couple of > > months can afford to be strict, we do them ones a year and a half > > it makes sense to be flexible. > > Yes, my only concern is to avoid slipping too badly -- and the "feature > freeze" period was actually intended to give us some time to wrap up > outstanding features. > > For the inner header hash, I see there's already an issue (#173), so > it's good. I have not followed the aq legacy access feature -- if > everyone agrees that we should be able to vote on it in July, we should > have an issue for it as well (I cannot find one just by looking at > titles.) > > Any other features that people are aware of? Was hoping for virtio-can but it looks like it's still in RFC stage. I am guessing the spec should be copied to people who reviewed the driver. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] [RFC] virtio 1.3 schedule
On Thu, Jun 29 2023, "Michael S. Tsirkin" wrote: > On Thu, Jun 29, 2023 at 05:41:34PM +, Parav Pandit wrote: >> Hi Cornelia, Michael, >> >> > From: virtio-comm...@lists.oasis-open.org > > open.org> On Behalf Of Cornelia Huck >> > Sent: Wednesday, April 12, 2023 12:17 PM >> >> > >> - July 1st 2023: enter feature freeze; all proposed changes must at >> > >> least have an open github issue that refers to a proposal on-list >> > >> - August 1st 2023: enter change freeze; everything needs to have been >> > >> voted upon (i.e. we can start with preparations for a release like >> > >> compiling the change log) >> > >> - at the same time, fork virtio-next, which can be used for development >> > >> while we're working on the release >> > >> - August/September 2023: prepare draft, initiate initial voting etc. >> > > >> > > Hmm looks like you forgot a 30 day the public review period (or is >> > > that included in the "etc"? ). During this period we will likely >> > > receive review comments which we have to address. Should the changes >> > > turn out to be material, another public review period will be required. >> > > Let's spell it out please. >> > >> > Yeah, that's what I meant with "etc"... let's make this: >> > >> > - August 2023: prepare draft, aim to have it ready and voted upon by >> > August 31 the latest (preferrably earlier) >> > - September 2023: public review period (30 days) >> > - October 2023: another public review period, if needed; otherwise, get >> > comittee specification ready and voted upon >> > - October (maybe November, if we're running late) 2023: virtio 1.3 >> > released >> > >> > The most important dates are feature freeze on Jul 1 and change freeze on >> > Aug >> > 1, I guess; for the rest, we depend on whether people are out on PTO, which >> > kind of feedback we get, etc. -- but as long as we release >> > 1.3 in 2023, I'll be happy enough. >> >> We have two extension features that has gone through several reviews and are >> in advance stage (and also late) for 1.3. :) >> >> 1. inner hash v19, rolling to v20 anytime this week. 1 Patch. >> [1] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00490.html >> >> 2. first user of aq for legacy access at v7. >> 2 patches. First 2 are Fixes. >> [2] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00491.html >> >> Both need few more days of review and vote. >> Shall we please have few days in July for them so that it can be part of >> 1.3? >> or whole July is needed to prepare the draft spec? > > I think it's not too bad, projects that do releases every couple of > months can afford to be strict, we do them ones a year and a half > it makes sense to be flexible. Yes, my only concern is to avoid slipping too badly -- and the "feature freeze" period was actually intended to give us some time to wrap up outstanding features. For the inner header hash, I see there's already an issue (#173), so it's good. I have not followed the aq legacy access feature -- if everyone agrees that we should be able to vote on it in July, we should have an issue for it as well (I cannot find one just by looking at titles.) Any other features that people are aware of? - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v19] virtio-net: support inner header hash
On Fri, Jun 30, 2023 at 02:15:13PM +0800, Heng Qi wrote:
>
>
> 在 2023/6/30 下午1:59, Michael S. Tsirkin 写道:
> > On Fri, Jun 30, 2023 at 09:55:41AM +0800, Heng Qi wrote:
> > >
> > > 在 2023/6/30 上午9:36, Parav Pandit 写道:
> > > > > From: Heng Qi
> > > > > Sent: Thursday, June 29, 2023 8:54 PM
> > > > >
> > > > > On Thu, Jun 29, 2023 at 04:59:28PM +, Parav Pandit wrote:
> > > > > > > From: Michael S. Tsirkin
> > > > > > > Sent: Thursday, June 29, 2023 7:48 AM
> > > > > > > > > struct virtio_net_hash_config reserved is fine.
> > > > > > > > +1.
> > > > > > > >
> > > > > > > > Inner header hash is orthogonal to RSS, and it's fine to have
> > > > > > > > its
> > > > > > > > own structure and commands.
> > > > > > > > There is no need to send additional RSS fields when we configure
> > > > > > > > inner header hash.
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > Not RSS, hash calculations. It's not critical, but I note that
> > > > > > > practically you said you will enable this with symmetric hash so
> > > > > > > it
> > > > > > > makes sense to me to send this in the same command with the key.
> > > > > > >
> > > > > > In the v19, we have,
> > > > > >
> > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
> > > > > > So it is done along with rss, so in same struct as rss config is
> > > > > > fine.
> > > > > Do you mean having both virtio_net_rss_config and
> > > > > virtio_net_hash_config
> > > > > have enabled_hash_types?
> > > > > Like this:
> > > > >
> > > > > struct virtio_net_rss_config {
> > > > >le32 hash_types;
> > > > >le16 indirection_table_mask;
> > > > >struct rss_rq_id unclassified_queue;
> > > > >struct rss_rq_id indirection_table[indirection_table_length];
> > > > >le16 max_tx_vq;
> > > > >u8 hash_key_length;
> > > > >u8 hash_key_data[hash_key_length];
> > > > > +le32 enabled_tunnel_types;
> > > > > };
> > > > >
> > > > > struct virtio_net_hash_config {
> > > > >le32 hash_types;
> > > > > -le16 reserved[4];
> > > > > +le32 enabled_tunnel_types;
> > > > > +le16 reserved[2];
> > > > >u8 hash_key_length;
> > > > >u8 hash_key_data[hash_key_length];
> > > > > };
> > Oh, I forgot that rss and hash had identical structures.
> > And we want to keep that I think.
> >
> > But now it's not clear to me: does the same enabled_tunnel_types
> > apply to both hash calculation and rss?
>
> Yes. What I'm trying to say is that making the inner header hash and
> RSS/hash calculation clear delimitation will make everything easier.
> The inner header hash just configures enabled_tunnel_types.
> The RSS/hash calculation is configured as before without modification.
>
> > I note we normally have separate configs for hash and rss.
> > So to keep it consistent what should we do? two set commands?
>
> As I explained above, like outer udp port hash/symmetric toeplitz hash,
> these fall under the umbrella of RSS/hash calculation.
Yes but note that symmetric toeplitz hash has to be configured
separately for RSS and for hashing.
> Let's keep the inner header hash simple.
>
> > Or does enabled_tunnel_types apply to both rss and hash?
>
> Certainly. See:
>
> +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along
> with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
It does not really say that.
> >
> > We should have reserved more space. We can still do it if you like:
> >
> > struct virtio_net_rss_tunnel_config {
> >le32 enabled_tunnel_types;
> >le16 reserved[6];
> >struct virtio_net_rss_config hash;
> > };
> >
> > struct virtio_net_hash_tunnel_config {
> >le32 enabled_tunnel_types;
> >le16 reserved[6];
> >struct virtio_net_hash_config hash;
> > };
> >
> > ?
> >
> >
> >
> >
> >
> > > > >
> > > > > If yes, this should have been discussed in v10 [1] before,
> > > > > enabled_tunnel_types
> > > > > in virtio_net_rss_config will follow the variable length field and
> > > > > cause
> > > > > misalignment.
> > > > >
> > > > > If we let the inner header hash reuse the virtio_net_hash_config
> > > > > structure, it
> > > > > can work, but the only disadvantage is that the configuration of the
> > > > > inner
> > > > > header hash and *RSS*(not hash calculations) becomes somewhat coupled.
> > > > > Just imagine:
> > > > > If the driver and the device negotiated VIRTIO_NET_F_HASH_TUNNEL and
> > > > > VIRTIO_NET_F_RSS, but did not negotiate VIRTIO_NET_F_HASH_REPORT, 1.
> > > > > then if we only want to configure the inner header hash (such as
> > > > > enabled_tunnel_types), it is good for us to send
> > > > > virtio_net_hash_config alone;
> > > > > 2. but then if we want to configure the inner header hash and RSS
> > > > > (such as
> > > > > indirection table), we need to send all virtio_net_rss_config and
> > > > > virtio_net_hash_config on
