Re: [Qemu-devel] [PATCH target-arm v1 1/1] arm/xilinx_zynq: Always instantiate the GEMs

[Peter Crosthwaite]

On Sat, Jan 4, 2014 at 2:22 AM, Peter Maydell <address@hidden> wrote:
> On 3 January 2014 16:15, Peter Crosthwaite <address@hidden> wrote:
>> One subtle question before respinning - should the
>> qdev_set_nic_properties actually be conditional? Setting it to an
>> unused NIC should be harmless, so perhaps it should be outside the
>> if(nd->used). Other boards/MAC combos that dont do the
>> qdev_check_nic_model can then just dispose of the if (nd->used) check
>> altogether.
>
> Well qdev_set_nic_properties as it stands today will unconditionally
> assume you've passed it a valid NICInfo, so we have to a void calling
> it if nd->used isn't set.

So I guess I'm thinking that the unusued nics should be a "valid"
NICInfo - just one that doesn't connect to anything. Currently,
qdev_set_nic_properties() sets three props - "mac", "netdev" and
"vectors". "mac" looks dubious to me, I'm not sure why the net layer
is telling devices what their MAC addresses are - it should be the
other way round. Setting of "netdev" is already protected against NULL
setting so that one is handled. "vectors" looks like it has a sanity
check guard on its setting as well via the DEV_NVECTORS_UNSPECIFIED
check. Although that value may rely on some net.c code paths for
non-NULL NICs.

Will experiment with it before net spin. Will just leave it in inside
the "if" if it goes pear shaped on me.

> If you're asking whether that nd->used check
> should be moved into qdev_set_nic_properties, I'm not sure : I don't

And that's probably still preferable to having all boards add null
guards on setting a qdev property.

Regards,
Peter

> have a good enough grasp of how the net code works and how it ought
> to work for embedded always-instantiated NICs to be able to say.
>
> thanks
> -- PMM
>