Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Tue, Jun 18, 2024 at 6:39 PM Michael S. Tsirkin wrote: > > On Mon, Jun 17, 2024 at 09:44:21AM -0700, Jakub Kicinski wrote: > > On Mon, 17 Jun 2024 12:20:19 -0400 Michael S. Tsirkin wrote: > > > > But the virtio spec doesn't allow setting the MAC... > > > > I'm probably just lost in the conversation but there's hypervisor side > > > > and there is user/VM side, each of them already has an interface to set > > > > the MAC. The MAC doesn't matter, but I want to make sure my mental model > > > > matches reality in case we start duplicating too much.. > > > > > > An obvious part of provisioning is specifying the config space > > > of the device. > > > > Agreed, that part is obvious. > > Please go ahead, I don't really care and you clearly don't have time > > to explain. > > Thanks! > Just in case Cindy who is working on it is also confused, > here is what I meant: > > - an interface to provision a device, including its config > space, makes sense to me > - default mac address is part of config space, and would thus be covered > - note how this is different from ability to tweak the mac of an existing > device > Thanks Micheal, I will continue working in this thanks cindy > > -- > MST >
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Mon, Jun 17, 2024 at 09:44:21AM -0700, Jakub Kicinski wrote: > On Mon, 17 Jun 2024 12:20:19 -0400 Michael S. Tsirkin wrote: > > > But the virtio spec doesn't allow setting the MAC... > > > I'm probably just lost in the conversation but there's hypervisor side > > > and there is user/VM side, each of them already has an interface to set > > > the MAC. The MAC doesn't matter, but I want to make sure my mental model > > > matches reality in case we start duplicating too much.. > > > > An obvious part of provisioning is specifying the config space > > of the device. > > Agreed, that part is obvious. > Please go ahead, I don't really care and you clearly don't have time > to explain. Thanks! Just in case Cindy who is working on it is also confused, here is what I meant: - an interface to provision a device, including its config space, makes sense to me - default mac address is part of config space, and would thus be covered - note how this is different from ability to tweak the mac of an existing device -- MST
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Mon, 17 Jun 2024 12:20:19 -0400 Michael S. Tsirkin wrote: > > But the virtio spec doesn't allow setting the MAC... > > I'm probably just lost in the conversation but there's hypervisor side > > and there is user/VM side, each of them already has an interface to set > > the MAC. The MAC doesn't matter, but I want to make sure my mental model > > matches reality in case we start duplicating too much.. > > An obvious part of provisioning is specifying the config space > of the device. Agreed, that part is obvious. Please go ahead, I don't really care and you clearly don't have time to explain.
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Mon, Jun 17, 2024 at 08:20:02AM -0700, Jakub Kicinski wrote: > On Mon, 17 Jun 2024 09:47:21 -0400 Michael S. Tsirkin wrote: > > I don't know what this discussion is about, at this point. > > For better or worse, vdpa gained interfaces for provisioning > > new devices. Yes the solution space was wide but it's been there > > for years so kind of too late to try and make people > > move to another interface for that. > > > > Having said that, vdpa interfaces are all built around > > virtio spec. Let's try to stick to that. > > But the virtio spec doesn't allow setting the MAC... > I'm probably just lost in the conversation but there's hypervisor side > and there is user/VM side, each of them already has an interface to set > the MAC. The MAC doesn't matter, but I want to make sure my mental model > matches reality in case we start duplicating too much.. An obvious part of provisioning is specifying the config space of the device. -- MST
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Mon, 17 Jun 2024 09:47:21 -0400 Michael S. Tsirkin wrote: > I don't know what this discussion is about, at this point. > For better or worse, vdpa gained interfaces for provisioning > new devices. Yes the solution space was wide but it's been there > for years so kind of too late to try and make people > move to another interface for that. > > Having said that, vdpa interfaces are all built around > virtio spec. Let's try to stick to that. But the virtio spec doesn't allow setting the MAC... I'm probably just lost in the conversation but there's hypervisor side and there is user/VM side, each of them already has an interface to set the MAC. The MAC doesn't matter, but I want to make sure my mental model matches reality in case we start duplicating too much..
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Mon, Jun 17, 2024 at 03:02:43PM +0200, Jiri Pirko wrote: > Mon, Jun 17, 2024 at 01:48:02PM CEST, pa...@nvidia.com wrote: > > > >> From: Jiri Pirko > >> Sent: Monday, June 17, 2024 5:10 PM > >> > >> Mon, Jun 17, 2024 at 11:44:53AM CEST, pa...@nvidia.com wrote: > >> > > >> >> From: Jiri Pirko > >> >> Sent: Monday, June 17, 2024 3:09 PM > >> >> > >> >> Mon, Jun 17, 2024 at 04:57:23AM CEST, pa...@nvidia.com wrote: > >> >> > > >> >> > > >> >> >> From: Jason Wang > >> >> >> Sent: Monday, June 17, 2024 7:18 AM > >> >> >> > >> >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: > >> >> >> > > >> >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >> >> >> > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> >> >> > >> Add new UAPI to support the mac address from vdpa tool > >> >> >> > >> Function > >> >> >> > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address > >> >> >> > >> from the vdpa tool and then set it to the device. > >> >> >> > >> > >> >> >> > >> The usage is: vdpa dev set name vdpa_name mac > >> >> >> > >> **:**:**:**:**:** > >> >> >> > > > >> >> >> > >Why don't you use devlink? > >> >> >> > > >> >> >> > Fair question. Why does vdpa-specific uapi even exist? To have > >> >> >> > driver-specific uapi Does not make any sense to me :/ > >> >> >> > >> >> >> It came with devlink first actually, but switched to a dedicated > >> >> >> uAPI. > >> >> >> > >> >> >> Parav(cced) may explain more here. > >> >> >> > >> >> >Devlink configures function level mac that applies to all protocol > >> >> >devices > >> >> (vdpa, rdma, netdev) etc. > >> >> >Additionally, vdpa device level mac can be different (an additional > >> >> >one) to > >> >> apply to only vdpa traffic. > >> >> >Hence dedicated uAPI was added. > >> >> > >> >> There is 1:1 relation between vdpa instance and devlink port, isn't it? > >> >> Then we have: > >> >>devlink port function set DEV/PORT_INDEX hw_addr ADDR > >> >> > >> >Above command is privilege command done by the hypervisor on the port > >> function. > >> >Vpda level setting the mac is similar to a function owner driver setting > >> >the > >> mac on the self netdev (even though devlink side has configured some mac > >> for > >> it). > >> >For example, > >> >$ ip link set dev wlan1 address 00:11:22:33:44:55 > >> > >> Hmm, under what sceratio exacly this is needed? > >The administrator on the host creating a vdpa device for the VM wants to > >configure the mac address for the VM. > >This administrator may not have the access to the devlink port function. > >Or he may just prefer a different MAC (theoretical case). > > Right, but that is not reason for new uapi but rather reason to alter > existing devlink model to have the "host side". We discussed this many > times. > > > > > >> I mean, the VM that has VDPA device can actually do that too. > >VM cannot do. Virtio spec do not allow modifying the mac address. > > I see. Any good reason to not allow that? > > > > > >> That is the actual function owner. > >vdpa is not mapping a whole VF to the VM. > >It is getting some synthetic PCI device composed using several software > >(kernel) and user space layers. > >so VM is not the function owner. > > Sure, but owner of the netdev side, to what the mac is related. That is > my point. I don't know what this discussion is about, at this point. For better or worse, vdpa gained interfaces for provisioning new devices. Yes the solution space was wide but it's been there for years so kind of too late to try and make people move to another interface for that. Having said that, vdpa interfaces are all built around virtio spec. Let's try to stick to that. -- MST
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Mon, Jun 17, 2024 at 01:48:02PM CEST, pa...@nvidia.com wrote: > >> From: Jiri Pirko >> Sent: Monday, June 17, 2024 5:10 PM >> >> Mon, Jun 17, 2024 at 11:44:53AM CEST, pa...@nvidia.com wrote: >> > >> >> From: Jiri Pirko >> >> Sent: Monday, June 17, 2024 3:09 PM >> >> >> >> Mon, Jun 17, 2024 at 04:57:23AM CEST, pa...@nvidia.com wrote: >> >> > >> >> > >> >> >> From: Jason Wang >> >> >> Sent: Monday, June 17, 2024 7:18 AM >> >> >> >> >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: >> >> >> > >> >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >> >> >> > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> >> >> > >> Add new UAPI to support the mac address from vdpa tool >> >> >> > >> Function >> >> >> > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address >> >> >> > >> from the vdpa tool and then set it to the device. >> >> >> > >> >> >> >> > >> The usage is: vdpa dev set name vdpa_name mac >> >> >> > >> **:**:**:**:**:** >> >> >> > > >> >> >> > >Why don't you use devlink? >> >> >> > >> >> >> > Fair question. Why does vdpa-specific uapi even exist? To have >> >> >> > driver-specific uapi Does not make any sense to me :/ >> >> >> >> >> >> It came with devlink first actually, but switched to a dedicated uAPI. >> >> >> >> >> >> Parav(cced) may explain more here. >> >> >> >> >> >Devlink configures function level mac that applies to all protocol >> >> >devices >> >> (vdpa, rdma, netdev) etc. >> >> >Additionally, vdpa device level mac can be different (an additional >> >> >one) to >> >> apply to only vdpa traffic. >> >> >Hence dedicated uAPI was added. >> >> >> >> There is 1:1 relation between vdpa instance and devlink port, isn't it? >> >> Then we have: >> >>devlink port function set DEV/PORT_INDEX hw_addr ADDR >> >> >> >Above command is privilege command done by the hypervisor on the port >> function. >> >Vpda level setting the mac is similar to a function owner driver setting the >> mac on the self netdev (even though devlink side has configured some mac for >> it). >> >For example, >> >$ ip link set dev wlan1 address 00:11:22:33:44:55 >> >> Hmm, under what sceratio exacly this is needed? >The administrator on the host creating a vdpa device for the VM wants to >configure the mac address for the VM. >This administrator may not have the access to the devlink port function. >Or he may just prefer a different MAC (theoretical case). Right, but that is not reason for new uapi but rather reason to alter existing devlink model to have the "host side". We discussed this many times. > >> I mean, the VM that has VDPA device can actually do that too. >VM cannot do. Virtio spec do not allow modifying the mac address. I see. Any good reason to not allow that? > >> That is the actual function owner. >vdpa is not mapping a whole VF to the VM. >It is getting some synthetic PCI device composed using several software >(kernel) and user space layers. >so VM is not the function owner. Sure, but owner of the netdev side, to what the mac is related. That is my point.
RE: [PATCH 1/2] vdpa: support set mac address from vdpa tool
> From: Jiri Pirko > Sent: Monday, June 17, 2024 5:10 PM > > Mon, Jun 17, 2024 at 11:44:53AM CEST, pa...@nvidia.com wrote: > > > >> From: Jiri Pirko > >> Sent: Monday, June 17, 2024 3:09 PM > >> > >> Mon, Jun 17, 2024 at 04:57:23AM CEST, pa...@nvidia.com wrote: > >> > > >> > > >> >> From: Jason Wang > >> >> Sent: Monday, June 17, 2024 7:18 AM > >> >> > >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: > >> >> > > >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >> >> > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> >> > >> Add new UAPI to support the mac address from vdpa tool > >> >> > >> Function > >> >> > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address > >> >> > >> from the vdpa tool and then set it to the device. > >> >> > >> > >> >> > >> The usage is: vdpa dev set name vdpa_name mac > >> >> > >> **:**:**:**:**:** > >> >> > > > >> >> > >Why don't you use devlink? > >> >> > > >> >> > Fair question. Why does vdpa-specific uapi even exist? To have > >> >> > driver-specific uapi Does not make any sense to me :/ > >> >> > >> >> It came with devlink first actually, but switched to a dedicated uAPI. > >> >> > >> >> Parav(cced) may explain more here. > >> >> > >> >Devlink configures function level mac that applies to all protocol > >> >devices > >> (vdpa, rdma, netdev) etc. > >> >Additionally, vdpa device level mac can be different (an additional > >> >one) to > >> apply to only vdpa traffic. > >> >Hence dedicated uAPI was added. > >> > >> There is 1:1 relation between vdpa instance and devlink port, isn't it? > >> Then we have: > >>devlink port function set DEV/PORT_INDEX hw_addr ADDR > >> > >Above command is privilege command done by the hypervisor on the port > function. > >Vpda level setting the mac is similar to a function owner driver setting the > mac on the self netdev (even though devlink side has configured some mac for > it). > >For example, > >$ ip link set dev wlan1 address 00:11:22:33:44:55 > > Hmm, under what sceratio exacly this is needed? The administrator on the host creating a vdpa device for the VM wants to configure the mac address for the VM. This administrator may not have the access to the devlink port function. Or he may just prefer a different MAC (theoretical case). > I mean, the VM that has VDPA device can actually do that too. VM cannot do. Virtio spec do not allow modifying the mac address. > That is the actual function owner. vdpa is not mapping a whole VF to the VM. It is getting some synthetic PCI device composed using several software (kernel) and user space layers. so VM is not the function owner.
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Mon, Jun 17, 2024 at 11:44:53AM CEST, pa...@nvidia.com wrote: > >> From: Jiri Pirko >> Sent: Monday, June 17, 2024 3:09 PM >> >> Mon, Jun 17, 2024 at 04:57:23AM CEST, pa...@nvidia.com wrote: >> > >> > >> >> From: Jason Wang >> >> Sent: Monday, June 17, 2024 7:18 AM >> >> >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: >> >> > >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >> >> > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> >> > >> Add new UAPI to support the mac address from vdpa tool Function >> >> > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address from >> >> > >> the vdpa tool and then set it to the device. >> >> > >> >> >> > >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** >> >> > > >> >> > >Why don't you use devlink? >> >> > >> >> > Fair question. Why does vdpa-specific uapi even exist? To have >> >> > driver-specific uapi Does not make any sense to me :/ >> >> >> >> It came with devlink first actually, but switched to a dedicated uAPI. >> >> >> >> Parav(cced) may explain more here. >> >> >> >Devlink configures function level mac that applies to all protocol devices >> (vdpa, rdma, netdev) etc. >> >Additionally, vdpa device level mac can be different (an additional one) to >> apply to only vdpa traffic. >> >Hence dedicated uAPI was added. >> >> There is 1:1 relation between vdpa instance and devlink port, isn't it? >> Then we have: >>devlink port function set DEV/PORT_INDEX hw_addr ADDR >> >Above command is privilege command done by the hypervisor on the port function. >Vpda level setting the mac is similar to a function owner driver setting the >mac on the self netdev (even though devlink side has configured some mac for >it). >For example, >$ ip link set dev wlan1 address 00:11:22:33:44:55 Hmm, under what sceratio exacly this is needed? I mean, the VM that has VDPA device can actually do that too. That is the actual function owner. > >> Which does exactly what you need, configure function hw address (mac). >> >> When you say VDPA traffic, do you suggest there might be VDPA instance and >> netdev running on the same VF in parallel. If yes, do we have 2 eswitch port >> representors to be separately used to steer the traffic? >> If no, how is that supposed to be working? >A eswitch may allow incoming and outgoing traffic from multiple mac addresses >left to the tc rules to decide. >It does not need two eswitch ports. Ugh.
RE: [PATCH 1/2] vdpa: support set mac address from vdpa tool
> From: Jiri Pirko > Sent: Monday, June 17, 2024 3:09 PM > > Mon, Jun 17, 2024 at 04:57:23AM CEST, pa...@nvidia.com wrote: > > > > > >> From: Jason Wang > >> Sent: Monday, June 17, 2024 7:18 AM > >> > >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: > >> > > >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >> > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> > >> Add new UAPI to support the mac address from vdpa tool Function > >> > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address from > >> > >> the vdpa tool and then set it to the device. > >> > >> > >> > >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > >> > > > >> > >Why don't you use devlink? > >> > > >> > Fair question. Why does vdpa-specific uapi even exist? To have > >> > driver-specific uapi Does not make any sense to me :/ > >> > >> It came with devlink first actually, but switched to a dedicated uAPI. > >> > >> Parav(cced) may explain more here. > >> > >Devlink configures function level mac that applies to all protocol devices > (vdpa, rdma, netdev) etc. > >Additionally, vdpa device level mac can be different (an additional one) to > apply to only vdpa traffic. > >Hence dedicated uAPI was added. > > There is 1:1 relation between vdpa instance and devlink port, isn't it? > Then we have: >devlink port function set DEV/PORT_INDEX hw_addr ADDR > Above command is privilege command done by the hypervisor on the port function. Vpda level setting the mac is similar to a function owner driver setting the mac on the self netdev (even though devlink side has configured some mac for it). For example, $ ip link set dev wlan1 address 00:11:22:33:44:55 > Which does exactly what you need, configure function hw address (mac). > > When you say VDPA traffic, do you suggest there might be VDPA instance and > netdev running on the same VF in parallel. If yes, do we have 2 eswitch port > representors to be separately used to steer the traffic? > If no, how is that supposed to be working? A eswitch may allow incoming and outgoing traffic from multiple mac addresses left to the tc rules to decide. It does not need two eswitch ports.
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Mon, Jun 17, 2024 at 04:57:23AM CEST, pa...@nvidia.com wrote: > > >> From: Jason Wang >> Sent: Monday, June 17, 2024 7:18 AM >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: >> > >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >> > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> > >> Add new UAPI to support the mac address from vdpa tool Function >> > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address from the >> > >> vdpa tool and then set it to the device. >> > >> >> > >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** >> > > >> > >Why don't you use devlink? >> > >> > Fair question. Why does vdpa-specific uapi even exist? To have >> > driver-specific uapi Does not make any sense to me :/ >> >> It came with devlink first actually, but switched to a dedicated uAPI. >> >> Parav(cced) may explain more here. >> >Devlink configures function level mac that applies to all protocol devices >(vdpa, rdma, netdev) etc. >Additionally, vdpa device level mac can be different (an additional one) to >apply to only vdpa traffic. >Hence dedicated uAPI was added. There is 1:1 relation between vdpa instance and devlink port, isn't it? Then we have: devlink port function set DEV/PORT_INDEX hw_addr ADDR Which does exactly what you need, configure function hw address (mac). When you say VDPA traffic, do you suggest there might be VDPA instance and netdev running on the same VF in parallel. If yes, do we have 2 eswitch port representors to be separately used to steer the traffic? If no, how is that supposed to be working?
RE: [PATCH 1/2] vdpa: support set mac address from vdpa tool
> From: Jason Wang > Sent: Monday, June 17, 2024 7:18 AM > > On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: > > > > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > > >> Add new UAPI to support the mac address from vdpa tool Function > > >> vdpa_nl_cmd_dev_config_set_doit() will get the MAC address from the > > >> vdpa tool and then set it to the device. > > >> > > >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > > > > > >Why don't you use devlink? > > > > Fair question. Why does vdpa-specific uapi even exist? To have > > driver-specific uapi Does not make any sense to me :/ > > It came with devlink first actually, but switched to a dedicated uAPI. > > Parav(cced) may explain more here. > Devlink configures function level mac that applies to all protocol devices (vdpa, rdma, netdev) etc. Additionally, vdpa device level mac can be different (an additional one) to apply to only vdpa traffic. Hence dedicated uAPI was added.
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko wrote: > > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> Add new UAPI to support the mac address from vdpa tool > >> Function vdpa_nl_cmd_dev_config_set_doit() will get the > >> MAC address from the vdpa tool and then set it to the device. > >> > >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > > > >Why don't you use devlink? > > Fair question. Why does vdpa-specific uapi even exist? To have > driver-specific uapi Does not make any sense to me :/ It came with devlink first actually, but switched to a dedicated uAPI. Parav(cced) may explain more here. Thanks >
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Thu, Jun 13, 2024 at 09:50:54AM CEST, m...@redhat.com wrote: >On Thu, Jun 13, 2024 at 09:21:07AM +0200, Jiri Pirko wrote: >> Thu, Jun 13, 2024 at 08:49:25AM CEST, m...@redhat.com wrote: >> >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: >> >> Wed, Jun 12, 2024 at 09:15:44AM CEST, m...@redhat.com wrote: >> >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: >> >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >> >> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> >> >> >> Add new UAPI to support the mac address from vdpa tool >> >> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the >> >> >> >> MAC address from the vdpa tool and then set it to the device. >> >> >> >> >> >> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** >> >> >> > >> >> >> >Why don't you use devlink? >> >> >> >> >> >> Fair question. Why does vdpa-specific uapi even exist? To have >> >> >> driver-specific uapi Does not make any sense to me :/ >> >> > >> >> >I am not sure which uapi do you refer to? The one this patch proposes or >> >> >the existing one? >> >> >> >> Sure, I'm sure pointing out, that devlink should have been the answer >> >> instead of vdpa netlink introduction. That ship is sailed, >> > >> >> now we have >> >> unfortunate api duplication which leads to questions like Jakub's one. >> >> That's all :/ >> > >> > >> > >> >Yea there's no point to argue now, there were arguments this and that >> >way. I don't think we currently have a lot >> >of duplication, do we? >> >> True. I think it would be good to establish guidelines for api >> extensions in this area. >> >> > >> >-- >> >MST >> > > > >Guidelines are good, are there existing examples of such guidelines in >Linux to follow though? Specifically after reviewing this some more, I Documentation directory in general. >think what Cindy is trying to do is actually provisioning as opposed to >programming. > >-- >MST >
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Thu, Jun 13, 2024 at 2:59 PM Michael S. Tsirkin wrote:
>
> On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> > Add new UAPI to support the mac address from vdpa tool
> > Function vdpa_nl_cmd_dev_config_set_doit() will get the
> > MAC address from the vdpa tool and then set it to the device.
> >
> > The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
> >
> > Here is sample:
> > root@L1# vdpa -jp dev config show vdpa0
> > {
> > "config": {
> > "vdpa0": {
> > "mac": "82:4d:e9:5d:d7:e6",
> > "link ": "up",
> > "link_announce ": false,
> > "mtu": 1500
> > }
> > }
> > }
> >
> > root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
> >
> > root@L1# vdpa -jp dev config show vdpa0
> > {
> > "config": {
> > "vdpa0": {
> > "mac": "00:11:22:33:44:55",
> > "link ": "up",
> > "link_announce ": false,
> > "mtu": 1500
> > }
> > }
> > }
> >
> > Signed-off-by: Cindy Lu
>
>
>
> I think actually the idea of allowing provisioning
> by specifying config of the device is actually valid.
> However
> - the name SET_CONFIG makes people think this allows
> writing even when e.g. device is assigned to guest
> - having the internal api be mac specific is weird
>
> Shouldn't config be an attribute maybe, not a new command?
>
Got it. Thanks, Michael. I will change this.
Thanks
Cindy
>
> > ---
> > drivers/vdpa/vdpa.c | 71 +++
> > include/linux/vdpa.h | 2 ++
> > include/uapi/linux/vdpa.h | 1 +
> > 3 files changed, 74 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index a7612e0783b3..347ae6e7749d 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct
> > sk_buff *skb, struct genl_info
> > return err;
> > }
> >
> > +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> > +struct genl_info *info)
> > +{
> > + struct vdpa_dev_set_config set_config = {};
> > + struct nlattr **nl_attrs = info->attrs;
> > + struct vdpa_mgmt_dev *mdev;
> > + const u8 *macaddr;
> > + const char *name;
> > + int err = 0;
> > + struct device *dev;
> > + struct vdpa_device *vdev;
> > +
> > + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > + return -EINVAL;
> > +
> > + name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > +
> > + down_write(&vdpa_dev_lock);
> > + dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
> > + if (!dev) {
> > + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> > + err = -ENODEV;
> > + goto dev_err;
> > + }
> > + vdev = container_of(dev, struct vdpa_device, dev);
> > + if (!vdev->mdev) {
> > + NL_SET_ERR_MSG_MOD(
> > + info->extack,
> > + "Fail to find the specified management device");
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
> > + mdev = vdev->mdev;
> > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> > + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "Missing features 0x%llx for provided
> > attributes",
> > + BIT_ULL(VIRTIO_NET_F_MAC));
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
> > + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> > + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> > + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> > + if (mdev->ops->set_mac) {
> > + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "%s device not support set mac address ",
> > name);
> > + }
> > +
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > +"%s device not support this config ",
> > +name);
> > + }
> > +
> > +mdev_err:
> > + put_device(dev);
> > +dev_err:
> > + up_write(&vdpa_dev_lock);
> > + return err;
> > +}
> > +
> > static int vdpa_dev_config_dump(struct device *dev, void *data)
> > {
> > struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> > @@ -1285,6 +1351,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
> > .doit = vdpa_nl_cmd_dev_stats_get_doit,
> > .flags = GENL_ADMIN_PERM,
> > },
> > + {
> > + .cmd = VDPA_CMD_DEV
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Thu, Jun 13, 2024 at 09:21:07AM +0200, Jiri Pirko wrote: > Thu, Jun 13, 2024 at 08:49:25AM CEST, m...@redhat.com wrote: > >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: > >> Wed, Jun 12, 2024 at 09:15:44AM CEST, m...@redhat.com wrote: > >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: > >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> >> >> Add new UAPI to support the mac address from vdpa tool > >> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the > >> >> >> MAC address from the vdpa tool and then set it to the device. > >> >> >> > >> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > >> >> > > >> >> >Why don't you use devlink? > >> >> > >> >> Fair question. Why does vdpa-specific uapi even exist? To have > >> >> driver-specific uapi Does not make any sense to me :/ > >> > > >> >I am not sure which uapi do you refer to? The one this patch proposes or > >> >the existing one? > >> > >> Sure, I'm sure pointing out, that devlink should have been the answer > >> instead of vdpa netlink introduction. That ship is sailed, > > > >> now we have > >> unfortunate api duplication which leads to questions like Jakub's one. > >> That's all :/ > > > > > > > >Yea there's no point to argue now, there were arguments this and that > >way. I don't think we currently have a lot > >of duplication, do we? > > True. I think it would be good to establish guidelines for api > extensions in this area. > > > > >-- > >MST > > Guidelines are good, are there existing examples of such guidelines in Linux to follow though? Specifically after reviewing this some more, I think what Cindy is trying to do is actually provisioning as opposed to programming. -- MST
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Thu, Jun 13, 2024 at 08:49:25AM CEST, m...@redhat.com wrote: >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: >> Wed, Jun 12, 2024 at 09:15:44AM CEST, m...@redhat.com wrote: >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> >> >> Add new UAPI to support the mac address from vdpa tool >> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the >> >> >> MAC address from the vdpa tool and then set it to the device. >> >> >> >> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** >> >> > >> >> >Why don't you use devlink? >> >> >> >> Fair question. Why does vdpa-specific uapi even exist? To have >> >> driver-specific uapi Does not make any sense to me :/ >> > >> >I am not sure which uapi do you refer to? The one this patch proposes or >> >the existing one? >> >> Sure, I'm sure pointing out, that devlink should have been the answer >> instead of vdpa netlink introduction. That ship is sailed, > >> now we have >> unfortunate api duplication which leads to questions like Jakub's one. >> That's all :/ > > > >Yea there's no point to argue now, there were arguments this and that >way. I don't think we currently have a lot >of duplication, do we? True. I think it would be good to establish guidelines for api extensions in this area. > >-- >MST >
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> Add new UAPI to support the mac address from vdpa tool
> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> MAC address from the vdpa tool and then set it to the device.
>
> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>
> Here is sample:
> root@L1# vdpa -jp dev config show vdpa0
> {
> "config": {
> "vdpa0": {
> "mac": "82:4d:e9:5d:d7:e6",
> "link ": "up",
> "link_announce ": false,
> "mtu": 1500
> }
> }
> }
>
> root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
>
> root@L1# vdpa -jp dev config show vdpa0
> {
> "config": {
> "vdpa0": {
> "mac": "00:11:22:33:44:55",
> "link ": "up",
> "link_announce ": false,
> "mtu": 1500
> }
> }
> }
>
> Signed-off-by: Cindy Lu
I think actually the idea of allowing provisioning
by specifying config of the device is actually valid.
However
- the name SET_CONFIG makes people think this allows
writing even when e.g. device is assigned to guest
- having the internal api be mac specific is weird
Shouldn't config be an attribute maybe, not a new command?
> ---
> drivers/vdpa/vdpa.c | 71 +++
> include/linux/vdpa.h | 2 ++
> include/uapi/linux/vdpa.h | 1 +
> 3 files changed, 74 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index a7612e0783b3..347ae6e7749d 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct
> sk_buff *skb, struct genl_info
> return err;
> }
>
> +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> +struct genl_info *info)
> +{
> + struct vdpa_dev_set_config set_config = {};
> + struct nlattr **nl_attrs = info->attrs;
> + struct vdpa_mgmt_dev *mdev;
> + const u8 *macaddr;
> + const char *name;
> + int err = 0;
> + struct device *dev;
> + struct vdpa_device *vdev;
> +
> + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> + return -EINVAL;
> +
> + name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +
> + down_write(&vdpa_dev_lock);
> + dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
> + if (!dev) {
> + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> + err = -ENODEV;
> + goto dev_err;
> + }
> + vdev = container_of(dev, struct vdpa_device, dev);
> + if (!vdev->mdev) {
> + NL_SET_ERR_MSG_MOD(
> + info->extack,
> + "Fail to find the specified management device");
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + mdev = vdev->mdev;
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "Missing features 0x%llx for provided
> attributes",
> + BIT_ULL(VIRTIO_NET_F_MAC));
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + if (mdev->ops->set_mac) {
> + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "%s device not support set mac address ", name);
> + }
> +
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"%s device not support this config ",
> +name);
> + }
> +
> +mdev_err:
> + put_device(dev);
> +dev_err:
> + up_write(&vdpa_dev_lock);
> + return err;
> +}
> +
> static int vdpa_dev_config_dump(struct device *dev, void *data)
> {
> struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> @@ -1285,6 +1351,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
> .doit = vdpa_nl_cmd_dev_stats_get_doit,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = VDPA_CMD_DEV_CONFIG_SET,
> + .doit = vdpa_nl_cmd_dev_config_set_doit,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index db15ac07f8a6..c97f4f1da753 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -581,6 +581,8 @@ struct vdp
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: > Wed, Jun 12, 2024 at 09:15:44AM CEST, m...@redhat.com wrote: > >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: > >> Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> >> Add new UAPI to support the mac address from vdpa tool > >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the > >> >> MAC address from the vdpa tool and then set it to the device. > >> >> > >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > >> > > >> >Why don't you use devlink? > >> > >> Fair question. Why does vdpa-specific uapi even exist? To have > >> driver-specific uapi Does not make any sense to me :/ > > > >I am not sure which uapi do you refer to? The one this patch proposes or > >the existing one? > > Sure, I'm sure pointing out, that devlink should have been the answer > instead of vdpa netlink introduction. That ship is sailed, > now we have > unfortunate api duplication which leads to questions like Jakub's one. > That's all :/ Yea there's no point to argue now, there were arguments this and that way. I don't think we currently have a lot of duplication, do we? -- MST
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Wed, Jun 12, 2024 at 3:12 PM Michael S. Tsirkin wrote:
>
> On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> > Add new UAPI to support the mac address from vdpa tool
>
> The patch does not do what commit log says.
> Instead there's an internal API to set mac and
> a UAPI to write into config space.
>
thanks, Michael, I will rewrite this part
Thanks
cindy
> > Function vdpa_nl_cmd_dev_config_set_doit() will get the
> > MAC address from the vdpa tool and then set it to the device.
> >
> > The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
> >
> > Here is sample:
> > root@L1# vdpa -jp dev config show vdpa0
> > {
> > "config": {
> > "vdpa0": {
> > "mac": "82:4d:e9:5d:d7:e6",
> > "link ": "up",
> > "link_announce ": false,
> > "mtu": 1500
> > }
> > }
> > }
> >
> > root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
> >
> > root@L1# vdpa -jp dev config show vdpa0
> > {
> > "config": {
> > "vdpa0": {
> > "mac": "00:11:22:33:44:55",
> > "link ": "up",
> > "link_announce ": false,
> > "mtu": 1500
> > }
> > }
> > }
> >
> > Signed-off-by: Cindy Lu
> > ---
> > drivers/vdpa/vdpa.c | 71 +++
> > include/linux/vdpa.h | 2 ++
> > include/uapi/linux/vdpa.h | 1 +
> > 3 files changed, 74 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index a7612e0783b3..347ae6e7749d 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct
> > sk_buff *skb, struct genl_info
> > return err;
> > }
> >
> > +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> > +struct genl_info *info)
> > +{
> > + struct vdpa_dev_set_config set_config = {};
> > + struct nlattr **nl_attrs = info->attrs;
> > + struct vdpa_mgmt_dev *mdev;
> > + const u8 *macaddr;
> > + const char *name;
> > + int err = 0;
> > + struct device *dev;
> > + struct vdpa_device *vdev;
> > +
> > + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > + return -EINVAL;
> > +
> > + name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > +
> > + down_write(&vdpa_dev_lock);
> > + dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
> > + if (!dev) {
> > + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> > + err = -ENODEV;
> > + goto dev_err;
> > + }
> > + vdev = container_of(dev, struct vdpa_device, dev);
> > + if (!vdev->mdev) {
> > + NL_SET_ERR_MSG_MOD(
> > + info->extack,
> > + "Fail to find the specified management device");
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
> > + mdev = vdev->mdev;
> > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> > + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
>
>
> Seems to poke at a device without even making sure it's a network
> device.
>
sure ,will add the check
Thansk
cindy
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "Missing features 0x%llx for provided
> > attributes",
> > + BIT_ULL(VIRTIO_NET_F_MAC));
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
> > + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> > + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> > + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> > + if (mdev->ops->set_mac) {
> > + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "%s device not support set mac address ",
> > name);
> > + }
> > +
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > +"%s device not support this config ",
> > +name);
> > + }
> > +
> > +mdev_err:
> > + put_device(dev);
> > +dev_err:
> > + up_write(&vdpa_dev_lock);
> > + return err;
> > +}
> > +
> > static int vdpa_dev_config_dump(struct device *dev, void *data)
> > {
> > struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> > @@ -1285,6 +1351,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
> > .doit = vdpa_nl_cmd_dev_stats_get_doit,
> > .flags = GENL_ADMIN_PERM,
> > },
> > + {
> > + .cmd = VDPA_CMD_DEV_CONFIG_SET,
> > + .doit = vdpa_nl_cmd_dev_config_set_doit,
> > + .flags = G
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Wed, Jun 12, 2024 at 09:15:44AM CEST, m...@redhat.com wrote: >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: >> Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> >> Add new UAPI to support the mac address from vdpa tool >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the >> >> MAC address from the vdpa tool and then set it to the device. >> >> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** >> > >> >Why don't you use devlink? >> >> Fair question. Why does vdpa-specific uapi even exist? To have >> driver-specific uapi Does not make any sense to me :/ > >I am not sure which uapi do you refer to? The one this patch proposes or >the existing one? Sure, I'm sure pointing out, that devlink should have been the answer instead of vdpa netlink introduction. That ship is sailed, now we have unfortunate api duplication which leads to questions like Jakub's one. That's all :/
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: > Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: > >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > >> Add new UAPI to support the mac address from vdpa tool > >> Function vdpa_nl_cmd_dev_config_set_doit() will get the > >> MAC address from the vdpa tool and then set it to the device. > >> > >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > > > >Why don't you use devlink? > > Fair question. Why does vdpa-specific uapi even exist? To have > driver-specific uapi Does not make any sense to me :/ I am not sure which uapi do you refer to? The one this patch proposes or the existing one? -- MST
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> Add new UAPI to support the mac address from vdpa tool
The patch does not do what commit log says.
Instead there's an internal API to set mac and
a UAPI to write into config space.
> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> MAC address from the vdpa tool and then set it to the device.
>
> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>
> Here is sample:
> root@L1# vdpa -jp dev config show vdpa0
> {
> "config": {
> "vdpa0": {
> "mac": "82:4d:e9:5d:d7:e6",
> "link ": "up",
> "link_announce ": false,
> "mtu": 1500
> }
> }
> }
>
> root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
>
> root@L1# vdpa -jp dev config show vdpa0
> {
> "config": {
> "vdpa0": {
> "mac": "00:11:22:33:44:55",
> "link ": "up",
> "link_announce ": false,
> "mtu": 1500
> }
> }
> }
>
> Signed-off-by: Cindy Lu
> ---
> drivers/vdpa/vdpa.c | 71 +++
> include/linux/vdpa.h | 2 ++
> include/uapi/linux/vdpa.h | 1 +
> 3 files changed, 74 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index a7612e0783b3..347ae6e7749d 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct
> sk_buff *skb, struct genl_info
> return err;
> }
>
> +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> +struct genl_info *info)
> +{
> + struct vdpa_dev_set_config set_config = {};
> + struct nlattr **nl_attrs = info->attrs;
> + struct vdpa_mgmt_dev *mdev;
> + const u8 *macaddr;
> + const char *name;
> + int err = 0;
> + struct device *dev;
> + struct vdpa_device *vdev;
> +
> + if (!info->attrs[VDPA_ATTR_DEV_NAME])
> + return -EINVAL;
> +
> + name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +
> + down_write(&vdpa_dev_lock);
> + dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
> + if (!dev) {
> + NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> + err = -ENODEV;
> + goto dev_err;
> + }
> + vdev = container_of(dev, struct vdpa_device, dev);
> + if (!vdev->mdev) {
> + NL_SET_ERR_MSG_MOD(
> + info->extack,
> + "Fail to find the specified management device");
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + mdev = vdev->mdev;
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
Seems to poke at a device without even making sure it's a network
device.
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "Missing features 0x%llx for provided
> attributes",
> + BIT_ULL(VIRTIO_NET_F_MAC));
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + if (mdev->ops->set_mac) {
> + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "%s device not support set mac address ", name);
> + }
> +
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"%s device not support this config ",
> +name);
> + }
> +
> +mdev_err:
> + put_device(dev);
> +dev_err:
> + up_write(&vdpa_dev_lock);
> + return err;
> +}
> +
> static int vdpa_dev_config_dump(struct device *dev, void *data)
> {
> struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> @@ -1285,6 +1351,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
> .doit = vdpa_nl_cmd_dev_stats_get_doit,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = VDPA_CMD_DEV_CONFIG_SET,
> + .doit = vdpa_nl_cmd_dev_config_set_doit,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index db15ac07f8a6..c97f4f1da753 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -581,6 +581,8 @@ struct vdpa_mgmtdev_ops {
> int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name,
> const struct vdpa_dev_
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
Wed, Jun 12, 2024 at 03:58:10AM CEST, k...@kernel.org wrote: >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: >> Add new UAPI to support the mac address from vdpa tool >> Function vdpa_nl_cmd_dev_config_set_doit() will get the >> MAC address from the vdpa tool and then set it to the device. >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** > >Why don't you use devlink? Fair question. Why does vdpa-specific uapi even exist? To have driver-specific uapi Does not make any sense to me :/
Re: [PATCH 1/2] vdpa: support set mac address from vdpa tool
On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote: > Add new UAPI to support the mac address from vdpa tool > Function vdpa_nl_cmd_dev_config_set_doit() will get the > MAC address from the vdpa tool and then set it to the device. > > The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:** Why don't you use devlink?
[PATCH 1/2] vdpa: support set mac address from vdpa tool
Add new UAPI to support the mac address from vdpa tool
Function vdpa_nl_cmd_dev_config_set_doit() will get the
MAC address from the vdpa tool and then set it to the device.
The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
Here is sample:
root@L1# vdpa -jp dev config show vdpa0
{
"config": {
"vdpa0": {
"mac": "82:4d:e9:5d:d7:e6",
"link ": "up",
"link_announce ": false,
"mtu": 1500
}
}
}
root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
root@L1# vdpa -jp dev config show vdpa0
{
"config": {
"vdpa0": {
"mac": "00:11:22:33:44:55",
"link ": "up",
"link_announce ": false,
"mtu": 1500
}
}
}
Signed-off-by: Cindy Lu
---
drivers/vdpa/vdpa.c | 71 +++
include/linux/vdpa.h | 2 ++
include/uapi/linux/vdpa.h | 1 +
3 files changed, 74 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index a7612e0783b3..347ae6e7749d 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct
sk_buff *skb, struct genl_info
return err;
}
+static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct vdpa_dev_set_config set_config = {};
+ struct nlattr **nl_attrs = info->attrs;
+ struct vdpa_mgmt_dev *mdev;
+ const u8 *macaddr;
+ const char *name;
+ int err = 0;
+ struct device *dev;
+ struct vdpa_device *vdev;
+
+ if (!info->attrs[VDPA_ATTR_DEV_NAME])
+ return -EINVAL;
+
+ name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
+
+ down_write(&vdpa_dev_lock);
+ dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
+ if (!dev) {
+ NL_SET_ERR_MSG_MOD(info->extack, "device not found");
+ err = -ENODEV;
+ goto dev_err;
+ }
+ vdev = container_of(dev, struct vdpa_device, dev);
+ if (!vdev->mdev) {
+ NL_SET_ERR_MSG_MOD(
+ info->extack,
+ "Fail to find the specified management device");
+ err = -EINVAL;
+ goto mdev_err;
+ }
+ mdev = vdev->mdev;
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
+ if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
+ NL_SET_ERR_MSG_FMT_MOD(
+ info->extack,
+ "Missing features 0x%llx for provided
attributes",
+ BIT_ULL(VIRTIO_NET_F_MAC));
+ err = -EINVAL;
+ goto mdev_err;
+ }
+ macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
+ memcpy(set_config.net.mac, macaddr, ETH_ALEN);
+ set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+ if (mdev->ops->set_mac) {
+ err = mdev->ops->set_mac(mdev, vdev, &set_config);
+ } else {
+ NL_SET_ERR_MSG_FMT_MOD(
+ info->extack,
+ "%s device not support set mac address ", name);
+ }
+
+ } else {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "%s device not support this config ",
+ name);
+ }
+
+mdev_err:
+ put_device(dev);
+dev_err:
+ up_write(&vdpa_dev_lock);
+ return err;
+}
+
static int vdpa_dev_config_dump(struct device *dev, void *data)
{
struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
@@ -1285,6 +1351,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
.doit = vdpa_nl_cmd_dev_stats_get_doit,
.flags = GENL_ADMIN_PERM,
},
+ {
+ .cmd = VDPA_CMD_DEV_CONFIG_SET,
+ .doit = vdpa_nl_cmd_dev_config_set_doit,
+ .flags = GENL_ADMIN_PERM,
+ },
};
static struct genl_family vdpa_nl_family __ro_after_init = {
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index db15ac07f8a6..c97f4f1da753 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -581,6 +581,8 @@ struct vdpa_mgmtdev_ops {
int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name,
const struct vdpa_dev_set_config *config);
void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev);
+ int (*set_mac)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
+ const struct vdpa_dev_set_config *config);
};
/**
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 54b649ab0f22..53f249fb26bc 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/
