On 7/29/2022 5:54 AM, Si-Wei Liu wrote:

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

quoted

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

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

quoted

quoted

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

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

quoted

quoted

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

quoted

When the user space which invokes netlink commands, 
detects that

_MQ

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

The features bits of a device don't depend on the phases, and the 
driver_features only has meaningful values when FEATURES_OK.

Thanks

-Siwei

quoted

quoted

-Siwei

quoted

Thanks,
Zhu Lingshan

quoted

-Siwei