| Message ID | 20230630003609.28527-3-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pds_vdpa: mac, reset, and irq updates | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Jun 30, 2023 at 8:36 AM Shannon Nelson <shannon.nelson@amd.com> wrote: > > Our driver sets a mac if the HW is 00:..:00 so we need to be sure to > advertise VIRTIO_NET_F_MAC even if the HW doesn't. We also need to be > sure that virtio_net sees the VIRTIO_NET_F_MAC and doesn't rewrite the > mac address that a user may have set with the vdpa utility. > > After reading the hw_feature bits, add the VIRTIO_NET_F_MAC to the driver's > supported_features and use that for reporting what is available. If the > HW is not advertising it, be sure to strip the VIRTIO_NET_F_MAC before > finishing the feature negotiation. If the user specifies a device_features > bitpattern in the vdpa utility without the VIRTIO_NET_F_MAC set, then > don't set the mac. > > Fixes: 151cc834f3dd ("pds_vdpa: add support for vdpa and vdpamgmt interfaces") > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > Reviewed-by: Brett Creeley <brett.creeley@amd.com> > --- > drivers/vdpa/pds/vdpa_dev.c | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c > index e2e99bb0be2b..5e761d625ef3 100644 > --- a/drivers/vdpa/pds/vdpa_dev.c > +++ b/drivers/vdpa/pds/vdpa_dev.c > @@ -316,6 +316,7 @@ static int pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 featur > { > struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev); > struct device *dev = &pdsv->vdpa_dev.dev; > + u64 requested_features; > u64 driver_features; > u64 nego_features; > u64 missing; > @@ -325,18 +326,24 @@ static int pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 featur > return -EOPNOTSUPP; > } > > + /* save original request for debugfs */ > pdsv->req_features = features; > + requested_features = features; > + > + /* if we're faking the F_MAC, strip it from features to be negotiated */ > + driver_features = pds_vdpa_get_driver_features(vdpa_dev); > + if (!(driver_features & BIT_ULL(VIRTIO_NET_F_MAC))) > + requested_features &= ~BIT_ULL(VIRTIO_NET_F_MAC); I'm not sure I understand here, assuming we are doing feature negotiation here. In this case driver_features we read should be zero? Or did you actually mean device_features here? Thanks > > /* Check for valid feature bits */ > - nego_features = features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features); > - missing = pdsv->req_features & ~nego_features; > + nego_features = requested_features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features); > + missing = requested_features & ~nego_features; > if (missing) { > dev_err(dev, "Can't support all requested features in %#llx, missing %#llx features\n", > pdsv->req_features, missing); > return -EOPNOTSUPP; > } > > - driver_features = pds_vdpa_get_driver_features(vdpa_dev); > dev_dbg(dev, "%s: %#llx => %#llx\n", > __func__, driver_features, nego_features); > > @@ -564,7 +571,7 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > > if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) { > u64 unsupp_features = > - add_config->device_features & ~mgmt->supported_features; > + add_config->device_features & ~pdsv->supported_features; > > if (unsupp_features) { > dev_err(dev, "Unsupported features: %#llx\n", unsupp_features); > @@ -615,7 +622,8 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > } > > /* Set a mac, either from the user config if provided > - * or set a random mac if default is 00:..:00 > + * or use the device's mac if not 00:..:00 > + * or set a random mac > */ > if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) { > ether_addr_copy(pdsv->mac, add_config->net.mac); > @@ -624,7 +632,8 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > > vc = pdsv->vdpa_aux->vd_mdev.device; > memcpy_fromio(pdsv->mac, vc->mac, sizeof(pdsv->mac)); > - if (is_zero_ether_addr(pdsv->mac)) { > + if (is_zero_ether_addr(pdsv->mac) && > + (pdsv->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) { > eth_random_addr(pdsv->mac); > dev_info(dev, "setting random mac %pM\n", pdsv->mac); > } > @@ -752,6 +761,10 @@ int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux) > mgmt->id_table = pds_vdpa_id_table; > mgmt->device = dev; > mgmt->supported_features = le64_to_cpu(vdpa_aux->ident.hw_features); > + > + /* advertise F_MAC even if the device doesn't */ > + mgmt->supported_features |= BIT_ULL(VIRTIO_NET_F_MAC); > + > mgmt->config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR); > mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP); > mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES); > -- > 2.17.1 >
On 7/7/23 12:44 AM, Jason Wang wrote: > > On Fri, Jun 30, 2023 at 8:36 AM Shannon Nelson <shannon.nelson@amd.com> wrote: >> >> Our driver sets a mac if the HW is 00:..:00 so we need to be sure to >> advertise VIRTIO_NET_F_MAC even if the HW doesn't. We also need to be >> sure that virtio_net sees the VIRTIO_NET_F_MAC and doesn't rewrite the >> mac address that a user may have set with the vdpa utility. >> >> After reading the hw_feature bits, add the VIRTIO_NET_F_MAC to the driver's >> supported_features and use that for reporting what is available. If the >> HW is not advertising it, be sure to strip the VIRTIO_NET_F_MAC before >> finishing the feature negotiation. If the user specifies a device_features >> bitpattern in the vdpa utility without the VIRTIO_NET_F_MAC set, then >> don't set the mac. >> >> Fixes: 151cc834f3dd ("pds_vdpa: add support for vdpa and vdpamgmt interfaces") >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> Reviewed-by: Brett Creeley <brett.creeley@amd.com> >> --- >> drivers/vdpa/pds/vdpa_dev.c | 25 +++++++++++++++++++------ >> 1 file changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c >> index e2e99bb0be2b..5e761d625ef3 100644 >> --- a/drivers/vdpa/pds/vdpa_dev.c >> +++ b/drivers/vdpa/pds/vdpa_dev.c >> @@ -316,6 +316,7 @@ static int pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 featur >> { >> struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev); >> struct device *dev = &pdsv->vdpa_dev.dev; >> + u64 requested_features; >> u64 driver_features; >> u64 nego_features; >> u64 missing; >> @@ -325,18 +326,24 @@ static int pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 featur >> return -EOPNOTSUPP; >> } >> >> + /* save original request for debugfs */ >> pdsv->req_features = features; >> + requested_features = features; >> + >> + /* if we're faking the F_MAC, strip it from features to be negotiated */ >> + driver_features = pds_vdpa_get_driver_features(vdpa_dev); >> + if (!(driver_features & BIT_ULL(VIRTIO_NET_F_MAC))) >> + requested_features &= ~BIT_ULL(VIRTIO_NET_F_MAC); > > I'm not sure I understand here, assuming we are doing feature > negotiation here. In this case driver_features we read should be zero? > Or did you actually mean device_features here? Yes, this needs to be cleared up. I'll address it in v2. sln > > Thanks > >> >> /* Check for valid feature bits */ >> - nego_features = features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features); >> - missing = pdsv->req_features & ~nego_features; >> + nego_features = requested_features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features); >> + missing = requested_features & ~nego_features; >> if (missing) { >> dev_err(dev, "Can't support all requested features in %#llx, missing %#llx features\n", >> pdsv->req_features, missing); >> return -EOPNOTSUPP; >> } >> >> - driver_features = pds_vdpa_get_driver_features(vdpa_dev); >> dev_dbg(dev, "%s: %#llx => %#llx\n", >> __func__, driver_features, nego_features); >> >> @@ -564,7 +571,7 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, >> >> if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) { >> u64 unsupp_features = >> - add_config->device_features & ~mgmt->supported_features; >> + add_config->device_features & ~pdsv->supported_features; >> >> if (unsupp_features) { >> dev_err(dev, "Unsupported features: %#llx\n", unsupp_features); >> @@ -615,7 +622,8 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, >> } >> >> /* Set a mac, either from the user config if provided >> - * or set a random mac if default is 00:..:00 >> + * or use the device's mac if not 00:..:00 >> + * or set a random mac >> */ >> if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) { >> ether_addr_copy(pdsv->mac, add_config->net.mac); >> @@ -624,7 +632,8 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, >> >> vc = pdsv->vdpa_aux->vd_mdev.device; >> memcpy_fromio(pdsv->mac, vc->mac, sizeof(pdsv->mac)); >> - if (is_zero_ether_addr(pdsv->mac)) { >> + if (is_zero_ether_addr(pdsv->mac) && >> + (pdsv->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) { >> eth_random_addr(pdsv->mac); >> dev_info(dev, "setting random mac %pM\n", pdsv->mac); >> } >> @@ -752,6 +761,10 @@ int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux) >> mgmt->id_table = pds_vdpa_id_table; >> mgmt->device = dev; >> mgmt->supported_features = le64_to_cpu(vdpa_aux->ident.hw_features); >> + >> + /* advertise F_MAC even if the device doesn't */ >> + mgmt->supported_features |= BIT_ULL(VIRTIO_NET_F_MAC); >> + >> mgmt->config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR); >> mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP); >> mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES); >> -- >> 2.17.1 >> >
diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c index e2e99bb0be2b..5e761d625ef3 100644 --- a/drivers/vdpa/pds/vdpa_dev.c +++ b/drivers/vdpa/pds/vdpa_dev.c @@ -316,6 +316,7 @@ static int pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 featur { struct pds_vdpa_device *pdsv = vdpa_to_pdsv(vdpa_dev); struct device *dev = &pdsv->vdpa_dev.dev; + u64 requested_features; u64 driver_features; u64 nego_features; u64 missing; @@ -325,18 +326,24 @@ static int pds_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 featur return -EOPNOTSUPP; } + /* save original request for debugfs */ pdsv->req_features = features; + requested_features = features; + + /* if we're faking the F_MAC, strip it from features to be negotiated */ + driver_features = pds_vdpa_get_driver_features(vdpa_dev); + if (!(driver_features & BIT_ULL(VIRTIO_NET_F_MAC))) + requested_features &= ~BIT_ULL(VIRTIO_NET_F_MAC); /* Check for valid feature bits */ - nego_features = features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features); - missing = pdsv->req_features & ~nego_features; + nego_features = requested_features & le64_to_cpu(pdsv->vdpa_aux->ident.hw_features); + missing = requested_features & ~nego_features; if (missing) { dev_err(dev, "Can't support all requested features in %#llx, missing %#llx features\n", pdsv->req_features, missing); return -EOPNOTSUPP; } - driver_features = pds_vdpa_get_driver_features(vdpa_dev); dev_dbg(dev, "%s: %#llx => %#llx\n", __func__, driver_features, nego_features); @@ -564,7 +571,7 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) { u64 unsupp_features = - add_config->device_features & ~mgmt->supported_features; + add_config->device_features & ~pdsv->supported_features; if (unsupp_features) { dev_err(dev, "Unsupported features: %#llx\n", unsupp_features); @@ -615,7 +622,8 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, } /* Set a mac, either from the user config if provided - * or set a random mac if default is 00:..:00 + * or use the device's mac if not 00:..:00 + * or set a random mac */ if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR)) { ether_addr_copy(pdsv->mac, add_config->net.mac); @@ -624,7 +632,8 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, vc = pdsv->vdpa_aux->vd_mdev.device; memcpy_fromio(pdsv->mac, vc->mac, sizeof(pdsv->mac)); - if (is_zero_ether_addr(pdsv->mac)) { + if (is_zero_ether_addr(pdsv->mac) && + (pdsv->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) { eth_random_addr(pdsv->mac); dev_info(dev, "setting random mac %pM\n", pdsv->mac); } @@ -752,6 +761,10 @@ int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux) mgmt->id_table = pds_vdpa_id_table; mgmt->device = dev; mgmt->supported_features = le64_to_cpu(vdpa_aux->ident.hw_features); + + /* advertise F_MAC even if the device doesn't */ + mgmt->supported_features |= BIT_ULL(VIRTIO_NET_F_MAC); + mgmt->config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR); mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP); mgmt->config_attr_mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
Our driver sets a mac if the HW is 00:..:00 so we need to be sure to advertise VIRTIO_NET_F_MAC even if the HW doesn't. We also need to be sure that virtio_net sees the VIRTIO_NET_F_MAC and doesn't rewrite the mac address that a user may have set with the vdpa utility. After reading the hw_feature bits, add the VIRTIO_NET_F_MAC to the driver's supported_features and use that for reporting what is available. If the HW is not advertising it, be sure to strip the VIRTIO_NET_F_MAC before finishing the feature negotiation. If the user specifies a device_features bitpattern in the vdpa utility without the VIRTIO_NET_F_MAC set, then don't set the mac. Fixes: 151cc834f3dd ("pds_vdpa: add support for vdpa and vdpamgmt interfaces") Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> Reviewed-by: Brett Creeley <brett.creeley@amd.com> --- drivers/vdpa/pds/vdpa_dev.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)