Re: [PATCH v3] docs: Introducing pseries documentation.

2021-12-08 Thread Leonardo Augusto Guimarães Garcia 
Argh! Disregard this patch. It fails to build. I'll send v4 fixing this.

On 12/8/21 12:47, lagar...@linux.ibm.com wrote:
> From: Leonardo Garcia 
>
> The purpose of this document is to substitute the content currently
> available in the QEMU wiki at [0]. This initial version does contain
> some additional content as well. Whenever this documentation gets
> upstream and is reflected in [1], the QEMU wiki will be edited to point
> to this documentation, so that we only need to keep it updated in one
> place.
>
> 0. https://wiki.qemu.org/Documentation/Platforms/POWER
> 1. https://qemu.readthedocs.io/en/latest/system/ppc/pseries.html
>
> Signed-off-by: Leonardo Garcia 
> Reviewed-by: David Gibson 
> ---
>
> Changelog:
> v2->v3:
>  - Updating LoPAR link.
>  - Minor clarifications in the text.
>
> v1->v2:
> - Addressing David Gibson and Cédric's comments. Thanks!
>
>  docs/system/ppc/pseries.rst | 226 
>  1 file changed, 226 insertions(+)
>
> diff --git a/docs/system/ppc/pseries.rst b/docs/system/ppc/pseries.rst
> index 932d4dd17d..f072f4a935 100644
> --- a/docs/system/ppc/pseries.rst
> +++ b/docs/system/ppc/pseries.rst
> @@ -1,12 +1,238 @@
>  pSeries family boards (``pseries``)
>  ===
>
> +The Power machine para-virtualized environment described by the `Linux on 
> Power
> +Architecture Reference document (LoPAR)
> +`_
> +is called pSeries. This environment is also known as sPAPR, System p guests, 
> or
> +simply Power Linux guests (although it is capable of running other operating
> +systems, such as AIX).
> +
> +Even though pSeries is designed to behave as a guest environment, it is also
> +capable of acting as a hypervisor OS, providing, on that role, nested
> +virtualization capabilities.
> +
>  Supported devices
>  -
>
> + * Multi processor support for many Power processors generations: POWER7,
> +   POWER7+, POWER8, POWER8NVL, POWER9, and Power10. Support for POWER5+ 
> exists,
> +   but its state is unknown.
> + * Interrupt Controller, XICS (POWER8) and XIVE (POWER9 and Power10)
> + * vPHB PCIe Host bridge.
> + * vscsi and vnet devices, compatible with the same devices available on a
> +   PowerVM hypervisor with VIOS managing LPARs.
> + * Virtio based devices.
> + * PCIe device pass through.
> +
>  Missing devices
>  ---
>
> + * SPICE support.
>
>  Firmware
>  
> +
> +`SLOF `_ (Slimline Open Firmware) is an
> +implementation of the `IEEE 1275-1994, Standard for Boot (Initialization
> +Configuration) Firmware: Core Requirements and Practices
> +`_.
> +
> +QEMU includes a prebuilt image of SLOF which is updated when a more recent
> +version is required.
> +
> +Build directions
> +
> +
> +.. code-block:: bash
> +
> +  ./configure --target-list=ppc64-softmmu && make
> +
> +Running instructions
> +
> +
> +Someone can select the pSeries machine type by running QEMU with the 
> following
> +options:
> +
> +.. code-block:: bash
> +
> +  qemu-system-ppc64 -M pseries 
> +
> +sPAPR devices
> +-
> +
> +The sPAPR specification defines a set of para-virtualized devices, which are
> +also supported by the pSeries machine in QEMU and can be instantiated with 
> the
> +``-device`` option:
> +
> +* ``spapr-vlan`` : a virtual network interface.
> +* ``spapr-vscsi`` : a virtual SCSI disk interface.
> +* ``spapr-rng`` : a pseudo-device for passing random number generator data 
> to the
> +  guest (see the `H_RANDOM hypercall feature
> +  `_ for details).
> +* ``spapr-vty``: a virtual teletype.
> +* ``spapr-pci-host-bridge``: a PCI host bridge.
> +* ``tpm-spapr``: a Trusted Platform Module (TPM).
> +* ``spapr-tpm-proxy``: a TPM proxy.
> +
> +These are compatible with the devices historically available for use when
> +running the IBM PowerVM hypervisor with LPARs.
> +
> +However, since these devices have originally been specified with another
> +hypervisor and non-Linux guests in mind, you should use the virtio 
> counterparts
> +(virtio-net, virtio-blk/scsi and virtio-rng for instance) if possible 
> instead,
> +since they will most probably give you better performance with Linux guests 
> in a
> +QEMU environment.
> +
> +The pSeries machine in QEMU is always instantiated with the following 
> devices:
> +
> +* A NVRAM device (``spapr-nvram``).
> +* A virtual teletype (``spapr-vty``).
> +* A PCI host bridge (``spapr-pci-host-bridge``).
> +
> +Hence, it is not needed to add them manually, unless you use the 
> ``-nodefaults``
> +command line option in QEMU.
> +
> +In the case of the default ``spapr-nvram`` device, if someone wants to make 
> the
> +contents of the NVRAM device persistent, they will need to specify a PFLASH
> +device when starting QEMU, i.e. either

Re: [PATCH] docs: Introducing pseries documentation.

2021-12-07 Thread Leonardo Augusto Guimarães Garcia 
Hi Cédric,

On 11/18/21 05:57, Cédric Le Goater wrote:
> Hello Leonardo,
>
> On 11/17/21 21:14, lagar...@linux.ibm.com wrote:
>> From: Leonardo Garcia 
>>
>> The purpose of this document is to substitute the content currently
>> available in the QEMU wiki at [0]. This initial version does contain
>> some additional content as well. Whenever this documentation gets
>> upstream and is reflected in [1], the QEMU wiki will be edited to point
>> to this documentation, so that we only need to keep it updated in one
>> place.
>>
>> 0. https://wiki.qemu.org/Documentation/Platforms/POWER
>> 1. https://qemu.readthedocs.io/en/latest/system/ppc/pseries.html
>>
>> Signed-off-by: Leonardo Garcia 
>
>
> Thanks for this update,
>
> Some general comments,
>
> There are other ppc documents :
>
>   docs/specs/ppc-spapr-hcalls.txt
>   docs/specs/ppc-spapr-hotplug.txt
>   docs/specs/ppc-spapr-numa.rst
>   docs/specs/ppc-spapr-uv-hcalls.txt
>   docs/specs/ppc-spapr-xive.rst
>   docs/specs/ppc-xive.rst
>
> that should be moved maybe and/or referenced by this file ? Feel free
> to do a follow up.


Definitely! Thanks for pointing this out. I have included some reference
to these files in v2, but proper reference will need some rework on the
above files as well (such as transforming txt files into rst files).
I'll work on that as a follow up.


>
> On the terminology, I think we should use in the text :
>
>    pSeries, PowerNV, POWER[0-9]


Sure. I updated the terminology according to the above in v2.


>
> When it comes to QEMU machines names, it's 'pseries', 'powernv'
>
> Some small comments,
>
>
>> ---
>>   docs/system/ppc/pseries.rst | 185 
>>   1 file changed, 185 insertions(+)
>>
>> diff --git a/docs/system/ppc/pseries.rst b/docs/system/ppc/pseries.rst
>> index 932d4dd17d..2de3fb4d51 100644
>> --- a/docs/system/ppc/pseries.rst
>> +++ b/docs/system/ppc/pseries.rst
>> @@ -1,12 +1,197 @@
>>   pSeries family boards (``pseries``)
>>   ===
>>   +The Power machine virtualized environment described by the `Linux
>> on Power
>
> para-virtualized ?


Absolutely. Fixed.


>
>> +Architecture Reference document (LoPAR)
>> +`_
>>
>> +is called pseries. This environment is also known as sPAPR, System p
>> guests, or
>> +simply Power Linux guests (although it is capable of running other
>> operating
>> +systems, such as AIX).
>> +
>> +Even though pseries is designed to behave as a guest environment, it
>> is also
>> +capable of acting as a hypervisor OS, providing, on that role, nested
>> +virtualization capabilities.
>
> on POWER9 and above processors. Maybe we should start introducing the
> KVM-pseries term.


We can do nested virtualization with the combination of KVM-HV and
KVM-PR on POWER8 machines, for instance. At this point of the text, I
wouldn't like to enter in this kind of detail, as it will be treated
later in the documentation.


>
>> +
>>   Supported devices
>>   -
>>   + * Multi processor support for many Power processors generations:
>> POWER5+,
>> +   POWER7, POWER7+, POWER8, POWER8NVL, Power9, and Power10 (there is
>> no support
>> +   for POWER6 processors).
>
> POWER8NVL is for baremetal only.


You can actually create pseries guests on a POWER8NVL machine using
QEMU/KVM.


>
>> + * Interrupt Controller, XICS (POWER8) and XIVE (Power9 and Power10)
>> + * vPHB PCIe Host bridge.
>> + * vscsi and vnet devices, compatible with the same devices
>> available on a
>> +   PowerVM hypervisor with VIOS managing LPARs.
>> + * Virtio based devices.
>> + * PCIe device pass through.
>> +
>>   Missing devices
>>   ---
>>   + * SPICE support.
>>     Firmware
>>   
>> +
>> +`SLOF `_ (Slimline Open Firmware) is an
>> +implementation of the `IEEE 1275-1994, Standard for Boot
>> (Initialization
>> +Configuration) Firmware: Core Requirements and Practices
>> +`_.
>> +
>> +QEMU includes a prebuilt image of SLOF which is updated when a more
>> recent
>> +version is required.
>> +
>> +Build directions
>> +
>> +
>> +.. code-block:: bash
>> +
>> +  ./configure --target-list=ppc64-softmmu && make
>> +
>> +Running instructions
>> +
>> +
>> +Someone can select the pseries machine type by running QEMU with the
>> following
>> +options:
>> +
>> +.. code-block:: bash
>> +
>> +  qemu-system-ppc64 -M pseries 
>> +
>> +sPAPR devices
>> +-
>> +
>> +The sPAPR specification defines a set of para-virtualized devices,
>> which are
>> +also supported by the pseries machine in QEMU and can be
>> instantiated with the
>> +`-device` option:
>> +
>> +* spapr-vlan : A virtual network interface.
>> +* spapr-vscsi : A virtual SCSI disk interface.
>> +* spapr-rng : A pseudo-device for passing random number generator
>> data to the
>> +  guest (see the `H_RANDOM

Re: [PATCH v2] docs: Minor updates on the powernv documentation.

2021-12-07 Thread Leonardo Augusto Guimarães Garcia 
On 12/2/21 03:51, Cédric Le Goater wrote:
> Hello Leonardo,
>
> On 11/23/21 13:10, lagar...@linux.ibm.com wrote:
>> From: Leonardo Garcia 
>>
>> Signed-off-by: Leonardo Garcia 
>> ---
>
> It seems that POWER10 was renamed to Power10 but not POWER9. And :
>
>   https://en.wikipedia.org/wiki/Power9  redirects to POWER9
>   https://en.wikipedia.org/wiki/POWER10 redirects to Power10
>
> I will keep the upper case POWER9. No need to resend.


 
Ok. Thanks! I'll keep this nomenclature on the pseries documentation
patches as well.
 
Cheers,
 
Leo


>
> Thanks,
>
> C.
>
>
>
>>   docs/system/ppc/powernv.rst | 57 +++--
>>   1 file changed, 29 insertions(+), 28 deletions(-)
>>
>> diff --git a/docs/system/ppc/powernv.rst b/docs/system/ppc/powernv.rst
>> index 86186b7d2c..eda4219a27 100644
>> --- a/docs/system/ppc/powernv.rst
>> +++ b/docs/system/ppc/powernv.rst
>> @@ -1,7 +1,7 @@
>> -PowerNV family boards (``powernv8``, ``powernv9``)
>> +PowerNV family boards (``powernv8``, ``powernv9``, ``powernv10``)
>>   ==
>>   -PowerNV (as Non-Virtualized) is the "baremetal" platform using the
>> +PowerNV (as Non-Virtualized) is the "bare metal" platform using the
>>   OPAL firmware. It runs Linux on IBM and OpenPOWER systems and it can
>>   be used as an hypervisor OS, running KVM guests, or simply as a host
>>   OS.
>> @@ -15,17 +15,15 @@ beyond the scope of what QEMU addresses today.
>>   Supported devices
>>   -
>>   - * Multi processor support for POWER8, POWER8NVL and POWER9.
>> - * XSCOM, serial communication sideband bus to configure chiplets
>> - * Simple LPC Controller
>> - * Processor Service Interface (PSI) Controller
>> - * Interrupt Controller, XICS (POWER8) and XIVE (POWER9)
>> - * POWER8 PHB3 PCIe Host bridge and POWER9 PHB4 PCIe Host bridge
>> - * Simple OCC is an on-chip microcontroller used for power management
>> -   tasks
>> - * iBT device to handle BMC communication, with the internal BMC
>> -   simulator provided by QEMU or an external BMC such as an Aspeed
>> -   QEMU machine.
>> + * Multi processor support for POWER8, POWER8NVL and Power9.
>> + * XSCOM, serial communication sideband bus to configure chiplets.
>> + * Simple LPC Controller.
>> + * Processor Service Interface (PSI) Controller.
>> + * Interrupt Controller, XICS (POWER8) and XIVE (Power9) and XIVE2
>> (Power10).
>> + * POWER8 PHB3 PCIe Host bridge and POWER9 PHB4 PCIe Host bridge.
>> + * Simple OCC is an on-chip micro-controller used for power
>> management tasks.
>> + * iBT device to handle BMC communication, with the internal BMC
>> simulator
>> +   provided by QEMU or an external BMC such as an Aspeed QEMU machine.
>>    * PNOR containing the different firmware partitions.
>>     Missing devices
>> @@ -33,27 +31,25 @@ Missing devices
>>     A lot is missing, among which :
>>   - * POWER10 processor
>> - * XIVE2 (POWER10) interrupt controller
>> - * I2C controllers (yet to be merged)
>> - * NPU/NPU2/NPU3 controllers
>> - * EEH support for PCIe Host bridge controllers
>> - * NX controller
>> - * VAS controller
>> - * chipTOD (Time Of Day)
>> + * I2C controllers (yet to be merged).
>> + * NPU/NPU2/NPU3 controllers.
>> + * EEH support for PCIe Host bridge controllers.
>> + * NX controller.
>> + * VAS controller.
>> + * chipTOD (Time Of Day).
>>    * Self Boot Engine (SBE).
>> - * FSI bus
>> + * FSI bus.
>>     Firmware
>>   
>>     The OPAL firmware (OpenPower Abstraction Layer) for OpenPower
>> systems
>>   includes the runtime services ``skiboot`` and the bootloader kernel
>> and
>> -initramfs ``skiroot``. Source code can be found on GitHub:
>> +initramfs ``skiroot``. Source code can be found on the `OpenPOWER
>> account at
>> +GitHub `_.
>>   -  https://github.com/open-power.
>> -
>> -Prebuilt images of ``skiboot`` and ``skiroot`` are made available on
>> the `OpenPOWER `__
>> site.
>> +Prebuilt images of ``skiboot`` and ``skiroot`` are made available on
>> the
>> +`OpenPOWER `__ site.
>>     QEMU includes a prebuilt image of ``skiboot`` which is updated
>> when a
>>   more recent version is required by the models.
>> @@ -83,6 +79,7 @@ and a SATA disk :
>>     Complex PCIe configuration
>>   ~~
>> +
>>   Six PHBs are defined per chip (POWER9) but no default PCI layout is
>>   provided (to be compatible with libvirt). One PCI device can be added
>>   on any of the available PCIe slots using command line options such as:
>> @@ -157,7 +154,7 @@ one on the command line :
>>   The files `palmetto-SDR.bin
>> `__
>>   and `palmetto-FRU.bin
>> `__
>>   define a Sensor Data Record repository and a Field Replaceable Unit
>> -inventory for a palmetto BMC. They can be used to extend

Re: [PATCH] docs: Introducing pseries documentation.

2021-11-23 Thread Leonardo Augusto Guimarães Garcia 
On 11/17/21 20:30, David Gibson wrote:
> On Wed, Nov 17, 2021 at 05:14:30PM -0300, lagar...@linux.ibm.com wrote:
>> From: Leonardo Garcia 
>>
>> The purpose of this document is to substitute the content currently
>> available in the QEMU wiki at [0]. This initial version does contain
>> some additional content as well. Whenever this documentation gets
>> upstream and is reflected in [1], the QEMU wiki will be edited to point
>> to this documentation, so that we only need to keep it updated in one
>> place.
>>
>> 0. https://wiki.qemu.org/Documentation/Platforms/POWER
>> 1. https://qemu.readthedocs.io/en/latest/system/ppc/pseries.html
>>
>> Signed-off-by: Leonardo Garcia 
> LGTM,
>
> Reviewed-by: David Gibson 


Thanks for the review.


Minor comments below.


>
> Couple of minor points below.
>
>> ---
>>  docs/system/ppc/pseries.rst | 185 
>>  1 file changed, 185 insertions(+)
>>
>> diff --git a/docs/system/ppc/pseries.rst b/docs/system/ppc/pseries.rst
>> index 932d4dd17d..2de3fb4d51 100644
>> --- a/docs/system/ppc/pseries.rst
>> +++ b/docs/system/ppc/pseries.rst
>> @@ -1,12 +1,197 @@
>>  pSeries family boards (``pseries``)
>>  ===
>>  
>> +The Power machine virtualized environment described by the `Linux on Power
>> +Architecture Reference document (LoPAR)
>> +`_
>> +is called pseries. This environment is also known as sPAPR, System p 
>> guests, or
>> +simply Power Linux guests (although it is capable of running other operating
>> +systems, such as AIX).
>> +
>> +Even though pseries is designed to behave as a guest environment, it is also
>> +capable of acting as a hypervisor OS, providing, on that role, nested
>> +virtualization capabilities.
>> +
>>  Supported devices
>>  -
>>  
>> + * Multi processor support for many Power processors generations: POWER5+,
>> +   POWER7, POWER7+, POWER8, POWER8NVL, Power9, and Power10 (there is no 
>> support
>> +   for POWER6 processors).
> I wouldn't trust the POWER5+ cpu emulation with pseries; only POWER7
> and later has been tested at all.  Actually.. I wouldn't trust the
> POWER5+ CPU emulation much at all, if it's been tested, it's not for a
> long, long time.


Sure. Removed POWER5+ from the list of supported processors and put a
note saying that the code is there but the state is unknown in v2.


>> + * Interrupt Controller, XICS (POWER8) and XIVE (Power9 and Power10)
>> + * vPHB PCIe Host bridge.
>> + * vscsi and vnet devices, compatible with the same devices available on a
>> +   PowerVM hypervisor with VIOS managing LPARs.
>> + * Virtio based devices.
>> + * PCIe device pass through.
>> +
>>  Missing devices
>>  ---
>>  
>> + * SPICE support.
>>  
>>  Firmware
>>  
>> +
>> +`SLOF `_ (Slimline Open Firmware) is an
>> +implementation of the `IEEE 1275-1994, Standard for Boot (Initialization
>> +Configuration) Firmware: Core Requirements and Practices
>> +`_.
>> +
>> +QEMU includes a prebuilt image of SLOF which is updated when a more recent
>> +version is required.
>> +
>> +Build directions
>> +
>> +
>> +.. code-block:: bash
>> +
>> +  ./configure --target-list=ppc64-softmmu && make
>> +
>> +Running instructions
>> +
>> +
>> +Someone can select the pseries machine type by running QEMU with the 
>> following
>> +options:
>> +
>> +.. code-block:: bash
>> +
>> +  qemu-system-ppc64 -M pseries 
>> +
>> +sPAPR devices
>> +-
>> +
>> +The sPAPR specification defines a set of para-virtualized devices, which are
>> +also supported by the pseries machine in QEMU and can be instantiated with 
>> the
>> +`-device` option:
>> +
>> +* spapr-vlan : A virtual network interface.
>> +* spapr-vscsi : A virtual SCSI disk interface.
>> +* spapr-rng : A pseudo-device for passing random number generator data to 
>> the
>> +  guest (see the `H_RANDOM hypercall feature
>> +  `_ for details).
>> +
>> +These are compatible with the devices historically available for use when
>> +running the IBM PowerVM hypervisor with LPARs.
>> +
>> +However, since these devices have originally been specified with another
>> +hypervisor and non-Linux guests in mind, you should use the virtio 
>> counterparts
>> +(virtio-net, virtio-blk/scsi and virtio-rng) if possible instead, since they
>> +will most probably give you better performance with Linux guests in a QEMU
>> +environment.
>> +
>> +The pseries machine in QEMU is always instantiated with a NVRAM device
>> +(``spapr-nvram``), so it is not needed to add this manually. However, if 
>> someone
>> +wants to make the contents of the NVRAM device persistent, they will need to
>> +specify a PFLASH device when starting QEMU, i.e. either use
>> +``-drive if=pflash,file=,format=raw`` to set the default

Re: [PATCH] docs: Minor updates on the powernv documentation.

2021-11-23 Thread Leonardo Augusto Guimarães Garcia 
On 11/18/21 06:03, Cédric Le Goater wrote:
> On 11/17/21 21:16, lagar...@linux.ibm.com wrote:
>> From: Leonardo Garcia 
>>
>> Signed-off-by: Leonardo Garcia 
>> ---
>>   docs/system/ppc/powernv.rst | 56 +++--
>>   1 file changed, 29 insertions(+), 27 deletions(-)
>>
>> diff --git a/docs/system/ppc/powernv.rst b/docs/system/ppc/powernv.rst
>> index 86186b7d2c..907c4ce4f9 100644
>> --- a/docs/system/ppc/powernv.rst
>> +++ b/docs/system/ppc/powernv.rst
>> @@ -1,7 +1,7 @@
>> -PowerNV family boards (``powernv8``, ``powernv9``)
>> +PowerNV family boards (``powernv8``, ``powernv9``, ``power10``)
>
> powernv10.
>
> And I still haven't resent the XIVE2/PHB5 patches to enable full support.
>
>>   ==
>>   -PowerNV (as Non-Virtualized) is the "baremetal" platform using the
>> +PowerNV (as Non-Virtualized) is the "bare metal" platform using the
>>   OPAL firmware. It runs Linux on IBM and OpenPOWER systems and it can
>>   be used as an hypervisor OS, running KVM guests, or simply as a host
>>   OS.
>> @@ -15,17 +15,15 @@ beyond the scope of what QEMU addresses today.
>>   Supported devices
>>   -
>>   - * Multi processor support for POWER8, POWER8NVL and POWER9.
>> - * XSCOM, serial communication sideband bus to configure chiplets
>> - * Simple LPC Controller
>> - * Processor Service Interface (PSI) Controller
>> - * Interrupt Controller, XICS (POWER8) and XIVE (POWER9)
>> - * POWER8 PHB3 PCIe Host bridge and POWER9 PHB4 PCIe Host bridge
>> - * Simple OCC is an on-chip microcontroller used for power management
>> -   tasks
>> - * iBT device to handle BMC communication, with the internal BMC
>> -   simulator provided by QEMU or an external BMC such as an Aspeed
>> -   QEMU machine.
>> + * Multi processor support for POWER8, POWER8NVL and Power9.
>
> s/Power9/POWER9/ right ? I am starting to doubt :)


Well... ...

According to IBM new branding rules (I wasn't able to find any external
documents on this but we were asked to use it externally), the processor
name is now Power instead of POWER. As the document I have access to
only references Power9 and Power10, I am interpreting this as "don't
mess with old names", specially as I don't want to start rewriting
documents and comments because of these marketing changes of humor. TBH,
I am OK either way.

I will include the other suggestions in a new patch and resubmit.

Cheers,

Leo


>
>> + * XSCOM, serial communication sideband bus to configure chiplets.
>> + * Simple LPC Controller.
>> + * Processor Service Interface (PSI) Controller.
>> + * Interrupt Controller, XICS (POWER8) and XIVE (Power9) and XIVE2
>> (Power10).
>> + * POWER8 PHB3 PCIe Host bridge and POWER9 PHB4 PCIe Host bridge.
>> + * Simple OCC is an on-chip micro-controller used for power
>> management tasks.
>> + * iBT device to handle BMC communication, with the internal BMC
>> simulator
>> +   provided by QEMU or an external BMC such as an Aspeed QEMU machine.
>>    * PNOR containing the different firmware partitions.
>>     Missing devices
>> @@ -33,27 +31,25 @@ Missing devices
>>     A lot is missing, among which :
>>   - * POWER10 processor
>> - * XIVE2 (POWER10) interrupt controller
>> - * I2C controllers (yet to be merged)
>> - * NPU/NPU2/NPU3 controllers
>> - * EEH support for PCIe Host bridge controllers
>> - * NX controller
>> - * VAS controller
>> - * chipTOD (Time Of Day)
>> + * I2C controllers (yet to be merged).
>> + * NPU/NPU2/NPU3 controllers.
>> + * EEH support for PCIe Host bridge controllers.
>> + * NX controller.
>> + * VAS controller.
>> + * chipTOD (Time Of Day).
>>    * Self Boot Engine (SBE).
>> - * FSI bus
>> + * FSI bus.
>>     Firmware
>>   
>>     The OPAL firmware (OpenPower Abstraction Layer) for OpenPower
>> systems
>>   includes the runtime services ``skiboot`` and the bootloader kernel
>> and
>> -initramfs ``skiroot``. Source code can be found on GitHub:
>> +initramfs ``skiroot``. Source code can be found on the `OpenPOWER
>> account at
>> +GitHub `_.
>>   -  https://github.com/open-power.
>> -
>> -Prebuilt images of ``skiboot`` and ``skiroot`` are made available on
>> the `OpenPOWER `__
>> site.
>> +Prebuilt images of ``skiboot`` and ``skiroot`` are made available on
>> the
>> +`OpenPOWER `__ site.
>>     QEMU includes a prebuilt image of ``skiboot`` which is updated
>> when a
>>   more recent version is required by the models.
>> @@ -83,6 +79,7 @@ and a SATA disk :
>>     Complex PCIe configuration
>>   ~~
>> +
>>   Six PHBs are defined per chip (POWER9) but no default PCI layout is
>>   provided (to be compatible with libvirt). One PCI device can be added
>>   on any of the available PCIe slots using command line options such as:
>> @@ -157,7 +154,7 @@ one on the command line :
>>   The files

Re: Editing QEMU POWER Platform wiki page

2021-02-22 Thread Leonardo Augusto Guimarães Garcia 

On 2/22/21 8:01 AM, Greg Kurz wrote:

On Thu, 18 Feb 2021 10:16:25 -0300
Leonardo Augusto Guimarães Garcia  wrote:


Hi there,

I would like to edit the wiki page at [0] as it contains some outdated
information. Could anyone that has access to the wiki please help me
create a user so that I can edit it?

0. https://wiki.qemu.org/Documentation/Platforms/POWER


Hi Leo,

User creation isn't publicly available to avoid spam : only an existing
user can create a new account.



Yeah, I saw that. That's why I asked here.



This being said, wiki isn't the preferred
way to expose documentation since there's no review and things ultimately
bitrot. Page [0] you want to update is a perfect example of the mess :
not only it contains irrelevant data but also stuff that is definitely
wrong (e.g. 'compat' cpu property was deprecated in QEMU 5.0 and will
be removed in QEMU 6.0).

Ideally we'd want everything to be in the main QEMU doc and don't
even need a wiki.

On the PowerPC front, the most up-to-date docs are in the QEMU tree:

docs/system/ppc/embedded.rst
docs/system/ppc/powermac.rst
docs/system/ppc/powernv.rst
docs/system/ppc/prep.rst
docs/system/ppc/pseries.rst
docs/system/target-ppc.rst

So I don't know exactly what changes you had in mind, but maybe first
consider to update the main documentation.



I got here because someone pointed to me the wiki is saying that nested 
virtualization is not supported on Power, which is wrong. But I saw many 
other outdated information on the wiki as you pointed out.





On my side, I think I want do ditch all the current content and just put
links to https://qemu.readthedocs.io/en/latest/ instead. I can take care
of that, in which case you wouldn't need an account.



I agree this would be the preferable way. Could you go ahead and do 
that, please, if others agree as well?


Cheers,

Leo




Cheers,

--
Greg

PS:

Cedric reported that we also have a page for non-pseries
platforms:

https://wiki.qemu.org/Documentation/Platforms/PowerPC

I'm Cc'ing some regular contributors for those platforms so
they can  evaluate the bitrotting status of this wiki.


Cheers,

Leo

Re: How to get a wiki account?

2021-02-19 Thread Leonardo Augusto Guimarães Garcia 

On 2/18/21 8:56 PM, Philippe Mathieu-Daudé wrote:

On 2/19/21 12:23 AM, Taylor Simpson wrote:

How do I get a wiki account for wiki.qemu.org?  Going forward, I’d like
to be able to add information about the Hexagon target.

As any user with access to the wiki can create user accounts,
I created yours and will send you your information off-list.



I submitted a similar request yesterday to this list. Could you please 
help me creating an account as well, Phi?


Cheers,

Leo




Regards,

Phil.

Editing QEMU POWER Platform wiki page

2021-02-18 Thread Leonardo Augusto Guimarães Garcia 

Hi there,

I would like to edit the wiki page at [0] as it contains some outdated 
information. Could anyone that has access to the wiki please help me 
create a user so that I can edit it?


0. https://wiki.qemu.org/Documentation/Platforms/POWER

Cheers,

Leo

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-02-17 Thread Leonardo Augusto Guimarães Garcia 

On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:

On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagar...@linux.ibm.com wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
  hw/virtio/vhost-user-fs-pci.c | 7 +++
  hw/virtio/vhost-user-fs.c | 5 +
  2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
   * top-level directory.
   */
  
+#include "qemu/osdep.h"

+#include "qapi/error.h"
  #include "qemu/osdep.h"
  #include "hw/qdev-properties.h"
  #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
  vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
  }
  
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {

+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}

Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?

What needs to be added to support ATS?



As I am working on v2 for this patch, I revisited this. There is no need 
for this check. ATS works fine. The only problem is with the 
iommu_platform flag.



Cheers,


Leo





+
  qdev_realize(vdev, BUS(_dev->bus), errp);
  }
  
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c

index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {

+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}
+
  if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {

I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

 ret = vhost_dev_init(>vhost_dev, >vhost_user,
  VHOST_BACKEND_TYPE_USER, 0);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "vhost_dev_init failed");
 goto err_virtio;
 }
+
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+   !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+   error_setg(errp, "IOMMU is not supported by the vhost-user device 
backend");
+   goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.

Stefan

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-01-28 Thread Leonardo Augusto Guimarães Garcia 

On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:

On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagar...@linux.ibm.com wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
  hw/virtio/vhost-user-fs-pci.c | 7 +++
  hw/virtio/vhost-user-fs.c | 5 +
  2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
   * top-level directory.
   */
  
+#include "qemu/osdep.h"

+#include "qapi/error.h"
  #include "qemu/osdep.h"
  #include "hw/qdev-properties.h"
  #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
  vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
  }
  
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {

+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}

Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?

What needs to be added to support ATS?


+
  qdev_realize(vdev, BUS(_dev->bus), errp);
  }
  
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c

index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {

+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}
+
  if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {

I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

 ret = vhost_dev_init(>vhost_dev, >vhost_user,
  VHOST_BACKEND_TYPE_USER, 0);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "vhost_dev_init failed");
 goto err_virtio;
 }
+
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+   !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+   error_setg(errp, "IOMMU is not supported by the vhost-user device 
backend");
+   goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.



I am afraid I will need some additional guidance on how to do that. If I 
am reading the code correctly, the vhost-user devices don't follow any 
specific pattern. Looking at the vhost-user-fs code path, we have:


vuf_device_realize -> vhost_dev_init -> vhost_user_backend_init

So, I thought that naturally, if the check was in vuf_device_realize on 
my previous patch, I should move it to vhost_user_backend_init. However, 
in order to check if the VirtIODevice has been specified with the 
VIRTIO_F_IOMMU_PLATFORM option, I would need to have access to the 
VirtIODevice inside vhost_user_backend_init, which currently is not 
available and not one of the arguments of vhost_backend_init virtual 
function it implements. vhost_dev_init doesn't have access to 
VirIODevices as well. Looking at other device types that call 
vhost_dev_init, not all of them initialized the VirtIODevice by the time 
vhost_dev_init is called (e.g. cryptodev-host). Hence, adding a 
VirtIODevice as parameter to vhost_dev_init and vhost_backedn_init is 
not a feasible solution. vhost_dev_init does receive structu vhost_dev 
which has a field VirtIODevice vdev. But as the VirtIODevice hasn't been 
initialized by the time vhost_dev_init is called on all vhost-user 
devices today also makes this not a solution.


Any ideas on this? Is it correct for a virtio-user devices to call 
vhost_dev_init before having VirtIODevice ready?


Leo




Stefan

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-01-27 Thread Leonardo Augusto Guimarães Garcia 

On 1/27/21 4:40 PM, Dr. David Alan Gilbert wrote:

* Leonardo Augusto Guimarães Garcia (lagar...@br.ibm.com) wrote:

On 1/27/21 1:34 PM, Dr. David Alan Gilbert wrote:

* Leonardo Augusto Guimarães Garcia (lagar...@linux.ibm.com) wrote:

On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:

On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagar...@linux.ibm.com wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
hw/virtio/vhost-user-fs-pci.c | 7 +++
hw/virtio/vhost-user-fs.c | 5 +
2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
 * top-level directory.
 */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/osdep.h"
#include "hw/qdev-properties.h"
#include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
}
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {
+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}

Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?

What needs to be added to support ATS?


+
qdev_realize(vdev, BUS(_dev->bus), errp);
}
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
return;
}
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}
+
if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {

I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

   ret = vhost_dev_init(>vhost_dev, >vhost_user,
VHOST_BACKEND_TYPE_USER, 0);
   if (ret < 0) {
   error_setg_errno(errp, -ret, "vhost_dev_init failed");
   goto err_virtio;
   }
+
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+   !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+   error_setg(errp, "IOMMU is not supported by the vhost-user device 
backend");
+   goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.

Further analyzing this, on vhost-user.c, I see:

        if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) &&
                !(virtio_has_feature(dev->protocol_features,
                    VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
                 virtio_has_feature(dev->protocol_features,
                    VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
            error_report("IOMMU support requires reply-ack and "
                         "slave-req protocol 
features.");
            return -1;
        }

This code was included by commit 6dcdd06. It included support for
VIRTIO_F_IOMMU_PLATFORM with vhost_net devices. So, the restriction is not
generic for all vhost-user devices.

That test is a slightly different test; that's checking that the
vhost-user device has two underlying features that are needed to make
iommu work; it's not a full test though; it doesn't actually check the
vhost-user device also wants to do iommu.


Right... but Stefan was suggesting to move the check I proposed to avoid
VIRTIO_F_IOMMU_PLATFORM on vhost-user-fs up to vhost-user. What I understand
from the above code excerpt and the rest of commit 6dcdd06 is that IOMMU
support has been added into vhost-user already. However, it seems
vhost-user-fs (maybe all other devices on top of vhost-user) doesn't support
it yet. If I move the check up to vhost-user, does it make sense to still
have all the IOMMU code support there?

Libvhost-user isn't just used by virtiofs; so it can have IOMMU code in
(I'm not convinced it was complete though); it just needs to make sure
that the device is also happy to do IOMMU, as Stefan's code did.



Oh, I think I finally got what Stefan and you are saying. Thanks for the 
additional clarification!


Leo




Dave


Leo



Dave


Cheers,

Leo


Stefan

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-01-27 Thread Leonardo Augusto Guimarães Garcia 

On 1/27/21 1:34 PM, Dr. David Alan Gilbert wrote:

* Leonardo Augusto Guimarães Garcia (lagar...@linux.ibm.com) wrote:

On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:

On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagar...@linux.ibm.com wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
   hw/virtio/vhost-user-fs-pci.c | 7 +++
   hw/virtio/vhost-user-fs.c | 5 +
   2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
* top-level directory.
*/
+#include "qemu/osdep.h"
+#include "qapi/error.h"
   #include "qemu/osdep.h"
   #include "hw/qdev-properties.h"
   #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
   vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
   }
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {
+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}

Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?

What needs to be added to support ATS?


+
   qdev_realize(vdev, BUS(_dev->bus), errp);
   }
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
   return;
   }
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}
+
   if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {

I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

  ret = vhost_dev_init(>vhost_dev, >vhost_user,
   VHOST_BACKEND_TYPE_USER, 0);
  if (ret < 0) {
  error_setg_errno(errp, -ret, "vhost_dev_init failed");
  goto err_virtio;
  }
+
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+   !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+   error_setg(errp, "IOMMU is not supported by the vhost-user device 
backend");
+   goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.

Further analyzing this, on vhost-user.c, I see:

        if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) &&
                !(virtio_has_feature(dev->protocol_features,
                    VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
                 virtio_has_feature(dev->protocol_features,
                    VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
            error_report("IOMMU support requires reply-ack and "
                         "slave-req protocol 
features.");
            return -1;
        }

This code was included by commit 6dcdd06. It included support for
VIRTIO_F_IOMMU_PLATFORM with vhost_net devices. So, the restriction is not
generic for all vhost-user devices.

That test is a slightly different test; that's checking that the
vhost-user device has two underlying features that are needed to make
iommu work; it's not a full test though; it doesn't actually check the
vhost-user device also wants to do iommu.



Right... but Stefan was suggesting to move the check I proposed to avoid 
VIRTIO_F_IOMMU_PLATFORM on vhost-user-fs up to vhost-user. What I 
understand from the above code excerpt and the rest of commit 6dcdd06 is 
that IOMMU support has been added into vhost-user already. However, it 
seems vhost-user-fs (maybe all other devices on top of vhost-user) 
doesn't support it yet. If I move the check up to vhost-user, does it 
make sense to still have all the IOMMU code support there?


Leo




Dave


Cheers,

Leo


Stefan

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-01-27 Thread Leonardo Augusto Guimarães Garcia 

On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:

On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagar...@linux.ibm.com wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
  hw/virtio/vhost-user-fs-pci.c | 7 +++
  hw/virtio/vhost-user-fs.c | 5 +
  2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
   * top-level directory.
   */
  
+#include "qemu/osdep.h"

+#include "qapi/error.h"
  #include "qemu/osdep.h"
  #include "hw/qdev-properties.h"
  #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
  vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
  }
  
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {

+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}

Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?

What needs to be added to support ATS?


+
  qdev_realize(vdev, BUS(_dev->bus), errp);
  }
  
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c

index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {

+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}
+
  if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {

I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

 ret = vhost_dev_init(>vhost_dev, >vhost_user,
  VHOST_BACKEND_TYPE_USER, 0);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "vhost_dev_init failed");
 goto err_virtio;
 }
+
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+   !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+   error_setg(errp, "IOMMU is not supported by the vhost-user device 
backend");
+   goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.


Further analyzing this, on vhost-user.c, I see:

    if (virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM) &&
    !(virtio_has_feature(dev->protocol_features,
    VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
 virtio_has_feature(dev->protocol_features,
    VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
    error_report("IOMMU support requires reply-ack and "
 "slave-req protocol features.");
    return -1;
    }

This code was included by commit 6dcdd06. It included support for 
VIRTIO_F_IOMMU_PLATFORM with vhost_net devices. So, the restriction is 
not generic for all vhost-user devices.


Cheers,

Leo



Stefan

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-01-27 Thread Leonardo Augusto Guimarães Garcia 

On 1/27/21 8:19 AM, Stefan Hajnoczi wrote:

On Tue, Jan 26, 2021 at 03:23:38PM -0300, lagar...@linux.ibm.com wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
  hw/virtio/vhost-user-fs-pci.c | 7 +++
  hw/virtio/vhost-user-fs.c | 5 +
  2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
   * top-level directory.
   */
  
+#include "qemu/osdep.h"

+#include "qapi/error.h"
  #include "qemu/osdep.h"
  #include "hw/qdev-properties.h"
  #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
  vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
  }
  
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {

+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}

Why is this check needed in addition to VIRTIO_F_IOMMU_PLATFORM?



I don't know if VIRTIO_PCI_FLAG_ATS should depend on 
VIRTIO_F_IOMMU_PLATFORM. At least from a code perspective today, they 
are completely independent. A user can specify one or the other or both. 
And if a user specifies VIRTIO_PCI_FLAG_ATS without specifying 
VIRTIO_F_IOMMU_PLATFORM, the same issue described in the original 
message will happen inside the guest.





What needs to be added to support ATS?



Unfortunately I don't know the answer for this question. Hopefully 
someone else can help with this one.






+
  qdev_realize(vdev, BUS(_dev->bus), errp);
  }
  
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c

index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {

+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}
+
  if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {

I thought IOMMU support depends on the vhost-user device backend (e.g.
virtiofsd), so the vhost-user backend should participate in advertising
this feature.

Perhaps the check should be:

 ret = vhost_dev_init(>vhost_dev, >vhost_user,
  VHOST_BACKEND_TYPE_USER, 0);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "vhost_dev_init failed");
 goto err_virtio;
 }
+
+   if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
+   !(fs->vhost_dev.hdev_features & (1ull << VIRTIO_F_IOMMU_PLATFORM))) {
+   error_setg(errp, "IOMMU is not supported by the vhost-user device 
backend");
+   goto err_iommu_needed;
+   }

Also, can this logic be made generic for all vhost-user devices? It's
not really specific to vhost-user-fs.



Sure, I can do that. I wasn't sure whether this restriction was only for 
vhost-user-fs or whether it was generic for all vhost-user devices. I 
will include this in a next version of the patch.



Cheers,


Leo




Stefan

Re: [PATCH] IOMMU and ATS not supported by vhost-user filesystem.

2021-01-26 Thread Leonardo Augusto Guimarães Garcia 

On 1/26/21 4:45 PM, Dr. David Alan Gilbert wrote:

* lagar...@linux.ibm.com (lagar...@linux.ibm.com) wrote:

From: Leonardo Garcia 

Currently, as IOMMU and ATS are not supported, if a user mistakenly set
any of them and tries to mount the vhost-user filesystem inside the
guest, whenever the user tries to access the mount point, the system
will hang forever.

Signed-off-by: Leonardo Garcia 
---
  hw/virtio/vhost-user-fs-pci.c | 7 +++
  hw/virtio/vhost-user-fs.c | 5 +
  2 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index 2ed8492b3f..564d1fd108 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -11,6 +11,8 @@
   * top-level directory.
   */
  
+#include "qemu/osdep.h"

+#include "qapi/error.h"
  #include "qemu/osdep.h"
  #include "hw/qdev-properties.h"
  #include "hw/virtio/vhost-user-fs.h"
@@ -45,6 +47,11 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
  vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
  }
  
+if (vpci_dev->flags & VIRTIO_PCI_FLAG_ATS) {

+error_setg(errp, "ATS is currently not supported with 
vhost-user-fs-pci");
+return;
+}
+
  qdev_realize(vdev, BUS(_dev->bus), errp);
  }
  
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c

index ac4fc34b36..914d68b3ee 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -203,6 +203,11 @@ static void vuf_device_realize(DeviceState *dev, Error 
**errp)
  return;
  }
  
+if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {

+error_setg(errp, "IOMMU is currently not supported with 
vhost-user-fs");
+return;
+}

Yes, I've seen this problem - however, I'm a little confused; isn't the
negotiation of features on virtio supposed to happen automatically?
If so, how come it's managing to set VIRTIO_F_IOMMU_PLATFORM?



It is easy to set by passing iommu_platform=on on the QEMU command line. 
Something like this:


-device 
vhost-user-fs-pci,chardev=chr-vu-fs0,queue-size=512,tag=myfs,iommu_platform=on,ats=on


I understand this is a user error, but QEMU should not allow this 
configuration as it doesn't work.


Cheers,

Leo




Dave


  if (!vhost_user_init(>vhost_user, >conf.chardev, errp)) {
  return;
  }
--
2.29.2