[libvirt] [PATCH v3 0/6] PCI hostdev partial assignment support

Daniel Henrique Barboza posted 6 patches 7 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191126133401.1242133-1-danielhb413@gmail.com
docs/formatdomain.html.in                     | 14 +++
docs/schemas/domaincommon.rng                 |  5 ++
src/conf/device_conf.c                        |  2 +
src/conf/device_conf.h                        |  1 +
src/conf/domain_conf.c                        | 20 ++++-
src/libvirt_private.syms                      |  2 +
src/qemu/qemu_alias.c                         |  6 ++
src/qemu/qemu_command.c                       |  5 ++
src/qemu/qemu_domain.c                        |  1 +
src/qemu/qemu_domain_address.c                |  5 ++
src/util/virhostdev.c                         | 88 +++++++++++++++++--
src/util/virhostdev.h                         |  3 +
src/util/virpci.c                             | 17 ++++
src/util/virpci.h                             |  2 +
.../hostdev-pci-address-unassigned.args       | 31 +++++++
.../hostdev-pci-address-unassigned.xml        | 42 +++++++++
...pci-multifunction-zero-unassigned-fail.xml | 42 +++++++++
tests/qemuxml2argvtest.c                      |  8 ++
.../hostdev-pci-address-unassigned.xml        | 58 ++++++++++++
tests/qemuxml2xmltest.c                       |  1 +
tests/virpcimock.c                            |  9 +-
21 files changed, 351 insertions(+), 11 deletions(-)
create mode 100644 tests/qemuxml2argvdata/hostdev-pci-address-unassigned.args
create mode 100644 tests/qemuxml2argvdata/hostdev-pci-address-unassigned.xml
create mode 100644 tests/qemuxml2argvdata/hostdev-pci-multifunction-zero-unassigned-fail.xml
create mode 100644 tests/qemuxml2xmloutdata/hostdev-pci-address-unassigned.xml

[libvirt] [PATCH v3 0/6] PCI hostdev partial assignment support

Posted by Daniel Henrique Barboza 7 weeks ago
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

Daniel Henrique Barboza (6):
  Introducing new address type='unassigned' for PCI hostdevs
  qemu: handle unassigned PCI hostdevs in command line and alias
  virhostdev.c: check all IOMMU devs in virHostdevPreparePCIDevices
  formatdomain.html.in: document <address type='unassigned'/>
  utils: PCI multifunction detection helpers
  domain_conf.c: don't allow function zero to be unassigned

 docs/formatdomain.html.in                     | 14 +++
 docs/schemas/domaincommon.rng                 |  5 ++
 src/conf/device_conf.c                        |  2 +
 src/conf/device_conf.h                        |  1 +
 src/conf/domain_conf.c                        | 20 ++++-
 src/libvirt_private.syms                      |  2 +
 src/qemu/qemu_alias.c                         |  6 ++
 src/qemu/qemu_command.c                       |  5 ++
 src/qemu/qemu_domain.c                        |  1 +
 src/qemu/qemu_domain_address.c                |  5 ++
 src/util/virhostdev.c                         | 88 +++++++++++++++++--
 src/util/virhostdev.h                         |  3 +
 src/util/virpci.c                             | 17 ++++
 src/util/virpci.h                             |  2 +
 .../hostdev-pci-address-unassigned.args       | 31 +++++++
 .../hostdev-pci-address-unassigned.xml        | 42 +++++++++
 ...pci-multifunction-zero-unassigned-fail.xml | 42 +++++++++
 tests/qemuxml2argvtest.c                      |  8 ++
 .../hostdev-pci-address-unassigned.xml        | 58 ++++++++++++
 tests/qemuxml2xmltest.c                       |  1 +
 tests/virpcimock.c                            |  9 +-
 21 files changed, 351 insertions(+), 11 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/hostdev-pci-address-unassigned.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-pci-address-unassigned.xml
 create mode 100644 tests/qemuxml2argvdata/hostdev-pci-multifunction-zero-unassigned-fail.xml
 create mode 100644 tests/qemuxml2xmloutdata/hostdev-pci-address-unassigned.xml

-- 
2.23.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/6] PCI hostdev partial assignment support

Posted by Cole Robinson 5 weeks ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/6] PCI hostdev partial assignment support

Posted by Daniel Henrique Barboza 5 weeks ago

On 12/11/19 8:49 PM, Cole Robinson wrote:
> On 11/26/19 8:33 AM, Daniel Henrique Barboza wrote:
>> changes from previous version [1]:
[...]
> 
> 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


The reason why I was skipping aliases was cosmetic due to QEMU command line.
I find it a bit odd that, in a scenario with 4 functions where function 1 is
unassigned, QEMU cmd line would have hostdev0, hostdev2 and hostdev3, because
'hostdev1' alias got assigned to the unassigned function '1'

I don't have any strong feelings about this though. We can keep giving aliases
or all functions, regardless of whether they are being assigned to the
guest or not. Unit tests will need to be adjusted though.



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

This is a strange scenario TBH. I discussed it a bit with Alex Williamson in
the v2 of this series. At first there is nothing wrong with this, in fact
QEMU allows it. Problem is that Power guests handle this case in a "better" way
than x86 and others, making the non-zero function being visible and usable
by the guest without any extra kernel option (x86 needs pci_scan_all). It's
also hardware dependent - the BCM5719 network card I am using for testing
deals with this scenario, but there's no guarantee that other cards will.

To answer your question directly: this might not fail in an obvious way in
the guest, but it's not like this feature is a default PCI assignment
mode - the user have to deliberately unassigned the function 0 in the XML.
Granted, I am being conservative with this extra check - we can simply let
the user play with the configuration and, in case it blows up, the user
can simply add function 0 back to the guest. If this turns out to be a
"toxic" setup then I can go to QEMU and deal with it there - I mean, in the
end it's QEMU that allows this, so makes sense to handle the case down there.



Thanks,


DHB


> 
> Maybe laine knows better if there's precedent for similar checks at
> Validate time
> 
> Thanks,
> Cole
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/6] PCI hostdev partial assignment support

Posted by Cole Robinson 5 weeks ago
On 12/12/19 4:41 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 12/11/19 8:49 PM, Cole Robinson wrote:
>> On 11/26/19 8:33 AM, Daniel Henrique Barboza wrote:
>>> changes from previous version [1]:
> [...]
>>
>> 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
> 
> 
> The reason why I was skipping aliases was cosmetic due to QEMU command
> line.
> I find it a bit odd that, in a scenario with 4 functions where function
> 1 is
> unassigned, QEMU cmd line would have hostdev0, hostdev2 and hostdev3,
> because
> 'hostdev1' alias got assigned to the unassigned function '1'
> 
> I don't have any strong feelings about this though. We can keep giving
> aliases
> or all functions, regardless of whether they are being assigned to the
> guest or not. Unit tests will need to be adjusted though.
> 
> 
> 
>>
>>>    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.
> 
> This is a strange scenario TBH. I discussed it a bit with Alex
> Williamson in
> the v2 of this series. At first there is nothing wrong with this, in fact
> QEMU allows it. Problem is that Power guests handle this case in a
> "better" way
> than x86 and others, making the non-zero function being visible and usable
> by the guest without any extra kernel option (x86 needs pci_scan_all). It's
> also hardware dependent - the BCM5719 network card I am using for testing
> deals with this scenario, but there's no guarantee that other cards will.
> 
> To answer your question directly: this might not fail in an obvious way in
> the guest, but it's not like this feature is a default PCI assignment
> mode - the user have to deliberately unassigned the function 0 in the XML.
> Granted, I am being conservative with this extra check - we can simply let
> the user play with the configuration and, in case it blows up, the user
> can simply add function 0 back to the guest. If this turns out to be a
> "toxic" setup then I can go to QEMU and deal with it there - I mean, in the
> end it's QEMU that allows this, so makes sense to handle the case down
> there.

Okay I think we should drop the last two patches then.

Small change of plan: Please respin this series with the alias changes
dropped. Also in the docs patch, the version needs to be updated to
6.0.0, I missed that the first time. After that I will push

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list