Networking with hyperstart #399
Conversation
|
|
||
| return ( g_strdup_printf("ip=%s::%s:%s:%s:%s:off::", | ||
| ipv4_cfg->ip_address, | ||
| return ( g_strdup_printf("ip=:::%s:::%s::off::", | ||
| config->net.gateway, |
There was a problem hiding this comment.
This function needs a check to ensure config != NULL before dereferencing. Also, I'd add checks on gateway and hostname to avoid (null) being returned as part of the string on error.
| /*! | ||
| * Append qemu options for networking | ||
| * | ||
| * config \ref cc_oci_config. |
There was a problem hiding this comment.
Both args need a \param in the function header.
| * additional_args Array that will be appended | ||
| */ | ||
| static void | ||
| cc_oci_append_network_args(struct cc_oci_config *config, |
There was a problem hiding this comment.
Need to check both params to ensure they are not NULL.
| @@ -105,7 +79,7 @@ cc_oci_expand_netdev_cmdline(struct cc_oci_config *config, guint index) { | |||
| return g_strdup(""); | |||
| } | |||
|
|
|||
| #define QEMU_FMT_DEVICE "driver=virtio-net-pci,netdev=%s" | |||
| #define QEMU_FMT_DEVICE "driver=virtio-net-pci,bus=/pci-lite-host/pcie.0,addr=%x,netdev=%s" | |||
There was a problem hiding this comment.
Is this bus a well-known address or something that is being "claimed"/allocated here? I think a comment would be useful to explain why this was added.
| } | ||
| /* append additional args array */ | ||
| for (int i = 0; i < extra_args_len; i++) { | ||
| gchar* arg = g_ptr_array_index(hypervisor_extra_args, i); |
There was a problem hiding this comment.
It would be clearer if arg were const here.
| * \param index Index into the array that was used for the qemu | ||
| * pcie address option as well | ||
| */ | ||
| gchar * |
| /* Offset to add to the interface index for assigning the pci slot. | ||
| * First 3 slots are in use for pc-lite machine type | ||
| */ | ||
| #define PCI_OFFSET 4 |
There was a problem hiding this comment.
Is there any likelihood that qemu might change the number of slots it assigns itself? What are the first 3 slots used for specifically?
This looks like a potential maintenance issue, but I'm not sure how we can protect against future changes here? Hopefully, if qemu did change, we'd get errors logged in the hypervisor logs, but can we do better than that?
There was a problem hiding this comment.
@jodh-intel The pci slots currently in use are:
PCI: slot 0 function 0 => in use by pci-lite-device
PCI: slot 1 function 0 => in use by PM_LITE
PCI: slot 2 function 0 => in use by virtio-9p-pci
PCI: slot 3 function 0 => in use by virtio-serial-pci
The first two 0 and 1 seem to be used by qemu for the pc-lite machine type, the last two are used by us for the 9p mount and for the virtual console respectively. I'll be updating the comments in the code to reflect this.
I am inclined to set the offset to a higher value (maybe 8) to take into account any available free slots being assigned by qemu or used by us in the future. What do you think?
( Slot 0 ref: https://github.com/01org/qemu-lite/blob/qemu-2.7-lite/hw/pci-host/pci_lite.c#L206
Slot 1 ref: https://github.com/01org/qemu-lite/blob/qemu-2.7-lite/hw/acpi/pm_lite.c#L302
These are part of qemu-lite changes submitted for pc-lite )
There was a problem hiding this comment.
Sounds reasonable to me. @anthonyzxu - do you have any comments on this strategy?
@amshinde - how about adding a g_debug() call where the value of PCI_OFFSET is logged (in cc_oci_expand_net_device_cmdline()?), so that if there is a future clash with qemu-lite, atleast there is a log entry to aid in debugging.
There was a problem hiding this comment.
We should correlate if this is at all affected/related to the PCI BAR scanning optimisations in the guest kernel as well - if you look at the guest kernel patches you'll find:
0006-probe-patch
407-pci-guest....
where we optimise the BAR scanning for the pc-lite machine, I believe to 4 slots presently, as we 'know' there is nothing beyond there ... or wasn't.
Indeed, it would be good to get @anthonyzxu input here.
(some of those two should really be merged ;-)
|
|
||
| ipaddr_arr = json_array_new(); | ||
|
|
||
| for (uint j = 0; j < g_slist_length(if_cfg->ipv4_addrs); j++) { |
|
I'm seeing the dreaded "generic hyperstart error" testing this branch: |
|
@jodh-intel Looks like your image does not have patch for multiple ip addresses. Looking at our package in Clear, this seems to have fallen through. I'll add that to the image. |
Signed-off-by: Archana Shinde <[email protected]>
Its sufficient to pass just the address of the array instead of pointer to the address. Signed-off-by: Archana Shinde <[email protected]>
The args to be appended should be dynamically allocated before adding to additional args array. The strings would be freed when the array is freed. Signed-off-by: Archana Shinde <[email protected]>
Assigning a pcie address to netdev device, ensures that the interface gets a unique name within the VM. This enables support for multiple interfaces by allowing an interface to be identified and configured based on its unique name mapped to pcie address. Signed-off-by: Archana Shinde <[email protected]>
… args. Signed-off-by: Archana Shinde <[email protected]>
…rfaces. Get rid of the network substitution variables and append the networking qemu options instead. Signed-off-by: Archana Shinde <[email protected]>
Configuration for multiple interfaces and multiple ip addresses for an interface can be passed while starting a POD.Remove the interface configuration from the kernel parameters. Signed-off-by: Archana Shinde <[email protected]>
amshinde
force-pushed
the
networking-hyper
branch
from
cabc196 to
6bdff9a
Compare
|
@jodh-intel I have addressed your review comments. |
|
lgtm. |
|
Haven't gone into details, but the general flow looks good to me. Better have it in the branch for all to test than in a PR. lgtm |
|
qa-passed |
No description provided.