Call for Workshop Proposals - WorldCIST'23 | Pisa, Italy

[WorldCIST-2023]

* Google Scholar H-Index = 25

* Indexed in WoS, Scopus, DBLP, etc.



 CALL for WORKSHOPS 


WorldCIST'23 - 11st World Conference on Information Systems and Technologies

Pisa, Italy, 4 - 6 April 2023

http://worldcist.org/ 






SCOPE

The Information Systems and Technologies research and industrial community is 
invited to submit proposals for the organization of Workshops at WorldCist'23 - 
11th World Conference on Information Systems and Technologies, to be held at 
Pisa, Italy, 4-6 April 2023. WorldCist is a global forum for researchers and 
practitioners to present and discuss the most recent innovations, trends, 
results, experiences and concerns in the several perspectives of Information 
Systems and Technologies.


WORKSHOP FORMAT

Workshops should focus on a specific scientific subject on the scope of 
WorldCist'23 but not directly included on the main conference areas. Each 
workshop will be coordinated by an Organizing Committee composed of, at least, 
two researchers in the field, preferably from different institutions and 
different countries. The organizers should create an international Program 
Committee for the Workshop, with recognized researchers within the specific 
Workshop scientific area. Each workshop should have at least ten submissions 
and five accepted papers to be conducted at WorldCist'23.

The selection of Workshops will be performed by WorldCist'23 
Conference/Workshop Chairs. Each Workshop will have 1 article offered for 10 
articles with paid registration, 2 articles offered for 20 articles with paid 
registration, and 3 articles offered for 40 articles with paid registration.

Workshops full and short papers will be published in the conference main 
proceedings in specific Workshop chapters published by Springer in a book of 
the LNNS series. Proceedings will be submitted for indexation by WoS, SCOPUS, 
DBLP, Google Scholar, among several other scientific databases. Extended 
versions of best selected papers will be published in journals indexed by 
WoS/SCI, SCOPUS and DBLP. Detailed and up-to-date information may be found at 
WorldCist'23 website: http://worldcist.org/ 



WORKSHOP ORGANIZATION

The Organizing Committee of each Workshop will be responsible for:

- Producing and distributing the Workshop Call for Papers (CFP);
- Coordinating the review and selection process for the papers submitted to the 
Workshop, as Workshop chairs (on the paper submission system to be installed);
- Delivering the final versions of the papers accepted for the Workshop in 
accordance with the guidelines and deadlines defined by WorldCist'23 organizers;
- Coordinating and chairing the Workshop sessions at the conference.

WorldCist'23 organizers reserve the right to cancel any Workshop if deadlines 
are missed or if the number of registered attendees is too low to support the 
costs associated with the Workshop.


PROPOSAL CONTENT

Workshop proposals should contain the following information:

- Workshop title;
- Brief description of the specific scientific scope of the Workshop;
- List of topics of interest (max 15 topics);
- Reasons the Workshop should be held within WorldCist’23;
- Name, postal address, phone and email of all the members of the Workshop 
Organizing Committee;
- Preliminary proposal for the Workshop Program Committee (Names and 
affiliations).

Proposals should be submitted at 
https://easychair.org/conferences/?conf=worldcistworkshops2023 
 in PDF (in English), by September 
4, 2022.


IMPORTANT DATES

- Deadline for Workshop proposals: September 10, 2022
- Notification of Workshop acceptance: September 15, 2022
- Workshop Final Information and Program Committee: September 25, 2022
- Deadline for paper submission: November 20, 2022
- Notification of paper acceptance: December 23, 2022
- Deadline for final versions and conference registration: January 4, 2023
- Conference dates: 4-6 April 2023


CHAIR

Fernando Moreira, Universidade Portucalense, Portugal



WorldCIST'23 Website: http://worldcist.org/ 





--
This email has been checked for viruses by AVG.
https://www.avg.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Si-Wei Liu]

On 7/28/2022 4:35 AM, Michael S. Tsirkin wrote:

On Thu, Jul 28, 2022 at 12:08:53AM -0700, Si-Wei Liu wrote:


On 7/27/2022 7:06 PM, Jason Wang wrote:

在 2022/7/28 08:56, Si-Wei Liu 写道:


On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:


On 7/27/2022 5:43 PM, Si-Wei Liu wrote:

Sorry to chime in late in the game. For some reason I
couldn't get to most emails for this discussion (I only
subscribed to the virtualization list), while I was taking
off amongst the past few weeks.

It looks to me this patch is incomplete. Noted down the way
in vdpa_dev_net_config_fill(), we have the following:
  features = vdev->config->get_driver_features(vdev);
  if (nla_put_u64_64bit(msg,
VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
    VDPA_ATTR_PAD))
  return -EMSGSIZE;

Making call to .get_driver_features() doesn't make sense
when feature negotiation isn't complete. Neither should
present negotiated_features to userspace before negotiation
is done.

Similarly, max_vqp through vdpa_dev_net_mq_config_fill()
probably should not show before negotiation is done - it
depends on driver features negotiated.

I have another patch in this series introduces device_features
and will report device_features to the userspace even features
negotiation not done. Because the spec says we should allow
driver access the config space before FEATURES_OK.

The config space can be accessed by guest before features_ok doesn't
necessarily mean the value is valid.


It's valid as long as the device offers the feature:

"The device MUST allow reading of any device-specific configuration
field before FEATURES_OK is set by the driver. This includes fields
which are conditional on feature bits, as long as those feature bits are
offered by the device."

I guess this statement only conveys that the field in config space can be
read before FEATURES_OK is set, though it does not *explicitly* states the
validity of field.

And looking at:

"The mac address field always exists (though is only valid if
VIRTIO_NET_F_MAC is set), and status only exists if VIRTIO_NET_F_STATUS is
set."

It appears to me there's a border line set between "exist" and "valid". If I
understand the spec wording correctly, a spec-conforming device
implementation may or may not offer valid status value in the config space
when VIRTIO_NET_F_STATUS is offered, but before the feature is negotiated.
On the other hand, config space should contain valid mac address the moment
VIRTIO_NET_F_MAC feature is offered, regardless being negotiated or not. By
that, there seems to be leeway for the device implementation to decide when
config space field may become valid, though for most of QEMU's software
virtio devices, valid value is present to config space the very first moment
when feature is offered.

"If the VIRTIO_NET_F_MAC feature bit is set, the configuration space mac
entry indicates the “physical” address of the network card, otherwise the
driver would typically generate a random local MAC address."
"If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status comes
from the bottom bit of status. Otherwise, the driver assumes it’s active."

And also there are special cases where the read of specific configuration
space field MUST be deferred to until FEATURES_OK is set:

"If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode can be
read or set through the writeback field. 0 corresponds to a writethrough
cache, 1 to a writeback cache11. The cache mode after reset can be either
writeback or writethrough. The actual mode can be determined by reading
writeback after feature negotiation."
"The driver MUST NOT read writeback before setting the FEATURES_OK device
status bit."
"If VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH is not, the
device MUST initialize writeback to 0."

Since the spec doesn't explicitly mandate the validity of each config space
field when feature of concern is offered, to be safer we'd have to live with
odd device implementation. I know for sure QEMU software devices won't for
99% of these cases, but that's not what is currently defined in the spec.


Thanks for raising this subject. I started working on this in April:

https://urldefense.com/v3/__https://lists.oasis-open.org/archives/virtio-comment/202201/msg00068.html__;!!ACWV5N9M2RV99hQ!Os6QE_RJokx7Us9y7-5-ByVVLuy3oLuPodAdZWxwJw_aNkJY0o0H7691FI9aYSTRLVieASUD_eOu$

working now to address the comments.

Great, thank you very much!

Is there a link to the latest draft that reflects the change uptodate? 
The one above with iterative feature negotiation seemed getting some 
resistance because of backward incompatibility with older spec? Please 
copy me in the loop when you have the draft ready. I am not in the 
virtio-comment list.


Thanks,
-Siwei






You may want to double check with Michael for what he quoted earlier:

Nope:

2.5.1  Driver Requirements: Device Configuration Space

...

For optional configuration space fields, t

Re: [PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Si-Wei Liu]

On 7/27/2022 7:44 PM, Zhu, Lingshan wrote:



On 7/28/2022 9:41 AM, Si-Wei Liu wrote:



On 7/27/2022 4:54 AM, Zhu, Lingshan wrote:



On 7/27/2022 6:09 PM, Si-Wei Liu wrote:



On 7/27/2022 2:01 AM, Michael S. Tsirkin wrote:

On Wed, Jul 27, 2022 at 12:50:33AM -0700, Si-Wei Liu wrote:


On 7/26/2022 11:01 PM, Michael S. Tsirkin wrote:

On Wed, Jul 27, 2022 at 03:47:35AM +, Parav Pandit wrote:

From: Zhu, Lingshan 
Sent: Tuesday, July 26, 2022 10:53 PM

On 7/27/2022 10:17 AM, Parav Pandit wrote:

From: Zhu, Lingshan 
Sent: Tuesday, July 26, 2022 10:15 PM

On 7/26/2022 11:56 PM, Parav Pandit wrote:

From: Zhu, Lingshan 
Sent: Tuesday, July 12, 2022 11:46 PM
When the user space which invokes netlink commands, 
detects that

_MQ
is not supported, hence it takes max_queue_pair = 1 by 
itself.
I think the kernel module have all necessary information 
and it is
the only one which have precise information of a device, 
so it
should answer precisely than let the user space guess. The 
kernel
module should be reliable than stay silent, leave the 
question to

the user space

tool.
Kernel is reliable. It doesn’t expose a config space field 
if the

field doesn’t

exist regardless of field should have default or no default.
so when you know it is one queue pair, you should answer 
one, not try

to guess.
User space should not guess either. User space gets to see 
if _MQ
present/not present. If _MQ present than get reliable data 
from kernel.

If _MQ not present, it means this device has one VQ pair.
it is still a guess, right? And all user space tools 
implemented this

feature need to guess

No. it is not a guess.
It is explicitly checking the _MQ feature and deriving the 
value.

The code you proposed will be present in the user space.
It will be uniform for _MQ and 10 other features that are 
present now and

in the future.
MQ and other features like RSS are different. If there is no 
_RSS_XX, there
are no attributes like max_rss_key_size, and there is not a 
default value.

But for MQ, we know it has to be 1 wihtout _MQ.

"we" = user space.
To keep the consistency among all the config space fields.

Actually I looked and the code some more and I'm puzzled:


struct virtio_net_config config = {};
u64 features;
u16 val_u16;

vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));

if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, 
sizeof(config.mac),

    config.mac))
    return -EMSGSIZE;


Mac returned even without VIRTIO_NET_F_MAC


val_u16 = le16_to_cpu(config.status);
if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
    return -EMSGSIZE;


status returned even without VIRTIO_NET_F_STATUS

val_u16 = le16_to_cpu(config.mtu);
if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
    return -EMSGSIZE;


MTU returned even without VIRTIO_NET_F_MTU


What's going on here?


I guess this is spec thing (historical debt), I vaguely recall 
these fields
are always present in config space regardless the existence of 
corresponding

feature bit.

-Siwei

Nope:

2.5.1  Driver Requirements: Device Configuration Space

...

For optional configuration space fields, the driver MUST check 
that the corresponding feature is offered

before accessing that part of the configuration space.
Well, this is driver side of requirement. As this interface is for 
host admin tool to query or configure vdpa device, we don't have to 
wait until feature negotiation is done on guest driver to extract 
vdpa attributes/parameters, say if we want to replicate another 
vdpa device with the same config on migration destination. I think 
what may need to be fix is to move off from using 
.vdpa_get_config_unlocked() which depends on feature negotiation. 
And/or expose config space register values through another set of 
attributes.
Yes, we don't have to wait for FEATURES_OK. In another patch in this 
series, I have added a new netlink attr to report the device 
features, and removed the blocker. So the LM orchestration SW can 
query the device features of the devices at the destination cluster, 
and pick a proper one, even mask out some features to meet the LM 
requirements.
For that end, you'd need to move off from using 
vdpa_get_config_unlocked() which depends on feature negotiation. 
Since this would slightly change the original semantics of each field 
that "vdpa dev config" shows, it probably need another netlink 
command and new uAPI.
why not show both device_features and driver_features in "vdpa dev 
config show"?


As I requested in the other email, I'd like to see the proposed 'vdpa 
dev config ...' example output for various phases in feature 
negotiation, and the specific use case (motivation) for this proposed 
output. I am having difficulty to match what you want to do with the 
patch posted.


-Siwei



-Siwei




Thanks,
Zhu Lingshan

-Siwei












___
Virtualization mailing list
Virtualization@lists.linux-found

Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Si-Wei Liu]

On 7/27/2022 7:43 PM, Zhu, Lingshan wrote:



On 7/28/2022 8:56 AM, Si-Wei Liu wrote:



On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:



On 7/27/2022 5:43 PM, Si-Wei Liu wrote:
Sorry to chime in late in the game. For some reason I couldn't get 
to most emails for this discussion (I only subscribed to the 
virtualization list), while I was taking off amongst the past few 
weeks.


It looks to me this patch is incomplete. Noted down the way in 
vdpa_dev_net_config_fill(), we have the following:

 features = vdev->config->get_driver_features(vdev);
 if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
   VDPA_ATTR_PAD))
 return -EMSGSIZE;

Making call to .get_driver_features() doesn't make sense when 
feature negotiation isn't complete. Neither should present 
negotiated_features to userspace before negotiation is done.


Similarly, max_vqp through vdpa_dev_net_mq_config_fill() probably 
should not show before negotiation is done - it depends on driver 
features negotiated.
I have another patch in this series introduces device_features and 
will report device_features to the userspace even features 
negotiation not done. Because the spec says we should allow driver 
access the config space before FEATURES_OK.
The config space can be accessed by guest before features_ok doesn't 
necessarily mean the value is valid. You may want to double check 
with Michael for what he quoted earlier:
that's why I proposed to fix these issues, e.g., if no _F_MAC, vDPA 
kernel should not return a mac to the userspace, there is not a 
default value for mac.
Then please show us the code, as I can only comment based on your latest 
(v4) patch and it was not there.. To be honest, I don't understand the 
motivation and the use cases you have, is it for debugging/monitoring or 
there's really a use case for live migration? For the former, you can do 
a direct dump on all config space fields regardless of endianess and 
feature negotiation without having to worry about validity (meaningful 
to present to admin user). To me these are conflict asks that is 
impossible to mix in exact one command.



Nope:

2.5.1  Driver Requirements: Device Configuration Space

...

For optional configuration space fields, the driver MUST check that the 
corresponding feature is offered
before accessing that part of the configuration space.


and how many driver bugs taking wrong assumption of the validity of 
config space field without features_ok. I am not sure what use case 
you want to expose config resister values for before features_ok, if 
it's mostly for live migration I guess it's probably heading a wrong 
direction.






Last but not the least, this "vdpa dev config" command was not 
designed to display the real config space register values in the 
first place. Quoting the vdpa-dev(8) man page:


vdpa dev config show - Show configuration of specific device or 
all devices.
DEV - specifies the vdpa device to show its configuration. If this 
argument is omitted all devices configuration is listed.
It doesn't say anything about configuration space or register 
values in config space. As long as it can convey the config 
attribute when instantiating vDPA device instance, and more 
importantly, the config can be easily imported from or exported to 
userspace tools when trying to reconstruct vdpa instance intact on 
destination host for live migration, IMHO in my personal 
interpretation it doesn't matter what the config space may present. 
It may be worth while adding a new debug command to expose the real 
register value, but that's another story.
I am not sure getting your points. vDPA now reports device feature 
bits(device_features) and negotiated feature bits(driver_features), 
and yes, the drivers features can be a subset of the device 
features; and the vDPA device features can be a subset of the 
management device features.
What I said is after unblocking the conditional check, you'd have to 
handle the case for each of the vdpa attribute when feature 
negotiation is not yet done: basically the register values you got 
from config space via the vdpa_get_config_unlocked() call is not 
considered to be valid before features_ok (per-spec). Although in 
some case you may get sane value, such behavior is generally 
undefined. If you desire to show just the device_features alone 
without any config space field, which the device had advertised 
*before feature negotiation is complete*, that'll be fine. But looks 
to me this is not how patch has been implemented. Probably need some 
more work?
They are driver_features(negotiated) and the device_features(which 
comes with the device), and the config space fields that depend on 
them. In this series, we report both to the userspace.
I fail to understand what you want to present from your description. May 
be worth showing some example outputs that at least include the 
following cases: 1) when device offers features but 

Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Michael S. Tsirkin]

On Thu, Jul 28, 2022 at 12:08:53AM -0700, Si-Wei Liu wrote:
> 
> 
> On 7/27/2022 7:06 PM, Jason Wang wrote:
> > 
> > 在 2022/7/28 08:56, Si-Wei Liu 写道:
> > > 
> > > 
> > > On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:
> > > > 
> > > > 
> > > > On 7/27/2022 5:43 PM, Si-Wei Liu wrote:
> > > > > Sorry to chime in late in the game. For some reason I
> > > > > couldn't get to most emails for this discussion (I only
> > > > > subscribed to the virtualization list), while I was taking
> > > > > off amongst the past few weeks.
> > > > > 
> > > > > It looks to me this patch is incomplete. Noted down the way
> > > > > in vdpa_dev_net_config_fill(), we have the following:
> > > > >  features = vdev->config->get_driver_features(vdev);
> > > > >  if (nla_put_u64_64bit(msg,
> > > > > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> > > > >    VDPA_ATTR_PAD))
> > > > >  return -EMSGSIZE;
> > > > > 
> > > > > Making call to .get_driver_features() doesn't make sense
> > > > > when feature negotiation isn't complete. Neither should
> > > > > present negotiated_features to userspace before negotiation
> > > > > is done.
> > > > > 
> > > > > Similarly, max_vqp through vdpa_dev_net_mq_config_fill()
> > > > > probably should not show before negotiation is done - it
> > > > > depends on driver features negotiated.
> > > > I have another patch in this series introduces device_features
> > > > and will report device_features to the userspace even features
> > > > negotiation not done. Because the spec says we should allow
> > > > driver access the config space before FEATURES_OK.
> > > The config space can be accessed by guest before features_ok doesn't
> > > necessarily mean the value is valid.
> > 
> > 
> > It's valid as long as the device offers the feature:
> > 
> > "The device MUST allow reading of any device-specific configuration
> > field before FEATURES_OK is set by the driver. This includes fields
> > which are conditional on feature bits, as long as those feature bits are
> > offered by the device."
> I guess this statement only conveys that the field in config space can be
> read before FEATURES_OK is set, though it does not *explicitly* states the
> validity of field.
> 
> And looking at:
> 
> "The mac address field always exists (though is only valid if
> VIRTIO_NET_F_MAC is set), and status only exists if VIRTIO_NET_F_STATUS is
> set."
> 
> It appears to me there's a border line set between "exist" and "valid". If I
> understand the spec wording correctly, a spec-conforming device
> implementation may or may not offer valid status value in the config space
> when VIRTIO_NET_F_STATUS is offered, but before the feature is negotiated.
> On the other hand, config space should contain valid mac address the moment
> VIRTIO_NET_F_MAC feature is offered, regardless being negotiated or not. By
> that, there seems to be leeway for the device implementation to decide when
> config space field may become valid, though for most of QEMU's software
> virtio devices, valid value is present to config space the very first moment
> when feature is offered.
> 
> "If the VIRTIO_NET_F_MAC feature bit is set, the configuration space mac
> entry indicates the “physical” address of the network card, otherwise the
> driver would typically generate a random local MAC address."
> "If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status comes
> from the bottom bit of status. Otherwise, the driver assumes it’s active."
> 
> And also there are special cases where the read of specific configuration
> space field MUST be deferred to until FEATURES_OK is set:
> 
> "If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode can be
> read or set through the writeback field. 0 corresponds to a writethrough
> cache, 1 to a writeback cache11. The cache mode after reset can be either
> writeback or writethrough. The actual mode can be determined by reading
> writeback after feature negotiation."
> "The driver MUST NOT read writeback before setting the FEATURES_OK device
> status bit."
> "If VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH is not, the
> device MUST initialize writeback to 0."
> 
> Since the spec doesn't explicitly mandate the validity of each config space
> field when feature of concern is offered, to be safer we'd have to live with
> odd device implementation. I know for sure QEMU software devices won't for
> 99% of these cases, but that's not what is currently defined in the spec.


Thanks for raising this subject. I started working on this in April:

https://lists.oasis-open.org/archives/virtio-comment/202201/msg00068.html

working now to address the comments.


> > 
> > 
> > > You may want to double check with Michael for what he quoted earlier:
> > > > Nope:
> > > > 
> > > > 2.5.1  Driver Requirements: Device Configuration Space
> > > > 
> > > > ...
> > > > 
> > > > For optional configuration space fields, the driver MUST check
> > > > that the c

Re: spec clarification (was Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space)

[Michael S. Tsirkin]

On Thu, Jul 28, 2022 at 01:20:26AM -0700, Si-Wei Liu wrote:
> Hi Michael,
> 
> Could you please comment on the different wording between "exist" and "valid"
> in spec? Having seen quite a few relevant discussions regarding MTU validation
> and VERSION_1 negotiation on s390, I was in the impression this is not the
> first time people getting confused because of ambiguity of random wording
> without detailed example helping to clarify around the context, or due lack of
> clear definition set ahead. I like your idea to keep things consistent
> (conditional depending on feature presence), however, without proper
> interpretation of how spec is supposed to work, we are on a slippery slope
> towards inconsistency.
> 
> On 7/28/2022 12:36 AM, Jason Wang wrote:
> 
> And looking at:
> 
> "The mac address field always exists (though is only valid if
> VIRTIO_NET_F_MAC is set), and status only exists if 
> VIRTIO_NET_F_STATUS
> is set."
> 
> It appears to me there's a border line set between "exist" and 
> "valid".
> If I understand the spec wording correctly, a spec-conforming device
> implementation may or may not offer valid status value in the config
> space when VIRTIO_NET_F_STATUS is offered, but before the feature is
> negotiated.
> 
> That's not what I read, maybe Michael can clarify this.
> 
> 
> 
> And Jason and I find below normatives are conflict with each other.
> 
> "The device MUST allow reading of any device-specific configuration
> field before FEATURES_OK is set by the driver. This includes fields
> which are conditional on feature bits, as long as those feature bits 
> are
> offered by the device."


So I proposed this back in April:

https://lists.oasis-open.org/archives/virtio-comment/202201/msg00068.html

I intended this for 1.2 but it quickly became clear it won't make it
in time. Working on reviving the proposal and addressing the comments.




> 
> ...
> 
> And also there are special cases where the read of specific
> configuration space field MUST be deferred to until FEATURES_OK is 
> set:
> 
> "If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode
> can be read or set through the writeback field. 0 corresponds to a
> writethrough cache, 1 to a writeback cache11. The cache mode after 
> reset
> can be either writeback or writethrough. The actual mode can be
> determined by reading writeback after feature negotiation."
> "The driver MUST NOT read writeback before setting the FEATURES_OK
> device status bit."
> 
> This seems to conflict with the normatives I quoted above, and I don't
> get why we need this.
> 
> 
> 
> Thanks,
> -Siwei


The last one I take to mean writeback is special.
I am not sure why it should be. Paolo you proposed this text could
you comment please?

Thanks!

-- 
MST

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v13 16/42] virtio_ring: split: introduce virtqueue_resize_split()

[Xuan Zhuo]

On Thu, 28 Jul 2022 17:04:36 +0800, Jason Wang  wrote:
> On Thu, Jul 28, 2022 at 4:18 PM Xuan Zhuo  wrote:
> >
> > On Thu, 28 Jul 2022 15:42:50 +0800, Jason Wang  wrote:
> > > On Thu, Jul 28, 2022 at 3:24 PM Xuan Zhuo  
> > > wrote:
> > > >
> > > > On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang  
> > > > wrote:
> > > > > On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo 
> > > > >  wrote:
> > > > > >
> > > > > > On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang 
> > > > > >  wrote:
> > > > > > >
> > > > > > > 在 2022/7/26 15:21, Xuan Zhuo 写道:
> > > > > > > > virtio ring split supports resize.
> > > > > > > >
> > > > > > > > Only after the new vring is successfully allocated based on the 
> > > > > > > > new num,
> > > > > > > > we will release the old vring. In any case, an error is 
> > > > > > > > returned,
> > > > > > > > indicating that the vring still points to the old vring.
> > > > > > > >
> > > > > > > > In the case of an error, 
> > > > > > > > re-initialize(virtqueue_reinit_split()) the
> > > > > > > > virtqueue to ensure that the vring can be used.
> > > > > > > >
> > > > > > > > Signed-off-by: Xuan Zhuo 
> > > > > > > > Acked-by: Jason Wang 
> > > > > > > > ---
> > > > > > > >   drivers/virtio/virtio_ring.c | 34 
> > > > > > > > ++
> > > > > > > >   1 file changed, 34 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_ring.c 
> > > > > > > > b/drivers/virtio/virtio_ring.c
> > > > > > > > index b6fda91c8059..58355e1ac7d7 100644
> > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > @@ -220,6 +220,7 @@ static struct virtqueue 
> > > > > > > > *__vring_new_virtqueue(unsigned int index,
> > > > > > > >void 
> > > > > > > > (*callback)(struct virtqueue *),
> > > > > > > >const char *name);
> > > > > > > >   static struct vring_desc_extra 
> > > > > > > > *vring_alloc_desc_extra(unsigned int num);
> > > > > > > > +static void vring_free(struct virtqueue *_vq);
> > > > > > > >
> > > > > > > >   /*
> > > > > > > >* Helpers.
> > > > > > > > @@ -1117,6 +1118,39 @@ static struct virtqueue 
> > > > > > > > *vring_create_virtqueue_split(
> > > > > > > > return vq;
> > > > > > > >   }
> > > > > > > >
> > > > > > > > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 
> > > > > > > > num)
> > > > > > > > +{
> > > > > > > > +   struct vring_virtqueue_split vring_split = {};
> > > > > > > > +   struct vring_virtqueue *vq = to_vvq(_vq);
> > > > > > > > +   struct virtio_device *vdev = _vq->vdev;
> > > > > > > > +   int err;
> > > > > > > > +
> > > > > > > > +   err = vring_alloc_queue_split(&vring_split, vdev, num,
> > > > > > > > + vq->split.vring_align,
> > > > > > > > + vq->split.may_reduce_num);
> > > > > > > > +   if (err)
> > > > > > > > +   goto err;
> > > > > > >
> > > > > > >
> > > > > > > I think we don't need to do anything here?
> > > > > >
> > > > > > Am I missing something?
> > > > >
> > > > > I meant it looks to me most of the virtqueue_reinit() is unnecessary.
> > > > > We probably only need to reinit avail/used idx there.
> > > >
> > > >
> > > > In this function, we can indeed remove some code.
> > > >
> > > > >   static void virtqueue_reinit_split(struct vring_virtqueue *vq)
> > > > >   {
> > > > >   int size, i;
> > > > >
> > > > >   memset(vq->split.vring.desc, 0, 
> > > > > vq->split.queue_size_in_bytes);
> > > > >
> > > > >   size = sizeof(struct vring_desc_state_split) * 
> > > > > vq->split.vring.num;
> > > > >   memset(vq->split.desc_state, 0, size);
> > > > >
> > > > >   size = sizeof(struct vring_desc_extra) * 
> > > > > vq->split.vring.num;
> > > > >   memset(vq->split.desc_extra, 0, size);
> > > >
> > > > These memsets can be removed, and theoretically it will not cause any
> > > > exceptions.
> > >
> > > Yes, otherwise we have bugs in detach_buf().
> > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > >   for (i = 0; i < vq->split.vring.num - 1; i++)
> > > > >   vq->split.desc_extra[i].next = i + 1;
> > > >
> > > > This can also be removed, but we need to record free_head that will 
> > > > been update
> > > > inside virtqueue_init().
> > >
> > > We can simply keep free_head unchanged? Otherwise it's a bug somewhere I 
> > > guess.
> > >
> > >
> > > >
> > > > >
> > > > >   virtqueue_init(vq, vq->split.vring.num);
> > > >
> > > > There are some operations in this, which can also be skipped, such as 
> > > > setting
> > > > use_dma_api. But I think calling this function directly will be more 
> > > > convenient
> > > > for maintenance.
> > >
> > > I don't see anything that is necessary here.
> >
> > These three are currently inside virtqueue_init()
> >
> > vq->last_used_idx = 0;

Re: [PATCH v13 16/42] virtio_ring: split: introduce virtqueue_resize_split()

[Jason Wang]

On Thu, Jul 28, 2022 at 4:18 PM Xuan Zhuo  wrote:
>
> On Thu, 28 Jul 2022 15:42:50 +0800, Jason Wang  wrote:
> > On Thu, Jul 28, 2022 at 3:24 PM Xuan Zhuo  
> > wrote:
> > >
> > > On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang  
> > > wrote:
> > > > On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo  
> > > > wrote:
> > > > >
> > > > > On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang  
> > > > > wrote:
> > > > > >
> > > > > > 在 2022/7/26 15:21, Xuan Zhuo 写道:
> > > > > > > virtio ring split supports resize.
> > > > > > >
> > > > > > > Only after the new vring is successfully allocated based on the 
> > > > > > > new num,
> > > > > > > we will release the old vring. In any case, an error is returned,
> > > > > > > indicating that the vring still points to the old vring.
> > > > > > >
> > > > > > > In the case of an error, re-initialize(virtqueue_reinit_split()) 
> > > > > > > the
> > > > > > > virtqueue to ensure that the vring can be used.
> > > > > > >
> > > > > > > Signed-off-by: Xuan Zhuo 
> > > > > > > Acked-by: Jason Wang 
> > > > > > > ---
> > > > > > >   drivers/virtio/virtio_ring.c | 34 
> > > > > > > ++
> > > > > > >   1 file changed, 34 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_ring.c 
> > > > > > > b/drivers/virtio/virtio_ring.c
> > > > > > > index b6fda91c8059..58355e1ac7d7 100644
> > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > @@ -220,6 +220,7 @@ static struct virtqueue 
> > > > > > > *__vring_new_virtqueue(unsigned int index,
> > > > > > >void 
> > > > > > > (*callback)(struct virtqueue *),
> > > > > > >const char *name);
> > > > > > >   static struct vring_desc_extra *vring_alloc_desc_extra(unsigned 
> > > > > > > int num);
> > > > > > > +static void vring_free(struct virtqueue *_vq);
> > > > > > >
> > > > > > >   /*
> > > > > > >* Helpers.
> > > > > > > @@ -1117,6 +1118,39 @@ static struct virtqueue 
> > > > > > > *vring_create_virtqueue_split(
> > > > > > > return vq;
> > > > > > >   }
> > > > > > >
> > > > > > > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > > > > > > +{
> > > > > > > +   struct vring_virtqueue_split vring_split = {};
> > > > > > > +   struct vring_virtqueue *vq = to_vvq(_vq);
> > > > > > > +   struct virtio_device *vdev = _vq->vdev;
> > > > > > > +   int err;
> > > > > > > +
> > > > > > > +   err = vring_alloc_queue_split(&vring_split, vdev, num,
> > > > > > > + vq->split.vring_align,
> > > > > > > + vq->split.may_reduce_num);
> > > > > > > +   if (err)
> > > > > > > +   goto err;
> > > > > >
> > > > > >
> > > > > > I think we don't need to do anything here?
> > > > >
> > > > > Am I missing something?
> > > >
> > > > I meant it looks to me most of the virtqueue_reinit() is unnecessary.
> > > > We probably only need to reinit avail/used idx there.
> > >
> > >
> > > In this function, we can indeed remove some code.
> > >
> > > >   static void virtqueue_reinit_split(struct vring_virtqueue *vq)
> > > >   {
> > > >   int size, i;
> > > >
> > > >   memset(vq->split.vring.desc, 0, 
> > > > vq->split.queue_size_in_bytes);
> > > >
> > > >   size = sizeof(struct vring_desc_state_split) * 
> > > > vq->split.vring.num;
> > > >   memset(vq->split.desc_state, 0, size);
> > > >
> > > >   size = sizeof(struct vring_desc_extra) * 
> > > > vq->split.vring.num;
> > > >   memset(vq->split.desc_extra, 0, size);
> > >
> > > These memsets can be removed, and theoretically it will not cause any
> > > exceptions.
> >
> > Yes, otherwise we have bugs in detach_buf().
> >
> > >
> > > >
> > > >
> > > >
> > > >   for (i = 0; i < vq->split.vring.num - 1; i++)
> > > >   vq->split.desc_extra[i].next = i + 1;
> > >
> > > This can also be removed, but we need to record free_head that will been 
> > > update
> > > inside virtqueue_init().
> >
> > We can simply keep free_head unchanged? Otherwise it's a bug somewhere I 
> > guess.
> >
> >
> > >
> > > >
> > > >   virtqueue_init(vq, vq->split.vring.num);
> > >
> > > There are some operations in this, which can also be skipped, such as 
> > > setting
> > > use_dma_api. But I think calling this function directly will be more 
> > > convenient
> > > for maintenance.
> >
> > I don't see anything that is necessary here.
>
> These three are currently inside virtqueue_init()
>
> vq->last_used_idx = 0;
> vq->event_triggered = false;
> vq->num_added = 0;

Right. Let's keep it there.

(Though it's kind of strange that the last_used_idx is not initialized
at the same place with avail_idx/flags_shadow, we can optimize it on
top).

Thanks

>
> Thanks.
>
>
> >
> > >
> > >
> > > >   virtqueue_vring_init_split(&vq->split, vq);
> > >
> >

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 4:27 PM Yongji Xie  wrote:
>
> On Thu, Jul 28, 2022 at 2:45 PM Jason Wang  wrote:
> >
> > On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie  wrote:
> > >
> > > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang  wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji  
> > > > wrote:
> > > > >
> > > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > > support querying some information of IOVA regions.
> > > > >
> > > > > Now it can be used to query whether the IOVA region
> > > > > supports userspace memory registration.
> > > > >
> > > > > Signed-off-by: Xie Yongji 
> > > > > ---
> > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 
> > > > > ++
> > > > >  include/uapi/linux/vduse.h | 24 ++
> > > > >  2 files changed, 63 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c 
> > > > > b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index eedff0a3885a..e820c37dcba8 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, 
> > > > > unsigned int cmd,
> > > > >umem.size);
> > > > > break;
> > > > > }
> > > > > +   case VDUSE_IOTLB_GET_INFO: {
> > > > > +   struct vduse_iova_info info;
> > > > > +   struct vhost_iotlb_map *map;
> > > > > +   struct vduse_iova_domain *domain = dev->domain;
> > > > > +
> > > > > +   ret = -EFAULT;
> > > > > +   if (copy_from_user(&info, argp, sizeof(info)))
> > > > > +   break;
> > > > > +
> > > > > +   ret = -EINVAL;
> > > > > +   if (info.start > info.last)
> > > > > +   break;
> > > > > +
> > > > > +   if (!is_mem_zero((const char *)info.reserved,
> > > > > +sizeof(info.reserved)))
> > > > > +   break;
> > > > > +
> > > > > +   spin_lock(&domain->iotlb_lock);
> > > > > +   map = vhost_iotlb_itree_first(domain->iotlb,
> > > > > + info.start, info.last);
> > > > > +   if (map) {
> > > > > +   info.start = map->start;
> > > > > +   info.last = map->last;
> > > > > +   info.capability = 0;
> > > > > +   if (domain->bounce_map && map->start >= 0 &&
> > > > > +   map->last < domain->bounce_size)
> > > > > +   info.capability |= 
> > > > > VDUSE_IOVA_CAP_UMEM;
> > > > > +   }
> > > > > +   spin_unlock(&domain->iotlb_lock);
> > > > > +   if (!map)
> > > > > +   break;
> > > > > +
> > > > > +   ret = -EFAULT;
> > > > > +   if (copy_to_user(argp, &info, sizeof(info)))
> > > > > +   break;
> > > > > +
> > > > > +   ret = 0;
> > > > > +   break;
> > > > > +   }
> > > > > default:
> > > > > ret = -ENOIOCTLCMD;
> > > > > break;
> > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > > --- a/include/uapi/linux/vduse.h
> > > > > +++ b/include/uapi/linux/vduse.h
> > > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > > >  /* De-register the userspace memory. Caller should set iova and size 
> > > > > field. */
> > > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct 
> > > > > vduse_iova_umem)
> > > > >
> > > > > +/**
> > > > > + * struct vduse_iova_info - information of one IOVA region
> > > > > + * @start: start of the IOVA region
> > > > > + * @last: last of the IOVA region
> > > > > + * @capability: capability of the IOVA regsion
> > > > > + * @reserved: for future use, needs to be initialized to zero
> > > > > + *
> > > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > > + * one IOVA region.
> > > > > + */
> > > > > +struct vduse_iova_info {
> > > > > +   __u64 start;
> > > > > +   __u64 last;
> > > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > > +   __u64 capability;
> > > > > +   __u64 reserved[3];
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Find the first IOVA region that overlaps with the range [start, 
> > > > > last]
> > > >
> > > > So the code is actually find the IOVA region that is the super range
> > > > of [start, last] instead of overlap:
> > > >
> > >
> > > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > > range of [start,last] be considered overlapping?
> >
> > Ok, but what I want to ask is, under which condition can we hit the
> > following case
> >
> > map->last >= domain->bounce_size ?
> >
>
> I think we would not hit t

Re: [PATCH] VMCI: Update maintainers for VMCI

[Greg KH]

On Tue, Jul 26, 2022 at 09:35:54AM -0700, rajesh jalisatgi wrote:



This message never made it to any public list as it was sent in HTML
format.  Please resend in a way we can properly access it.

thanks,

greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

spec clarification (was Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space)

[Si-Wei Liu]

Hi Michael,

Could you please comment on the different wording between "exist" and 
"valid" in spec? Having seen quite a few relevant discussions regarding 
MTU validation and VERSION_1 negotiation on s390, I was in the 
impression this is not the first time people getting confused because of 
ambiguity of random wording without detailed example helping to clarify 
around the context, or due lack of clear definition set ahead. I like 
your idea to keep things consistent (conditional depending on feature 
presence), however, without proper interpretation of how spec is 
supposed to work, we are on a slippery slope towards inconsistency.


On 7/28/2022 12:36 AM, Jason Wang wrote:

And looking at:

"The mac address field always exists (though is only valid if
VIRTIO_NET_F_MAC is set), and status only exists if VIRTIO_NET_F_STATUS
is set."

It appears to me there's a border line set between "exist" and "valid".
If I understand the spec wording correctly, a spec-conforming device
implementation may or may not offer valid status value in the config
space when VIRTIO_NET_F_STATUS is offered, but before the feature is
negotiated.

That's not what I read, maybe Michael can clarify this.



And Jason and I find below normatives are conflict with each other.
"The device MUST allow reading of any device-specific configuration 
field before FEATURES_OK is set by the driver. This includes fields 
which are conditional on feature bits, as long as those feature bits 
are offered by the device." 

...

And also there are special cases where the read of specific
configuration space field MUST be deferred to until FEATURES_OK is set:

"If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode
can be read or set through the writeback field. 0 corresponds to a
writethrough cache, 1 to a writeback cache11. The cache mode after reset
can be either writeback or writethrough. The actual mode can be
determined by reading writeback after feature negotiation."
"The driver MUST NOT read writeback before setting the FEATURES_OK
device status bit."

This seems to conflict with the normatives I quoted above, and I don't
get why we need this.



Thanks,
-Siwei___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v13 16/42] virtio_ring: split: introduce virtqueue_resize_split()

[Xuan Zhuo]

On Thu, 28 Jul 2022 15:42:50 +0800, Jason Wang  wrote:
> On Thu, Jul 28, 2022 at 3:24 PM Xuan Zhuo  wrote:
> >
> > On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang  wrote:
> > > On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo  
> > > wrote:
> > > >
> > > > On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang  
> > > > wrote:
> > > > >
> > > > > 在 2022/7/26 15:21, Xuan Zhuo 写道:
> > > > > > virtio ring split supports resize.
> > > > > >
> > > > > > Only after the new vring is successfully allocated based on the new 
> > > > > > num,
> > > > > > we will release the old vring. In any case, an error is returned,
> > > > > > indicating that the vring still points to the old vring.
> > > > > >
> > > > > > In the case of an error, re-initialize(virtqueue_reinit_split()) the
> > > > > > virtqueue to ensure that the vring can be used.
> > > > > >
> > > > > > Signed-off-by: Xuan Zhuo 
> > > > > > Acked-by: Jason Wang 
> > > > > > ---
> > > > > >   drivers/virtio/virtio_ring.c | 34 
> > > > > > ++
> > > > > >   1 file changed, 34 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c 
> > > > > > b/drivers/virtio/virtio_ring.c
> > > > > > index b6fda91c8059..58355e1ac7d7 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -220,6 +220,7 @@ static struct virtqueue 
> > > > > > *__vring_new_virtqueue(unsigned int index,
> > > > > >void (*callback)(struct 
> > > > > > virtqueue *),
> > > > > >const char *name);
> > > > > >   static struct vring_desc_extra *vring_alloc_desc_extra(unsigned 
> > > > > > int num);
> > > > > > +static void vring_free(struct virtqueue *_vq);
> > > > > >
> > > > > >   /*
> > > > > >* Helpers.
> > > > > > @@ -1117,6 +1118,39 @@ static struct virtqueue 
> > > > > > *vring_create_virtqueue_split(
> > > > > > return vq;
> > > > > >   }
> > > > > >
> > > > > > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > > > > > +{
> > > > > > +   struct vring_virtqueue_split vring_split = {};
> > > > > > +   struct vring_virtqueue *vq = to_vvq(_vq);
> > > > > > +   struct virtio_device *vdev = _vq->vdev;
> > > > > > +   int err;
> > > > > > +
> > > > > > +   err = vring_alloc_queue_split(&vring_split, vdev, num,
> > > > > > + vq->split.vring_align,
> > > > > > + vq->split.may_reduce_num);
> > > > > > +   if (err)
> > > > > > +   goto err;
> > > > >
> > > > >
> > > > > I think we don't need to do anything here?
> > > >
> > > > Am I missing something?
> > >
> > > I meant it looks to me most of the virtqueue_reinit() is unnecessary.
> > > We probably only need to reinit avail/used idx there.
> >
> >
> > In this function, we can indeed remove some code.
> >
> > >   static void virtqueue_reinit_split(struct vring_virtqueue *vq)
> > >   {
> > >   int size, i;
> > >
> > >   memset(vq->split.vring.desc, 0, 
> > > vq->split.queue_size_in_bytes);
> > >
> > >   size = sizeof(struct vring_desc_state_split) * 
> > > vq->split.vring.num;
> > >   memset(vq->split.desc_state, 0, size);
> > >
> > >   size = sizeof(struct vring_desc_extra) * 
> > > vq->split.vring.num;
> > >   memset(vq->split.desc_extra, 0, size);
> >
> > These memsets can be removed, and theoretically it will not cause any
> > exceptions.
>
> Yes, otherwise we have bugs in detach_buf().
>
> >
> > >
> > >
> > >
> > >   for (i = 0; i < vq->split.vring.num - 1; i++)
> > >   vq->split.desc_extra[i].next = i + 1;
> >
> > This can also be removed, but we need to record free_head that will been 
> > update
> > inside virtqueue_init().
>
> We can simply keep free_head unchanged? Otherwise it's a bug somewhere I 
> guess.
>
>
> >
> > >
> > >   virtqueue_init(vq, vq->split.vring.num);
> >
> > There are some operations in this, which can also be skipped, such as 
> > setting
> > use_dma_api. But I think calling this function directly will be more 
> > convenient
> > for maintenance.
>
> I don't see anything that is necessary here.

These three are currently inside virtqueue_init()

vq->last_used_idx = 0;
vq->event_triggered = false;
vq->num_added = 0;

Thanks.


>
> >
> >
> > >   virtqueue_vring_init_split(&vq->split, vq);
> >
> > virtqueue_vring_init_split() is necessary.
>
> Right.
>
> >
> > >   }
> >
> > Another method, we can take out all the variables to be reinitialized
> > separately, and repackage them into a new function. I don’t think it’s worth
> > it, because this path will only be reached if the memory allocation fails, 
> > which
> > is a rare occurrence. In this case, doing so will increase the cost of
> > maintenance. If you think so also, I will remove the above memset in the 
> > next
> > version.
>
> I agree.
>
> Thanks
>
> 

Re: [PATCH 12/12] drm/format-helper: Move destination-buffer handling into internal helper

[Thomas Zimmermann]

Hi

Am 28.07.22 um 09:26 schrieb José Expósito:

Hi!

On Wed, Jul 27, 2022 at 01:33:12PM +0200, Thomas Zimmermann wrote:

The format-convertion helpers handle several cases for different
values of destination buffer and pitch. Move that code into the
internal helper drm_fb_xfrm() and avoid quite a bit of duplucation.

Signed-off-by: Thomas Zimmermann 
---
  drivers/gpu/drm/drm_format_helper.c | 169 +++-
  1 file changed, 64 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c 
b/drivers/gpu/drm/drm_format_helper.c
index d296d181659d..35aebdb90165 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -41,11 +41,11 @@ unsigned int drm_fb_clip_offset(unsigned int pitch, const 
struct drm_format_info
  }
  EXPORT_SYMBOL(drm_fb_clip_offset);
  
-/* TODO: Make this functon work with multi-plane formats. */

-static int drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long 
dst_pixsize,
-  const void *vaddr, const struct drm_framebuffer *fb,
-  const struct drm_rect *clip, bool vaddr_cached_hint,
-  void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned 
int npixels))
+/* TODO: Make this function work with multi-plane formats. */
+static int __drm_fb_xfrm(void *dst, unsigned long dst_pitch, unsigned long 
dst_pixsize,
+const void *vaddr, const struct drm_framebuffer *fb,
+const struct drm_rect *clip, bool vaddr_cached_hint,
+void (*xfrm_line)(void *dbuf, const void *sbuf, 
unsigned int npixels))
  {
unsigned long linepixels = drm_rect_width(clip);
unsigned long lines = drm_rect_height(clip);
@@ -84,11 +84,11 @@ static int drm_fb_xfrm(void *dst, unsigned long dst_pitch, 
unsigned long dst_pix
return 0;
  }
  
-/* TODO: Make this functon work with multi-plane formats. */

-static int drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, 
unsigned long dst_pixsize,
-   const void *vaddr, const struct drm_framebuffer *fb,
-   const struct drm_rect *clip, bool vaddr_cached_hint,
-   void (*xfrm_line)(void *dbuf, const void *sbuf, 
unsigned int npixels))
+/* TODO: Make this function work with multi-plane formats. */
+static int __drm_fb_xfrm_toio(void __iomem *dst, unsigned long dst_pitch, 
unsigned long dst_pixsize,
+ const void *vaddr, const struct drm_framebuffer 
*fb,
+ const struct drm_rect *clip, bool 
vaddr_cached_hint,
+ void (*xfrm_line)(void *dbuf, const void *sbuf, 
unsigned int npixels))
  {
unsigned long linepixels = drm_rect_width(clip);
unsigned long lines = drm_rect_height(clip);
@@ -129,6 +129,29 @@ static int drm_fb_xfrm_toio(void __iomem *dst, unsigned 
long dst_pitch, unsigned
return 0;
  }
  
+/* TODO: Make this function work with multi-plane formats. */

+static int drm_fb_xfrm(struct iosys_map *dst,
+  const unsigned int *dst_pitch, const u8 *dst_pixsize,
+  const struct iosys_map *vmap, const struct 
drm_framebuffer *fb,
+  const struct drm_rect *clip, bool vaddr_cached_hint,
+  void (*xfrm_line)(void *dbuf, const void *sbuf, unsigned 
int npixels))
+{
+   static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
+   0, 0, 0, 0
+   };
+
+   if (!dst_pitch)
+   dst_pitch = default_dst_pitch;
+
+   if (dst[0].is_iomem)
+   return __drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 
dst_pixsize[0],
+ vmap[0].vaddr, fb, clip, false, 
xfrm_line);
+   else
+   return __drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], dst_pixsize[0],
+vmap[0].vaddr, fb, clip, false, xfrm_line);
+}
+
+


Nit: Extra blank line


Oh!




  /**
   * drm_fb_memcpy - Copy clip buffer
   * @dst: Array of destination buffers
@@ -213,14 +236,10 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned 
int *dst_pitch,
 const struct iosys_map *vmap, const struct drm_framebuffer *fb,
 const struct drm_rect *clip, bool cached)
  {
-   static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
-   0, 0, 0, 0
-   };
const struct drm_format_info *format = fb->format;
-   u8 cpp = format->cpp[0];
void (*swab_line)(void *dbuf, const void *sbuf, unsigned int npixels);
  
-	switch (cpp) {

+   switch (format->cpp[0]) {
case 4:
swab_line = drm_fb_swab32_line;
break;
@@ -230,21 +249,10 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned 
int *dst_pitch,
default:
drm_warn_once(fb->dev, "Format %p4cc has unsupported pixel 

Re: [PATCH v13 16/42] virtio_ring: split: introduce virtqueue_resize_split()

[Jason Wang]

On Thu, Jul 28, 2022 at 3:24 PM Xuan Zhuo  wrote:
>
> On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang  wrote:
> > On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo  
> > wrote:
> > >
> > > On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang  
> > > wrote:
> > > >
> > > > 在 2022/7/26 15:21, Xuan Zhuo 写道:
> > > > > virtio ring split supports resize.
> > > > >
> > > > > Only after the new vring is successfully allocated based on the new 
> > > > > num,
> > > > > we will release the old vring. In any case, an error is returned,
> > > > > indicating that the vring still points to the old vring.
> > > > >
> > > > > In the case of an error, re-initialize(virtqueue_reinit_split()) the
> > > > > virtqueue to ensure that the vring can be used.
> > > > >
> > > > > Signed-off-by: Xuan Zhuo 
> > > > > Acked-by: Jason Wang 
> > > > > ---
> > > > >   drivers/virtio/virtio_ring.c | 34 ++
> > > > >   1 file changed, 34 insertions(+)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c 
> > > > > b/drivers/virtio/virtio_ring.c
> > > > > index b6fda91c8059..58355e1ac7d7 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -220,6 +220,7 @@ static struct virtqueue 
> > > > > *__vring_new_virtqueue(unsigned int index,
> > > > >void (*callback)(struct 
> > > > > virtqueue *),
> > > > >const char *name);
> > > > >   static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int 
> > > > > num);
> > > > > +static void vring_free(struct virtqueue *_vq);
> > > > >
> > > > >   /*
> > > > >* Helpers.
> > > > > @@ -1117,6 +1118,39 @@ static struct virtqueue 
> > > > > *vring_create_virtqueue_split(
> > > > > return vq;
> > > > >   }
> > > > >
> > > > > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > > > > +{
> > > > > +   struct vring_virtqueue_split vring_split = {};
> > > > > +   struct vring_virtqueue *vq = to_vvq(_vq);
> > > > > +   struct virtio_device *vdev = _vq->vdev;
> > > > > +   int err;
> > > > > +
> > > > > +   err = vring_alloc_queue_split(&vring_split, vdev, num,
> > > > > + vq->split.vring_align,
> > > > > + vq->split.may_reduce_num);
> > > > > +   if (err)
> > > > > +   goto err;
> > > >
> > > >
> > > > I think we don't need to do anything here?
> > >
> > > Am I missing something?
> >
> > I meant it looks to me most of the virtqueue_reinit() is unnecessary.
> > We probably only need to reinit avail/used idx there.
>
>
> In this function, we can indeed remove some code.
>
> >   static void virtqueue_reinit_split(struct vring_virtqueue *vq)
> >   {
> >   int size, i;
> >
> >   memset(vq->split.vring.desc, 0, 
> > vq->split.queue_size_in_bytes);
> >
> >   size = sizeof(struct vring_desc_state_split) * 
> > vq->split.vring.num;
> >   memset(vq->split.desc_state, 0, size);
> >
> >   size = sizeof(struct vring_desc_extra) * vq->split.vring.num;
> >   memset(vq->split.desc_extra, 0, size);
>
> These memsets can be removed, and theoretically it will not cause any
> exceptions.

Yes, otherwise we have bugs in detach_buf().

>
> >
> >
> >
> >   for (i = 0; i < vq->split.vring.num - 1; i++)
> >   vq->split.desc_extra[i].next = i + 1;
>
> This can also be removed, but we need to record free_head that will been 
> update
> inside virtqueue_init().

We can simply keep free_head unchanged? Otherwise it's a bug somewhere I guess.


>
> >
> >   virtqueue_init(vq, vq->split.vring.num);
>
> There are some operations in this, which can also be skipped, such as setting
> use_dma_api. But I think calling this function directly will be more 
> convenient
> for maintenance.

I don't see anything that is necessary here.

>
>
> >   virtqueue_vring_init_split(&vq->split, vq);
>
> virtqueue_vring_init_split() is necessary.

Right.

>
> >   }
>
> Another method, we can take out all the variables to be reinitialized
> separately, and repackage them into a new function. I don’t think it’s worth
> it, because this path will only be reached if the memory allocation fails, 
> which
> is a rare occurrence. In this case, doing so will increase the cost of
> maintenance. If you think so also, I will remove the above memset in the next
> version.

I agree.

Thanks

>
> Thanks.
>
>
> >
> > Thanks
> >
> > >
> > > >
> > > >
> > > > > +
> > > > > +   err = vring_alloc_state_extra_split(&vring_split);
> > > > > +   if (err) {
> > > > > +   vring_free_split(&vring_split, vdev);
> > > > > +   goto err;
> > > >
> > > >
> > > > I suggest to move vring_free_split() into a dedicated error label.
> > >
> > > Will change.
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > +   }
> > > > > +
> > > > > +   vring_fr

Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Jason Wang]

On Thu, Jul 28, 2022 at 3:09 PM Si-Wei Liu  wrote:
>
>
>
> On 7/27/2022 7:06 PM, Jason Wang wrote:
> >
> > 在 2022/7/28 08:56, Si-Wei Liu 写道:
> >>
> >>
> >> On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:
> >>>
> >>>
> >>> On 7/27/2022 5:43 PM, Si-Wei Liu wrote:
>  Sorry to chime in late in the game. For some reason I couldn't get
>  to most emails for this discussion (I only subscribed to the
>  virtualization list), while I was taking off amongst the past few
>  weeks.
> 
>  It looks to me this patch is incomplete. Noted down the way in
>  vdpa_dev_net_config_fill(), we have the following:
>   features = vdev->config->get_driver_features(vdev);
>   if (nla_put_u64_64bit(msg,
>  VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> VDPA_ATTR_PAD))
>   return -EMSGSIZE;
> 
>  Making call to .get_driver_features() doesn't make sense when
>  feature negotiation isn't complete. Neither should present
>  negotiated_features to userspace before negotiation is done.
> 
>  Similarly, max_vqp through vdpa_dev_net_mq_config_fill() probably
>  should not show before negotiation is done - it depends on driver
>  features negotiated.
> >>> I have another patch in this series introduces device_features and
> >>> will report device_features to the userspace even features
> >>> negotiation not done. Because the spec says we should allow driver
> >>> access the config space before FEATURES_OK.
> >> The config space can be accessed by guest before features_ok doesn't
> >> necessarily mean the value is valid.
> >
> >
> > It's valid as long as the device offers the feature:
> >
> > "The device MUST allow reading of any device-specific configuration
> > field before FEATURES_OK is set by the driver. This includes fields
> > which are conditional on feature bits, as long as those feature bits
> > are offered by the device."
> I guess this statement only conveys that the field in config space can
> be read before FEATURES_OK is set, though it does not *explicitly*
> states the validity of field.

My understanding is that it should be valid as long as the device
offers the feature.

For example, if _MQ is offered by device, the max_virt_queue_pairs is
always valid and can be read from the driver no matter whether _MQ is
negotiated.

>
> And looking at:
>
> "The mac address field always exists (though is only valid if
> VIRTIO_NET_F_MAC is set), and status only exists if VIRTIO_NET_F_STATUS
> is set."
>
> It appears to me there's a border line set between "exist" and "valid".
> If I understand the spec wording correctly, a spec-conforming device
> implementation may or may not offer valid status value in the config
> space when VIRTIO_NET_F_STATUS is offered, but before the feature is
> negotiated.

That's not what I read, maybe Michael can clarify this.

> On the other hand, config space should contain valid mac
> address the moment VIRTIO_NET_F_MAC feature is offered, regardless being
> negotiated or not.

I agree here.

>By that, there seems to be leeway for the device
> implementation to decide when config space field may become valid,
> though for most of QEMU's software virtio devices, valid value is
> present to config space the very first moment when feature is offered.
>
> "If the VIRTIO_NET_F_MAC feature bit is set, the configuration space mac
> entry indicates the “physical” address of the network card, otherwise
> the driver would typically generate a random local MAC address."
> "If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status
> comes from the bottom bit of status. Otherwise, the driver assumes it’s
> active."

This is mostly the way how drivers that don't support _F_STATUS work.

>
> And also there are special cases where the read of specific
> configuration space field MUST be deferred to until FEATURES_OK is set:
>
> "If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode
> can be read or set through the writeback field. 0 corresponds to a
> writethrough cache, 1 to a writeback cache11. The cache mode after reset
> can be either writeback or writethrough. The actual mode can be
> determined by reading writeback after feature negotiation."
> "The driver MUST NOT read writeback before setting the FEATURES_OK
> device status bit."

This seems to conflict with the normatives I quoted above, and I don't
get why we need this.

> "If VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH is not,
> the device MUST initialize writeback to 0."
>
> Since the spec doesn't explicitly mandate the validity of each config
> space field when feature of concern is offered, to be safer we'd have to
> live with odd device implementation. I know for sure QEMU software
> devices won't for 99% of these cases, but that's not what is currently
> defined in the spec.
>
> >
> >
> >> You may want to double check with Michael for what he quoted earlier:
> >>> Nope:
> >>>
>

Re: [PATCH 04/12] drm/format-helper: Rework XRGB8888-to-RGBG332 conversion

[Thomas Zimmermann]

Hi

Am 28.07.22 um 09:13 schrieb José Expósito:

Hi Thomas,

On Wed, Jul 27, 2022 at 01:33:04PM +0200, Thomas Zimmermann wrote:

Update XRGB-to-RGB332 conversion to support struct iosys_map
and convert all users. Although these are single-plane color formats,
the new interface supports multi-plane formats for consistency with
drm_fb_blit().

Signed-off-by: Thomas Zimmermann 


Tested-by: José Expósito 
Reviewed-by: José Expósito 

I just ran the tests in x86_64 and UML and they work as expected.
I need to find some time to review all patches, but this one LGTM.


Thanks a lot.



This series will cause conflicts with [1]. Depending on which patchset
gets merged earlier, we will have to resolve the conflicts in one
series or the other.


I've seen this. Go ahead and commit your patches if they are ready. I 
can easily rebase on top.


Best reards
Thomas



Best wishes,
Jose

[1] https://patchwork.kernel.org/project/dri-devel/list/?series=663266


---
  drivers/gpu/drm/drm_format_helper.c   | 25 ++-
  drivers/gpu/drm/gud/gud_pipe.c|  2 +-
  .../gpu/drm/tests/drm_format_helper_test.c| 14 ++-
  include/drm/drm_format_helper.h   |  5 ++--
  4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c 
b/drivers/gpu/drm/drm_format_helper.c
index fa22d3cb11e8..2b5c3746ff4a 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -265,18 +265,31 @@ static void drm_fb_xrgb_to_rgb332_line(void *dbuf, 
const void *sbuf, unsigne
  
  /**

   * drm_fb_xrgb_to_rgb332 - Convert XRGB to RGB332 clip buffer
- * @dst: RGB332 destination buffer
- * @dst_pitch: Number of bytes between two consecutive scanlines within dst
- * @src: XRGB source buffer
+ * @dst: Array of RGB332 destination buffers
+ * @dst_pitch: Array of numbers of bytes between two consecutive scanlines 
within dst
+ * @vmap: Array of XRGB source buffers
   * @fb: DRM framebuffer
   * @clip: Clip rectangle area to copy
   *
   * Drivers can use this function for RGB332 devices that don't natively 
support XRGB.
   */
-void drm_fb_xrgb_to_rgb332(void *dst, unsigned int dst_pitch, const void 
*src,
-  const struct drm_framebuffer *fb, const struct 
drm_rect *clip)
+void drm_fb_xrgb_to_rgb332(struct iosys_map *dst, const unsigned int 
*dst_pitch,
+  const struct iosys_map *vmap, const struct 
drm_framebuffer *fb,
+  const struct drm_rect *clip)
  {
-   drm_fb_xfrm(dst, dst_pitch, 1, src, fb, clip, false, 
drm_fb_xrgb_to_rgb332_line);
+   static const unsigned int default_dst_pitch[DRM_FORMAT_MAX_PLANES] = {
+   0, 0, 0, 0
+   };
+
+   if (!dst_pitch)
+   dst_pitch = default_dst_pitch;
+
+   if (dst[0].is_iomem)
+   drm_fb_xfrm_toio(dst[0].vaddr_iomem, dst_pitch[0], 1, 
vmap[0].vaddr, fb, clip,
+false, drm_fb_xrgb_to_rgb332_line);
+   else
+   drm_fb_xfrm(dst[0].vaddr, dst_pitch[0], 1, vmap[0].vaddr, fb, 
clip,
+   false, drm_fb_xrgb_to_rgb332_line);
  }
  EXPORT_SYMBOL(drm_fb_xrgb_to_rgb332);
  
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c

index a15cda9ba058..426a3ae6cc50 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -196,7 +196,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct 
drm_framebuffer *fb,
} else if (format->format == DRM_FORMAT_R8) {
drm_fb_xrgb_to_gray8(buf, 0, vaddr, fb, rect);
} else if (format->format == DRM_FORMAT_RGB332) {
-   drm_fb_xrgb_to_rgb332(buf, 0, vaddr, fb, rect);
+   drm_fb_xrgb_to_rgb332(&dst, NULL, map_data, fb, 
rect);
} else if (format->format == DRM_FORMAT_RGB565) {
drm_fb_xrgb_to_rgb565(buf, 0, vaddr, fb, rect, 
gud_is_big_endian());
} else if (format->format == DRM_FORMAT_RGB888) {
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c 
b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 98583bf56044..b74dba06f704 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -124,7 +124,8 @@ static void xrgb_to_rgb332_test(struct kunit *test)
  {
const struct xrgb_to_rgb332_case *params = test->param_value;
size_t dst_size;
-   __u8 *dst = NULL;
+   struct iosys_map dst, xrgb;
+   __u8 *buf = NULL;
  
  	struct drm_framebuffer fb = {

.format = drm_format_info(DRM_FORMAT_XRGB),
@@ -135,12 +136,13 @@ static void xrgb_to_rgb332_test(struct kunit *test)
   ¶ms->clip);
KUNIT_ASSERT_GT(test, dst_size, 0);
  
-	dst = kunit_

Re: [PATCH v13 16/42] virtio_ring: split: introduce virtqueue_resize_split()

[Xuan Zhuo]

On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang  wrote:
> On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo  wrote:
> >
> > On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang  wrote:
> > >
> > > 在 2022/7/26 15:21, Xuan Zhuo 写道:
> > > > virtio ring split supports resize.
> > > >
> > > > Only after the new vring is successfully allocated based on the new num,
> > > > we will release the old vring. In any case, an error is returned,
> > > > indicating that the vring still points to the old vring.
> > > >
> > > > In the case of an error, re-initialize(virtqueue_reinit_split()) the
> > > > virtqueue to ensure that the vring can be used.
> > > >
> > > > Signed-off-by: Xuan Zhuo 
> > > > Acked-by: Jason Wang 
> > > > ---
> > > >   drivers/virtio/virtio_ring.c | 34 ++
> > > >   1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index b6fda91c8059..58355e1ac7d7 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -220,6 +220,7 @@ static struct virtqueue 
> > > > *__vring_new_virtqueue(unsigned int index,
> > > >void (*callback)(struct 
> > > > virtqueue *),
> > > >const char *name);
> > > >   static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int 
> > > > num);
> > > > +static void vring_free(struct virtqueue *_vq);
> > > >
> > > >   /*
> > > >* Helpers.
> > > > @@ -1117,6 +1118,39 @@ static struct virtqueue 
> > > > *vring_create_virtqueue_split(
> > > > return vq;
> > > >   }
> > > >
> > > > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > > > +{
> > > > +   struct vring_virtqueue_split vring_split = {};
> > > > +   struct vring_virtqueue *vq = to_vvq(_vq);
> > > > +   struct virtio_device *vdev = _vq->vdev;
> > > > +   int err;
> > > > +
> > > > +   err = vring_alloc_queue_split(&vring_split, vdev, num,
> > > > + vq->split.vring_align,
> > > > + vq->split.may_reduce_num);
> > > > +   if (err)
> > > > +   goto err;
> > >
> > >
> > > I think we don't need to do anything here?
> >
> > Am I missing something?
>
> I meant it looks to me most of the virtqueue_reinit() is unnecessary.
> We probably only need to reinit avail/used idx there.


In this function, we can indeed remove some code.

>   static void virtqueue_reinit_split(struct vring_virtqueue *vq)
>   {
>   int size, i;
>
>   memset(vq->split.vring.desc, 0, vq->split.queue_size_in_bytes);
>
>   size = sizeof(struct vring_desc_state_split) * 
> vq->split.vring.num;
>   memset(vq->split.desc_state, 0, size);
>
>   size = sizeof(struct vring_desc_extra) * vq->split.vring.num;
>   memset(vq->split.desc_extra, 0, size);

These memsets can be removed, and theoretically it will not cause any
exceptions.

>
>
>
>   for (i = 0; i < vq->split.vring.num - 1; i++)
>   vq->split.desc_extra[i].next = i + 1;

This can also be removed, but we need to record free_head that will been update
inside virtqueue_init().

>
>   virtqueue_init(vq, vq->split.vring.num);

There are some operations in this, which can also be skipped, such as setting
use_dma_api. But I think calling this function directly will be more convenient
for maintenance.


>   virtqueue_vring_init_split(&vq->split, vq);

virtqueue_vring_init_split() is necessary.

>   }

Another method, we can take out all the variables to be reinitialized
separately, and repackage them into a new function. I don’t think it’s worth
it, because this path will only be reached if the memory allocation fails, which
is a rare occurrence. In this case, doing so will increase the cost of
maintenance. If you think so also, I will remove the above memset in the next
version.

Thanks.


>
> Thanks
>
> >
> > >
> > >
> > > > +
> > > > +   err = vring_alloc_state_extra_split(&vring_split);
> > > > +   if (err) {
> > > > +   vring_free_split(&vring_split, vdev);
> > > > +   goto err;
> > >
> > >
> > > I suggest to move vring_free_split() into a dedicated error label.
> >
> > Will change.
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> > > > +   }
> > > > +
> > > > +   vring_free(&vq->vq);
> > > > +
> > > > +   virtqueue_vring_init_split(&vring_split, vq);
> > > > +
> > > > +   virtqueue_init(vq, vring_split.vring.num);
> > > > +   virtqueue_vring_attach_split(vq, &vring_split);
> > > > +
> > > > +   return 0;
> > > > +
> > > > +err:
> > > > +   virtqueue_reinit_split(vq);
> > > > +   return -ENOMEM;
> > > > +}
> > > > +
> > > >
> > > >   /*
> > > >* Packed ring specific functions - *_packed().
> > >
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lis

Re: [PATCH V3 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Si-Wei Liu]

On 7/27/2022 8:48 AM, Michael S. Tsirkin wrote:

On Wed, Jul 27, 2022 at 03:09:43AM -0700, Si-Wei Liu wrote:


On 7/27/2022 2:01 AM, Michael S. Tsirkin wrote:

On Wed, Jul 27, 2022 at 12:50:33AM -0700, Si-Wei Liu wrote:

On 7/26/2022 11:01 PM, Michael S. Tsirkin wrote:

On Wed, Jul 27, 2022 at 03:47:35AM +, Parav Pandit wrote:

From: Zhu, Lingshan
Sent: Tuesday, July 26, 2022 10:53 PM

On 7/27/2022 10:17 AM, Parav Pandit wrote:

From: Zhu, Lingshan
Sent: Tuesday, July 26, 2022 10:15 PM

On 7/26/2022 11:56 PM, Parav Pandit wrote:

From: Zhu, Lingshan
Sent: Tuesday, July 12, 2022 11:46 PM

When the user space which invokes netlink commands, detects that

_MQ

is not supported, hence it takes max_queue_pair = 1 by itself.
I think the kernel module have all necessary information and it is
the only one which have precise information of a device, so it
should answer precisely than let the user space guess. The kernel
module should be reliable than stay silent, leave the question to
the user space

tool.

Kernel is reliable. It doesn’t expose a config space field if the
field doesn’t

exist regardless of field should have default or no default.
so when you know it is one queue pair, you should answer one, not try
to guess.

User space should not guess either. User space gets to see if _MQ

present/not present. If _MQ present than get reliable data from kernel.

If _MQ not present, it means this device has one VQ pair.

it is still a guess, right? And all user space tools implemented this
feature need to guess

No. it is not a guess.
It is explicitly checking the _MQ feature and deriving the value.
The code you proposed will be present in the user space.
It will be uniform for _MQ and 10 other features that are present now and

in the future.
MQ and other features like RSS are different. If there is no _RSS_XX, there
are no attributes like max_rss_key_size, and there is not a default value.
But for MQ, we know it has to be 1 wihtout _MQ.

"we" = user space.
To keep the consistency among all the config space fields.

Actually I looked and the code some more and I'm puzzled:


struct virtio_net_config config = {};
u64 features;
u16 val_u16;

vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));

if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac),
config.mac))
return -EMSGSIZE;


Mac returned even without VIRTIO_NET_F_MAC


val_u16 = le16_to_cpu(config.status);
if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
return -EMSGSIZE;


status returned even without VIRTIO_NET_F_STATUS

val_u16 = le16_to_cpu(config.mtu);
if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
return -EMSGSIZE;


MTU returned even without VIRTIO_NET_F_MTU


What's going on here?



I guess this is spec thing (historical debt), I vaguely recall these fields
are always present in config space regardless the existence of corresponding
feature bit.

-Siwei

Nope:

2.5.1  Driver Requirements: Device Configuration Space

...

For optional configuration space fields, the driver MUST check that the 
corresponding feature is offered
before accessing that part of the configuration space.

Well, this is driver side of requirement.


Well driver and device are the only two entities in the spec.
We should mostly concern with device side requirement as this is what 
the vdpa admin tool is targeted for. Please refer to my earlier 
email//addressed to Jason.





As this interface is for host
admin tool to query or configure vdpa device, we don't have to wait until
feature negotiation is done on guest driver to extract vdpa
attributes/parameters, say if we want to replicate another vdpa device with
the same config on migration destination. I think what may need to be fix is
to move off from using .vdpa_get_config_unlocked() which depends on feature
negotiation. And/or expose config space register values through another set
of attributes.

-Siwei



Sounds like something that might use the proposed admin queue maybe.
Hope that makes progress ...
Well it doesn't need to be that complex. Depending on need we may 
extract device configs pre-negotiation from vendor's vdpa device via 
some other op, but I doubt worth doing so. I don't quite understand the 
use case why we need to expose those values.


-Siwei

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Si-Wei Liu]

On 7/27/2022 7:06 PM, Jason Wang wrote:


在 2022/7/28 08:56, Si-Wei Liu 写道:



On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:



On 7/27/2022 5:43 PM, Si-Wei Liu wrote:
Sorry to chime in late in the game. For some reason I couldn't get 
to most emails for this discussion (I only subscribed to the 
virtualization list), while I was taking off amongst the past few 
weeks.


It looks to me this patch is incomplete. Noted down the way in 
vdpa_dev_net_config_fill(), we have the following:

 features = vdev->config->get_driver_features(vdev);
 if (nla_put_u64_64bit(msg, 
VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,

   VDPA_ATTR_PAD))
 return -EMSGSIZE;

Making call to .get_driver_features() doesn't make sense when 
feature negotiation isn't complete. Neither should present 
negotiated_features to userspace before negotiation is done.


Similarly, max_vqp through vdpa_dev_net_mq_config_fill() probably 
should not show before negotiation is done - it depends on driver 
features negotiated.
I have another patch in this series introduces device_features and 
will report device_features to the userspace even features 
negotiation not done. Because the spec says we should allow driver 
access the config space before FEATURES_OK.
The config space can be accessed by guest before features_ok doesn't 
necessarily mean the value is valid. 



It's valid as long as the device offers the feature:

"The device MUST allow reading of any device-specific configuration 
field before FEATURES_OK is set by the driver. This includes fields 
which are conditional on feature bits, as long as those feature bits 
are offered by the device."
I guess this statement only conveys that the field in config space can 
be read before FEATURES_OK is set, though it does not *explicitly* 
states the validity of field.


And looking at:

"The mac address field always exists (though is only valid if 
VIRTIO_NET_F_MAC is set), and status only exists if VIRTIO_NET_F_STATUS 
is set."


It appears to me there's a border line set between "exist" and "valid". 
If I understand the spec wording correctly, a spec-conforming device 
implementation may or may not offer valid status value in the config 
space when VIRTIO_NET_F_STATUS is offered, but before the feature is 
negotiated. On the other hand, config space should contain valid mac 
address the moment VIRTIO_NET_F_MAC feature is offered, regardless being 
negotiated or not. By that, there seems to be leeway for the device 
implementation to decide when config space field may become valid, 
though for most of QEMU's software virtio devices, valid value is 
present to config space the very first moment when feature is offered.


"If the VIRTIO_NET_F_MAC feature bit is set, the configuration space mac 
entry indicates the “physical” address of the network card, otherwise 
the driver would typically generate a random local MAC address."
"If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status 
comes from the bottom bit of status. Otherwise, the driver assumes it’s 
active."


And also there are special cases where the read of specific 
configuration space field MUST be deferred to until FEATURES_OK is set:


"If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode 
can be read or set through the writeback field. 0 corresponds to a 
writethrough cache, 1 to a writeback cache11. The cache mode after reset 
can be either writeback or writethrough. The actual mode can be 
determined by reading writeback after feature negotiation."
"The driver MUST NOT read writeback before setting the FEATURES_OK 
device status bit."
"If VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH is not, 
the device MUST initialize writeback to 0."


Since the spec doesn't explicitly mandate the validity of each config 
space field when feature of concern is offered, to be safer we'd have to 
live with odd device implementation. I know for sure QEMU software 
devices won't for 99% of these cases, but that's not what is currently 
defined in the spec.






You may want to double check with Michael for what he quoted earlier:

Nope:

2.5.1  Driver Requirements: Device Configuration Space

...

For optional configuration space fields, the driver MUST check that 
the corresponding feature is offered

before accessing that part of the configuration space.


and how many driver bugs taking wrong assumption of the validity of 
config space field without features_ok. I am not sure what use case 
you want to expose config resister values for before features_ok, if 
it's mostly for live migration I guess it's probably heading a wrong 
direction.



I guess it's not for migration. 
Then what's the other possible use case than live migration, were to 
expose config space values? Troubleshooting config space discrepancy 
between vDPA and the emulated virtio device in userspace? Or tracking 
changes in config space across feature negotiation, but for wh