Re: [libvirt] [PATCH v3 0/6] PCI hostdev partial assignment support
On 11/26/19 8:33 AM, Daniel Henrique Barboza wrote:
changes from previous version [1]:
- do not overload address type='none'. A new address type called
'unassigned' is introduced;
- there is no unassigned' flag being created in virDomainHostdevDef.
The logic added by new address type is enough;
- do not allow function zero of multifunction devices to be
unassigned.
Nothing too special to discuss in this cover letter. More details
can be found at the discussions of the previous version [1]. Commit
messages of the patches have more background info as well.
[1] https://www.redhat.com/archives/libvir-list/2019-November/msg01099.html
For 1-4:
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
You asked in the last posting whether to use ADDRESS_TYPE_NONE and add a
new hostdev unassigned= attribute, or this approach. I think this is the
correct approach. Address type='unassigned' captures the description
well, and since address type='none' isn't printed in the XML, it would
make XML output a bit more confusing IMO if no address was printed.
One comment
Daniel Henrique Barboza (6):
Introducing new address type='unassigned' for PCI hostdevs
qemu: handle unassigned PCI hostdevs in command line and alias
Is there a specific reason to skip alias assign, beside it not being
needed for the command line? If it doesn't hurt, maybe we just keep it.
I don't have a strong argument for it though. If no one says anything
I'll leave it as is
virhostdev.c: check all IOMMU devs in virHostdevPreparePCIDevices
formatdomain.html.in: document <address type='unassigned'/>
For the first 4 I'll give it a few days and push on Friday if no one
complains.
For the last two:
utils: PCI multifunction detection helpers
domain_conf.c: don't allow function zero to be unassigned
The domain_conf.c additions should go into the
virDomainHostdevDefValidate. But this validation check seems to actually
read from host PCI space. I'm not sure if that's a good idea to do for
every XML parse? What's the failure scenario without this error message?
Does it fail in an obvious way? If so, maybe it's better to side step
the issue and just let it fail if it's a valid configuration.
Maybe laine knows better if there's precedent for similar checks at
Validate time
Thanks,
Cole