Ján Tomko wrote: > On 05/11/2014 08:48 AM, Roman Bogorodskiy wrote: > > Automatically allocate PCI addresses for devices instead > > of hardcoding them in the driver code. The current > > allocation schema is to dedicate an entire slot for each devices. > > > > > Also, allow having arbitrary number of devices. > > I think this can be split in a separate patch.
Sounds good. Originally, I decided to send it in a single series to
justify the list of functions and types moved.
But now as the first two patches are ACKed, I'll push them (hopefully
today or tomorrow) and continue iterating with the bhyve one.
> > diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> > index 91a8731..bc7ec5c 100644
> > --- a/src/bhyve/bhyve_command.c
> > +++ b/src/bhyve/bhyve_command.c
> > @@ -41,21 +41,14 @@ VIR_LOG_INIT("bhyve.bhyve_command");
> > static int
> > bhyveBuildNetArgStr(const virDomainDef *def, virCommandPtr cmd, bool
> > dryRun)
> > {
> > - virDomainNetDefPtr net = NULL;
> > - char *brname = NULL;
> > - char *realifname = NULL;
> > - int *tapfd = NULL;
> > char macaddr[VIR_MAC_STRING_BUFLEN];
> > + size_t i;
> >
> > - if (def->nnets != 1) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("domain should have one and only one net
> > defined"));
> > - return -1;
> > - }
> > -
> > - net = def->nets[0];
> > -
> > - if (net) {
> > + for (i = 0; i < def->nnets; i++) {
>
> Moving this loop to the caller of the function would reduce the extra
> indentation level.
Makes sense, will fix.
> > + char *realifname = NULL;
> > + int *tapfd = NULL;
> > + char *brname = NULL;
> > + virDomainNetDefPtr net = net = def->nets[i];;
>
> No need to assign it twice and one semicolon would be enough. :)
Must we a fat finger of mine while messing with vim. Funny that it is
still a valid code.
> > int actualType = virDomainNetGetActualType(net);
> >
> > if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>
> > @@ -146,7 +141,7 @@ bhyveBuildConsoleArgStr(const virDomainDef *def,
> > virCommandPtr cmd)
> > return -1;
> > }
> >
> > - virCommandAddArgList(cmd, "-s", "31,lpc", NULL);
> > + virCommandAddArgList(cmd, "-s", "1,lpc", NULL);
>
> Are both slot numbers arbitrary? And do we care about backwards compatibility?
>
Yes, both these slot numbers are arbitrary. I'm not sure we need to care
about backwards compatibility because guest console configuration
does not depend on the pci slot used for that PCI-ISA bridge.
> > virCommandAddArg(cmd, "-l");
> > virCommandAddArgFormat(cmd, "com%d,%s",
> > chr->target.port + 1,
> > chr->source.data.file.path);
> > @@ -157,46 +152,43 @@ bhyveBuildConsoleArgStr(const virDomainDef *def,
> > virCommandPtr cmd)
> > static int
> > bhyveBuildDiskArgStr(const virDomainDef *def, virCommandPtr cmd)
> > {
> > - virDomainDiskDefPtr disk;
> > const char *bus_type;
> > + size_t i;
> > +
> > + for (i = 0; i < def->ndisks; i++) {
> > + virDomainDiskDefPtr disk = def->disks[i];
> > +
> > + switch (disk->bus) {
> > + case VIR_DOMAIN_DISK_BUS_SATA:
> > + bus_type = "ahci-hd";
> > + break;
> > + case VIR_DOMAIN_DISK_BUS_VIRTIO:
> > + bus_type = "virtio-blk";
> > + break;
> > + default:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("unsupported disk bus type"));
> > + return -1;
> > + }
> >
> > - if (def->ndisks != 1) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("domain should have one and only one disk
> > defined"));
> > - return -1;
> > - }
> > -
> > - disk = def->disks[0];
> > -
> > - switch (disk->bus) {
> > - case VIR_DOMAIN_DISK_BUS_SATA:
> > - bus_type = "ahci-hd";
> > - break;
> > - case VIR_DOMAIN_DISK_BUS_VIRTIO:
> > - bus_type = "virtio-blk";
> > - break;
> > - default:
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("unsupported disk bus type"));
> > - return -1;
> > - }
> > + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("unsupported disk device"));
> > + return -1;
> > + }
> >
> > - if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("unsupported disk device"));
> > - return -1;
> > - }
> > + if (virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("unsupported disk type"));
> > + return -1;
> > + }
> >
> > - if (virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("unsupported disk type"));
> > - return -1;
> > + virCommandAddArg(cmd, "-s");
> > + virCommandAddArgFormat(cmd, "%d:0,%s,%s",
> > + disk->info.addr.pci.slot, bus_type,
> > + virDomainDiskGetSource(disk));
>
> Do SATA disks use a PCI slot too?
Yes:
http://www.freebsd.org/cgi/man.cgi?query=bhyve&sektion=0&manpath=FreeBSD+10.0-RELEASE&format=html
Basically, all the devices added through the "-s" flag are pci devices
that require the slot number (and function, optionally).
> > +static int
> > +bhyveCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
> > + virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
> > + virDomainDeviceInfoPtr info,
> > + void *opaque)
> > +{
> > + int ret = -1;
> > + virDomainPCIAddressSetPtr addrs = opaque;
> > + virDevicePCIAddressPtr addr = &info->addr.pci;
> > +
> > + if (addr->domain == 0 && addr->bus == 0) {
> > + if (addr->slot == 0) {
> > + return 0;
> > + } else if (addr->slot == 1) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("PCI bus 0 slot 1 is reserved for the
> > implicit "
> > + "LPC PCI-ISA bridge"));
>
> It's only implied when the console is present. Does it make sense to let
> someone use the address if they don't want the ISA bridge?
>
> Maybe we should generate an explicit <controller> element for it too.
As we currently do not have explicit support for PCI-ISA bridges, and in
bhyve it's used for console only (and unlikely will be used for
something else), the idea was to just reserve this slot to get a
consistent and reproducible pci addresses allocation and also not to
deal with a new controller type just for that single feature.
> > +bhyveAssignDevicePCISlots(virDomainDefPtr def,
> > + virDomainPCIAddressSetPtr addrs)
> > +{
> > + size_t i;
> > + virDevicePCIAddress lpc_addr;
> > +
> > + /* explicitly reserve slot 1 for LPC-ISA bridge */
> > + memset(&lpc_addr, 0, sizeof(lpc_addr));
> > + lpc_addr.slot = 0x1;
> > +
> > + if (virDomainPCIAddressReserveSlot(addrs, &lpc_addr,
> > VIR_PCI_CONNECT_TYPE_PCI) < 0)
> > + goto error;
> > +
> > + for (i = 0; i < def->ndisks; i++) {
> > + if (def->disks[i]->info.type !=
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> > + def->disks[i]->info.addr.pci.slot != 0)
> > + continue;
> > + if (virDomainPCIAddressReserveNextSlot(addrs,
> > + &def->disks[i]->info,
> > + VIR_PCI_CONNECT_TYPE_PCI) <
> > 0)
> > + goto error;
> > + }
> > +
> > + for (i = 0; i < def->nnets; i++) {
> > + if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> > + continue;
> > + if (virDomainPCIAddressReserveNextSlot(addrs,
> > + &def->nets[i]->info,
> > + VIR_PCI_CONNECT_TYPE_PCI) <
> > 0)
> > + goto error;
> > + }
>
> Assigning nets before disks would match the hardcoded slots that were used by
> a domain with one net and one disk before.
Good point.
> > +
> > + for (i = 0; i < def->ncontrollers; i++) {
> > + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> > + if (def->controllers[i]->info.type !=
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> > + continue;
> > + if (def->controllers[i]->model ==
> > VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
> > + continue;
> > +
> > + if (virDomainPCIAddressReserveNextSlot(addrs,
> > +
> > &def->controllers[i]->info,
> > +
> > VIR_PCI_CONNECT_TYPE_PCI) < 0)
> > + goto error;
> > + }
>
> I think unsupported controllers don't need PCI slots.
I will take a look into that.
Thanks a lot for reviewing this rather huge patchset!
Roman Bogorodskiy
pgp0NvPPaaFgv.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
