Hello Ferruh,
> On 9/10/2018 5:55 PM, Juhamatti Kuusisaari wrote:
> > At the moment, PCAP interfaces use dummy MAC by default. This change
> > adds support for selecting PCAP physical interface MAC with phy_mac=1
> > devarg. This allows to setup packet flows using the physical interface
> > MAC.
> >
> > Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisa...@coriant.com>
> >
> > ---
> > v6:
> > * Review changes:
> > * Clarified devarg applicability (applies to iface-only)
> > * Introduced detailed documentation for the devarg
> > * More checkpatches-fixes
> > v5-v3:
> > * FreeBSD related compilation and checkpatches-fixes
> > v2:
> > * FreeBSD support introduced
> > * Release notes added
> > v1:
> > * phy_mac=1 devarg support
>
> <...>
>
> > +#elif defined(__FreeBSD__)
>
> Just to double check did you check/verify below code on FreeBSD?
I did run a manual test of the added code and seems to work. I think it is
still better to run DPDK on it to make sure everything works, so I'll do that
before sending next revision.
>
> <...>
>
> > @@ -955,6 +1034,10 @@ eth_from_pcaps_common(struct
> rte_vdev_device *vdev,
> > else
> > (*internals)->if_index = if_nametoindex(pair->value);
> >
> > + if (phy_mac && pair) /* phy_mac arg is applied only to iface */
>
> Having this comment is good, but "iface" is so generic, it may be confusing
> for
> beyond this context, what about "only if iface devarg provided" kind of
> detail?
Yes, let's elaborate that.
> <...>
>
> > @@ -989,6 +1073,19 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
> > return 0;
> > }
> >
> > +static int
> > +select_phy_mac(const char *key, const char *value, void *extra_args)
> > +{
> > + if (extra_args) {
> > + const int phy_mac = atoi(value);
> > + int *enable_phy_mac = extra_args;
> > +
> > + if (phy_mac)
> > + *enable_phy_mac = 1;
> > + }
> > + return 0;
> > +}
>
> This is causing build error because of "key" not used, needs __rte_unused
> marker.
Yes, need to fix this.
> <...>
>
> > @@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> > * reading / writing
> > */
> > if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
> > + /*
> > + * We check first whether we want to use phy MAC of the
> PCAP
> > + * interface.
> > + */
> > + if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
>
> Do you need count check at all?
I think it is needed, as the arg may not exist there. At least to me calling
process directly does not feel right, even if it would work.
> > + ret = rte_kvargs_process(kvlist,
> ETH_PCAP_PHY_MAC_ARG,
> > + &select_phy_mac, &phy_mac);
> > + if (ret < 0)
> > + goto free_kvlist;
> > + }
>
> I would prefer to see this block below ETH_PCAP_IFACE_ARG check, this
> block is
> for "iface", so it makes more sense to me first verify it, later verify
> phy_mac
Sure, I'll add it.
As there is already pcap-related MAC address patch merged, I'll need to do a
rebase and recheck this patch functionality on top of that. It will take some
time (which is a scarce resource at the moment), so I'll try to get this in
again after 18.11.
Thanks,
--
Juhamatti
