[virtio-dev] Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum
On Mon, Dec 18, 2023 at 12:54 PM Heng Qi wrote:
>
>
>
> 在 2023/12/18 上午11:10, Jason Wang 写道:
> > On Fri, Dec 15, 2023 at 5:51 PM Heng Qi wrote:
> >> Hi all!
> >>
> >> I would like to ask if anyone has any comments on this version, if so
> >> please let me know!
> >> If not, I will collect Michael's comments and publish a new version next
> >> Monday.
> > I have a dumb question. (And sorry if I asked it before)
> >
> > Looking at the spec and code. It looks to me DATA_VALID could be set
> > without GUEST_CSUM.
>
> I don't see that in the spec.
> Am I missing something? [1][2]
>
> [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> VIRTIO_NET_HDR_F_DATA_VALID bit in flags can be set: if so, device has
> validated the packet checksum. In case of multiple encapsulated
> protocols, one level of checksums has been validated.
> Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> *enable receive checksum*, large receive offload and ECN support which
> are the input equivalents of the transmit checksum, transmit
> segmentation *offloading* and ECN features, as described in 5.1.6.2.
>
> [2] If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST set
> flags to zero* and SHOULD supply a fully checksummed packet to the driver.
So this is kind of ambiguous and seems not what I wanted when I wrote
the code for DATA_VALID in 2011.
NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
correct. So spec had
"""
If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
is set, the driver MUST NOT rely on the packet checksum being correct.
"""
For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
exclusive with CHECKSUM_PARTAIL. And this is what Linux did right now:
For tun_put_user():
if (skb->ip_summed == CHECKSUM_PARTIAL) {
...
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
} /* else everything is zero */
This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
I was not wrong.
And in receive_buf():
if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
skb->ip_summed = CHECKSUM_UNNECESSARY;
I think we can fix this by safely removing "*MUST set flags to zero*"
in [2] from the spec.
Thanks
>
> I think the reason why the feature bit is not checked in the code is
> because the check is omitted because it is on a per-packet basis,
> just like the reason why supported_valid_types is not needed as
> discussed in the v4 version threads. It is not unnecessary.
>
> Thanks!
>
> >
> > If yes, why do we need to bother here? If we disable GUEST_CSUM, the
> > packet will contain checksum. And if the device sets DATA_VALID, it
> > means the checksum is validated.
> >
> > Thanks
> >
> >
> >
> >> Since Christmas is coming, I think this feature may be in danger of
> >> following the pace of
> >> our hw version releases, so I sincerely request that you please review
> >> it as soon as possible.
> >>
> >> Thanks!
> >>
> >> 在 2023/12/12 下午5:30, Heng Qi 写道:
> >>>
> >>> 在 2023/12/12 下午5:23, Heng Qi 写道:
>
> 在 2023/12/12 下午4:44, Michael S. Tsirkin 写道:
> > On Tue, Dec 12, 2023 at 11:28:21AM +0800, Heng Qi wrote:
> >> 在 2023/12/12 上午12:35, Michael S. Tsirkin 写道:
> >>> On Mon, Dec 11, 2023 at 05:11:59PM +0800, Heng Qi wrote:
> virtio-net works in a virtualized system and is somewhat
> different from
> physical nics. One of the differences is that to save virtio device
> resources, rx may receive partially checksummed packets. However,
> XDP may
> cause partially checksummed packets to be dropped.
> So XDP loading currently conflicts with the feature
> VIRTIO_NET_F_GUEST_CSUM.
>
> This patch lets the device to supply fully checksummed packets to
> the driver.
> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the
> benefits of
> device validation checksum.
>
> In addition, implementation of some performant devices always do
> not generate
> partially checksummed packets, but the standard driver still need
> to clear
> VIRTIO_NET_F_GUEST_CSUM when XDP is there.
> A new feature VIRTIO_NET_F_GUEST_FULLY_CSUM is added to solve the
> above
> situation, which provides the driver with configurable offload.
> If the offload is enabled, then the device must deliver fully
> checksummed packets to the driver and may validate the checksum.
>
> Use case example:
> If VIRTIO_NET_F_GUEST_FULLY_CSUM is negotiated and the offload is
> enabled,
> after XDP processes a fully checksummed packet, the
> VIRTIO_NET_HDR_F_DATA_VALID bit
> is retained if the device has validated its
[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements
Cornelia Huck writes: > On Wed, Dec 06 2023, Viresh Kumar wrote: > >> On 05-12-23, 14:18, Cornelia Huck wrote: >>> On Tue, Dec 05 2023, Viresh Kumar wrote: >> >> - The transport MUST provide a mechanism for the device and the driver >> to notify each other, for example about availability of a buffer on >> the virtqueue. > > Probably a mechanism for the device to notify the driver, and a > mechanism for the driver to notify the device? They can be different, as > long both of them are present. > >> >> - The transport SHOULD provide a mechanism for the driver to initiate >> a reset of the virtqueues and device. > > Can we mandate a mechanism for a device reset? Reset of virtqueues needs > to be optional, I'm not sure whether a SHOULD would be appropriate for > that (or maybe we should finally come up with something for ccw ;) > > Other things that probably should be mandatory: > > - A way for the driver to change the device status. Reading it would > probably be a strong SHOULD. > - A way to implement config space. (For example, channel devices don't > have a config space or anything similar enough to repurpose, so I had > to invent a mechanism for ccw... maybe future transports will be in > the same boat.) I think Bill has run into problems with config space on OpenAMP setups where there can be no specific trapping between domains making it hard to manage a config space where both sides might want to make updates. I guess the original MMIO transport gets away with it because config space is always in the trap-and-emulate address space in a normal VM/emulation situation. Bill, Is the plan to introduce a new transport that manages config in a different way? -- Alex Bennée Virtualisation Tech Lead @ Linaro - 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: [PATCH] virtio-transport: Clarify requirements
Cornelia Huck writes:
> On Tue, Dec 05 2023, Alex Bennée wrote:
>
>> Cornelia Huck writes:
>>
>>> On Tue, Dec 05 2023, Viresh Kumar wrote:
+
+The device MUST present each event, in a transport defined way, from the
+moment it takes place until the driver acknowledges the event.
>>>
+
+\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio
Transport Options}
+
+The driver MUST NOT access guest memory locations outside what's made
+available by the device to the driver.
>>>
>>> I don't think that makes sense -- I'd assume most guest memory locations
>>> do not have anything to do with virtio, and we should try to avoid
>>> host/guest terminology.
>>
>> I agree guest memory isn't the right terminology here. However there are
>> discussions about how to implement secure buffers for VirtIO - so for
>> example a buffer mediated by some sort of secure layer. In those cases
>> the driver may not have access to it outside of the transactions.
>
> Yes, I think we need to limit the scope of "guest memory" here. I think
> we are basically wanting to deal with any memory used by virtio (device
> type including memory access controlled by it, transport, and the
> protocol itself). We would be talking about memory made available to the
> device by the driver for explicit usage to implement the virtio spec. I
> think this would cover mediation by a secure layer as well (with the
> driver calling into that secure layer?) Or does the (host) device end up
> donating memory to the (guest) driver, and we need to make sure it
> doesn't scribble over it?
I'm not sure if we have example of the host donating memory apart from
the sort of static partitioning we see with guests on start-up where a
region is defined as shared. The Xen grant model leaves the guest to
grant access to its own pages to the backend.
I guess for firmware mediated sharing this would still be driven by the
guest rather than the host?
>
+
+The driver MUST NOT access virtqueue contents before the device notifies
+about the readiness of the same.
+
+The driver MUST NOT access buffers, after it has added them to the
+virtqueue and notified the device about their availability. The driver
+MAY access them after the device has processed them and notified the
+driver of their availability, in a transport defined way.
+
+The driver MAY ask the device to reset the virtqueues if, for example,
+the driver times out waiting for a notification from the device for a
+previously queued request.
>>>
>>> Again, I believe this has already been covered in the generic
>>> sections -- do we instead need to specify that a transport MUST provide
>>> a method to do xy? (or SHOULD, MAY, as applicable -- it would be good to
>>> list explicitly what is mandatory for a transport to implement, and what
>>> is optional.)
>>
>> Yes I think so. The s390x channel transport gets referenced because it
>> has a nice enumerated list of operations. It would be good to codify
>> which operations are mandatory for all transports and which are
>> optional.
>
> The problem with the ccw transport is that while it has a nice list of
> operations, (a) it only covers guest-initiated actions,
What examples of host initiated actions are there (aside from an IPI
indicating a receive VirtQueue has buffers waiting)?
> (b) probably not
> all of them shold be mandatory (and some of them are more of an artifact
> of how channel I/O works),
These ones?
#define CCW_CMD_SET_IND 0x43
#define CCW_CMD_SET_CONF_IND 0x53
#define CCW_CMD_SET_IND_ADAPTER 0x73
> and (c) it only implements a subset of the
> defined operations (which makes the not-implemented ones de facto
> optional, of course :) But yes, we could use it as a starting point.
Got to start somewhere :-)
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
-
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: [PATCH] virtio-transport: Clarify requirements
On Wed, Dec 06 2023, Viresh Kumar wrote:
> On 05-12-23, 14:18, Cornelia Huck wrote:
>> On Tue, Dec 05 2023, Viresh Kumar wrote:
>> > +\section{Virtio Transport Requirements}\label{sec:Virtio Transport
>> > Options / Virtio Transport Requirements}
>> > +
>> > +\devicenormative{\subsection}{Virtio Transport Requirements}{Virtio
>> > Transport Options}
>>
>> I'm not sure we can introduce MUST (NOT) requirements for basic
>> functionality after the spec has been published for quite a time already
>> (although I'd assume every implementation is fulfilling the requirements
>> anyway)... thoughts?
>
> Fair point. I think it would be okay to use MUST (NOT) if we are sure
> that the existing implementations are fulfilling the requirements.
> Else maybe we can use SHOULD (NOT) ?
Yes, I think if we are just spelling out things explicitly in one place
that have been required anyway (by existing transports etc.), we are
fine using MUST; everything where someone could have chosen a different
implementation needs to be SHOULD.
(...)
>> > +\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio
>> > Transport Options}
>> > +
>> > +The driver MUST NOT access guest memory locations outside what's made
>> > +available by the device to the driver.
>>
>> I don't think that makes sense -- I'd assume most guest memory locations
>> do not have anything to do with virtio, and we should try to avoid
>> host/guest terminology.
>
> Hmm, you are right about the guest/host terminology. This comes mostly
> from the MMIO transport section, where a guest presents 0x100 bytes of
> memory, followed by configuration space (whose length is device/driver
> dependent). The MMIO section has this:
>
> "The driver MUST NOT access memory locations not described in the
> table \ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device
> Register Layout}
> (or, in case of the configuration space, described in the device
> specification),
> MUST NOT write to the read-only registers (direction R) and
> MUST NOT read from the write-only registers (direction W)."
Thanks for clarifying where this came from! See my suggestions in the
other fork of this thread.
>
>> > +The driver MUST NOT write to the read-only memory area and MUST NOT read
>> > +from the write-only memory area.
>>
>> Which memory areas does that refer to? Parts of the transport-specific
>> data structures?
>
> Yes, based again on the above MMIO section, but perhaps apply to other
> transports as well..
I think we can't assume transports to use r/o and w/o memory areas in
all cases -- for example, ccw is largely based on passing buffers
around. Of course, if you look at the actual structures, there are
always fields that are to be used for reading or writing only.
>
>> > +The driver MUST acknowledge events presented by the device, as mandated
>> > +by the transport.
>>
>> I don't think this is quite correct in the absolute -- for example, it
>> should be fine to not acknowledge events if some overriding event comes
>> along, or if the driver initiates a reset.
>
> What about:
>
> The driver SHOULD acknowledge events presented by the device, as
> mandated by the transport, unless a new event from the device
> overrides the previous one or the driver initiates a reset.
Should we do s/events/notifications/, or do we also cover transactions
initiated by the device explicitly (I'd assume that the device would
notify the driver im- or explicitly if it has to do something?)
>
>> > +The driver MUST NOT access virtqueue contents before the device notifies
>> > +about the readiness of the same.
>> > +
>> > +The driver MUST NOT access buffers, after it has added them to the
>> > +virtqueue and notified the device about their availability. The driver
>> > +MAY access them after the device has processed them and notified the
>> > +driver of their availability, in a transport defined way.
>> > +
>> > +The driver MAY ask the device to reset the virtqueues if, for example,
>> > +the driver times out waiting for a notification from the device for a
>> > +previously queued request.
>>
>> Again, I believe this has already been covered in the generic
>> sections
>
> Right, but having it all at a single place makes it more convenient,
> instead of looking at implementations.
In that case, I'm fine for the first two; however, I'm not sure what the
third one adds here -- I don't think we ever said anything about when
the driver can initiate a reset, it can basically be at any time, and it
overrides whatever the device did if we reword the statement above.
>
>> do we instead need to specify that a transport MUST provide
>> a method to do xy? (or SHOULD, MAY, as applicable -- it would be good to
>> list explicitly what is mandatory for a transport to implement, and what
>> is optional.)
>
> Hmm, yeah sounds good. How about these ?
>
> - The transport MUST provide a mechanism for device discovery at the
> driver's end.
"The transport MUST provide a mechanism for a
[virtio-dev] Re: [PATCH] virtio-transport: Clarify requirements
On Tue, Dec 05 2023, Alex Bennée wrote:
> Cornelia Huck writes:
>
>> On Tue, Dec 05 2023, Viresh Kumar wrote:
>>> +
>>> +The device MUST present each event, in a transport defined way, from the
>>> +moment it takes place until the driver acknowledges the event.
>>
>> I don't believe "event" is well-defined here.
>
> Maybe:
>
> "A device initiated transaction can isn't considered complete until
> acknowledged by the driver. As such data MUST remain visible to the
> driver until the transaction is complete"?
Transaction is good, what about:
"Any data associated with a device-initiated transaction MUST remain
accessible to the driver until the driver acknowledges the transaction
to be complete."
>
>>
>>> +
>>> +The device MUST NOT access virtqueue's contents before the driver
>>> +notifies that the queue is ready for access, in a transport defined way.
>>> +
>>> +The device MUST NOT access buffers on the virtqueue, after it has
>>> +modified them and notified the driver about their availability.
>>> +
>>> +The device MUST reset the virtqueues if requested by the driver, in a
>>> +transport defined way.
>>
>> Isn't all of this already defined in one place of the spec or another?
>
> I think the recent example is the virtio-sound driver continuing to feed
> data into buffers after those buffers where submitted into the
> virtqueue. We should be explicit that the only time both sides of a
> VirtIO implementation can access things at the same time is with
> explicitly shared memory (and you need some sort of mechanism to mediate
> that to avoid chaos).
Fair enough, let's make it explicit if people already stumbled
here. Some rewording suggestions:
"The device MUST NOT access the contents of a virtqueue before the
driver notifies, in a transport defined way, the device that the
virtqueue is ready to be accessed.
The device MUST NOT access or modify buffers on a virtqueue after it has
notified the driver about their availability.
The device MUST reset the virtqueues if requested, in a transport
defined way, by the driver."
>
>>> +
>>> +\drivernormative{\subsection}{Virtio Transport Requirements}{Virtio
>>> Transport Options}
>>> +
>>> +The driver MUST NOT access guest memory locations outside what's made
>>> +available by the device to the driver.
>>
>> I don't think that makes sense -- I'd assume most guest memory locations
>> do not have anything to do with virtio, and we should try to avoid
>> host/guest terminology.
>
> I agree guest memory isn't the right terminology here. However there are
> discussions about how to implement secure buffers for VirtIO - so for
> example a buffer mediated by some sort of secure layer. In those cases
> the driver may not have access to it outside of the transactions.
Yes, I think we need to limit the scope of "guest memory" here. I think
we are basically wanting to deal with any memory used by virtio (device
type including memory access controlled by it, transport, and the
protocol itself). We would be talking about memory made available to the
device by the driver for explicit usage to implement the virtio spec. I
think this would cover mediation by a secure layer as well (with the
driver calling into that secure layer?) Or does the (host) device end up
donating memory to the (guest) driver, and we need to make sure it
doesn't scribble over it?
>>> +
>>> +The driver MUST NOT access virtqueue contents before the device notifies
>>> +about the readiness of the same.
>>> +
>>> +The driver MUST NOT access buffers, after it has added them to the
>>> +virtqueue and notified the device about their availability. The driver
>>> +MAY access them after the device has processed them and notified the
>>> +driver of their availability, in a transport defined way.
>>> +
>>> +The driver MAY ask the device to reset the virtqueues if, for example,
>>> +the driver times out waiting for a notification from the device for a
>>> +previously queued request.
>>
>> Again, I believe this has already been covered in the generic
>> sections -- do we instead need to specify that a transport MUST provide
>> a method to do xy? (or SHOULD, MAY, as applicable -- it would be good to
>> list explicitly what is mandatory for a transport to implement, and what
>> is optional.)
>
> Yes I think so. The s390x channel transport gets referenced because it
> has a nice enumerated list of operations. It would be good to codify
> which operations are mandatory for all transports and which are
> optional.
The problem with the ccw transport is that while it has a nice list of
operations, (a) it only covers guest-initiated actions, (b) probably not
all of them shold be mandatory (and some of them are more of an artifact
of how channel I/O works), and (c) it only implements a subset of the
defined operations (which makes the not-implemented ones de facto
optional, of course :) But yes, we could use it as a starting point.
-
To
Re: [virtio-dev][PATCH V9 2/2] virtio-spi: add the device specification
On Tue, Dec 12 2023, Haixu Cui wrote:
[BTW: A changelog would be useful, either in the patch or in the cover
letter.]
> The Virtio SPI (Serial Peripheral Interface) device is a virtual
> SPI controller that allows the driver to operate and use the SPI
> controller under the control of the host.
>
> This patch adds the specification for virtio-spi.
>
> Signed-off-by: Haixu Cui
> Reviewed-by: Viresh Kumar
> ---
> device-types/spi/description.tex| 281
> device-types/spi/device-conformance.tex | 7 +
> device-types/spi/driver-conformance.tex | 7 +
> 3 files changed, 295 insertions(+)
> create mode 100644 device-types/spi/description.tex
> create mode 100644 device-types/spi/device-conformance.tex
> create mode 100644 device-types/spi/driver-conformance.tex
>
> diff --git a/device-types/spi/description.tex
> b/device-types/spi/description.tex
Please move inclusion of this new file into content.tex here, instead of
including a not-yet-existing file in the previous patch.
(...)
> +\field{mode_func_supported} indicates whether the following features are
> supported or not:
> +bit 0-1: CPHA feature,
> +0b00: invalid, must support as least one CPHA setting.
> +0b01: supports CPHA=0 only;
> +0b10: supports CPHA=1 only;
> +0b11: supports CPHA=0 and CPHA=1;
> +
> +bit 2-3: CPOL feature,
> +0b00: invalid, must support as least one CPOL setting.
> +0b01: supports CPOL=0 only;
> +0b10: supports CPOL=1 only;
> +0b11: supports CPOL=0 and CPOL=1;
> +
> +bit 4: chipselect active high feature, 0 for unsupported and 1 for
> supported,
> +chipselect active low must always be supported.
> +
> +bit 5: LSB first feature, 0 for unsupported and 1 for supported, MSB
> first must always be
> +supported.
> +
> +bit 6: loopback mode feature, 0 for unsupported and 1 for supported,
> normal mode
> +must always be supported.
> +
> +Note: CPOL is clock polarity and CPHA is clock phase. If CPOL is 0, the
> clock idles at the logical
> +low voltage, otherwise it idles at the logical high voltage. CPHA determines
> how data is outputted
> +and sampled.
Shouldn't you also specify what CPHA==0 and CPHA==1 mean?
(...)
> +VIRTIO_SPI_TRANS_ERR indicates a transfer error, which means that the
> parameters are all
> +valid but the trasnfer process failed.
s/trasnfer/transfer/
LGTM otherwise. Does anybody else have comments?
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
