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

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

[Peter Crosthwaite]

Don't conditionalise GEM instantiation on networking attachments. The
device should always be present even if not attached to a network.

This allows for probing of the device by expectant guests (such as
OS's).  This is needed because sysbus (or AXI in Xilinx's real hw case)
is not self identifying so the guest has no dynamic way of detecting
device absence.

Also allows for testing of the GEM in loopback mode with -net none.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/arm/xilinx_zynq.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 17251c7..9f2e0f5 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -49,9 +49,11 @@ static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     DeviceState *dev;
     SysBusDevice *s;
 
-    qemu_check_nic_model(nd, "cadence_gem");
     dev = qdev_create(NULL, "cadence_gem");
-    qdev_set_nic_properties(dev, nd);
+    if (nd) {
+        qemu_check_nic_model(nd, "cadence_gem");
+        qdev_set_nic_properties(dev, nd);
+    }
     qdev_init_nofail(dev);
     s = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(s, 0, base);
@@ -113,7 +115,6 @@ static void zynq_init(QEMUMachineInitArgs *args)
     DeviceState *dev;
     SysBusDevice *busdev;
     qemu_irq pic[64];
-    NICInfo *nd;
     Error *err = NULL;
     int n;
 
@@ -190,8 +191,8 @@ static void zynq_init(QEMUMachineInitArgs *args)
     sysbus_create_varargs("cadence_ttc", 0xF8002000,
             pic[69-IRQ_OFFSET], pic[70-IRQ_OFFSET], pic[71-IRQ_OFFSET], NULL);
 
-    for (n = 0; n < nb_nics; n++) {
-        nd = &nd_table[n];
+    for (n = 0; n < 2; n++) {
+        NICInfo *nd = n < nb_nics ? &nd_table[n] : NULL;
         if (n == 0) {
             gem_init(nd, 0xE000B000, pic[54-IRQ_OFFSET]);
         } else if (n == 1) {
-- 
1.8.5.2

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

[Peter Maydell]

On 3 January 2014 03:21, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Don't conditionalise GEM instantiation on networking attachments. The
> device should always be present even if not attached to a network.
>
> This allows for probing of the device by expectant guests (such as
> OS's).  This is needed because sysbus (or AXI in Xilinx's real hw case)
> is not self identifying so the guest has no dynamic way of detecting
> device absence.

Agreed that this is the right thing.

Some day I might try to look into how to update our handling
of embedded NICs to work with non-legacy command lines...


> -    for (n = 0; n < nb_nics; n++) {
> -        nd = &nd_table[n];
> +    for (n = 0; n < 2; n++) {
> +        NICInfo *nd = n < nb_nics ? &nd_table[n] : NULL;
>          if (n == 0) {
>              gem_init(nd, 0xE000B000, pic[54-IRQ_OFFSET]);
>          } else if (n == 1) {

This is now a rather odd loop which goes round exactly twice
and uses an if() statement to make the body different each time.

Instead you can just say:
     gem_init(nd_table[0], 0xe000b000, pic[54 - IRQ_OFFSET]);
     gem_init(nd_table[1], 0xe000c000, pic[54 - IRQ_OFFSET]);

and have gem_init() condition the calls to qdev_set_nic_properties
and qdev_check_nic_model on "if (nd->used)".

thanks
-- PMM

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

[Peter Crosthwaite]

On Fri, Jan 3, 2014 at 8:51 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 January 2014 03:21, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> Don't conditionalise GEM instantiation on networking attachments. The
>> device should always be present even if not attached to a network.
>>
>> This allows for probing of the device by expectant guests (such as
>> OS's).  This is needed because sysbus (or AXI in Xilinx's real hw case)
>> is not self identifying so the guest has no dynamic way of detecting
>> device absence.
>
> Agreed that this is the right thing.
>
> Some day I might try to look into how to update our handling
> of embedded NICs to work with non-legacy command lines...
>
>
>> -    for (n = 0; n < nb_nics; n++) {
>> -        nd = &nd_table[n];
>> +    for (n = 0; n < 2; n++) {
>> +        NICInfo *nd = n < nb_nics ? &nd_table[n] : NULL;
>>          if (n == 0) {
>>              gem_init(nd, 0xE000B000, pic[54-IRQ_OFFSET]);
>>          } else if (n == 1) {
>
> This is now a rather odd loop which goes round exactly twice
> and uses an if() statement to make the body different each time.

Yeh, I think I was going for minimum diff when I did this one. Will fix.

>
> Instead you can just say:
>      gem_init(nd_table[0], 0xe000b000, pic[54 - IRQ_OFFSET]);
>      gem_init(nd_table[1], 0xe000c000, pic[54 - IRQ_OFFSET]);
>
> and have gem_init() condition the calls to qdev_set_nic_properties

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.

Regards,
Peter

> and qdev_check_nic_model on "if (nd->used)".
>
> thanks
> -- PMM
>

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

[Peter Maydell]

On 3 January 2014 16:15, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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. If you're asking whether that nd->used check
should be moved into qdev_set_nic_properties, I'm not sure : I don't
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

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 <peter.maydell@linaro.org> wrote:
> On 3 January 2014 16:15, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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
>

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

[Peter Maydell]

On 3 January 2014 16:56, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Sat, Jan 4, 2014 at 2:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 3 January 2014 16:15, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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.

The device doesn't (in the old -net model) know what its MAC address
is, because that's something you specify in the -net command line option.
So this is the passing on of that info to the device.

> 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.

I think it would probably be better to look into properly cleaning up
the legacy -net nic support and moving embedded devices over to
work with some variant of the new command line model, rather
than tweaking the legacy stuff. nd_table eventually should ideally
just go away I think.

thanks
-- PMM

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

[Peter Crosthwaite]

On Sat, Jan 4, 2014 at 3:24 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 January 2014 16:56, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Sat, Jan 4, 2014 at 2:22 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 3 January 2014 16:15, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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.
>
> The device doesn't (in the old -net model) know what its MAC address
> is, because that's something you specify in the -net command line option.
> So this is the passing on of that info to the device.
>
>> 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.
>
> I think it would probably be better to look into properly cleaning up
> the legacy -net nic support and moving embedded devices over to
> work with some variant of the new command line model, rather
> than tweaking the legacy stuff. nd_table eventually should ideally
> just go away I think.
>

Ok, you've talked me out of it. Respun with the if.

Regards,
Peter

> thanks
> -- PMM
>