Re: [PATCH v3 2/2] spapr: Enable virtio iommu_platform=on by default

[Alexey Kardashevskiy]

On 10/03/2020 21:43, Greg Kurz wrote:
> On Thu, 5 Mar 2020 12:59:03 +0100
> Greg Kurz <address@hidden> wrote:
> 
>> On Thu,  5 Mar 2020 15:30:09 +1100
>> David Gibson <address@hidden> wrote:
>>
>>> Traditionally, virtio devices don't do DMA by the usual path on the
>>> guest platform.  In particular they usually bypass any virtual IOMMU
>>> the guest has, using hypervisor magic to access untranslated guest
>>> physical addresses.
>>>
>>> There's now the optional iommu_platform flag which can tell virtio
>>> devices to use the platform's normal DMA path, including any IOMMUs.
>>> That flag was motiviated for the case of hardware virtio
>>> implementations, but there are other reasons to want it.
>>>
>>> Specifically, the fact that the virtio device doesn't use vIOMMU
>>> translation means that virtio devices are unsafe to pass to nested
>>> guests, or to use with VFIO userspace drivers inside the guest.  This
>>> is particularly noticeable on the pseries platform which *always* has
>>> a guest-visible vIOMMU.
>>>
>>> Not using the normal DMA path also causes difficulties for the guest
>>> side driver when using the upcoming POWER Secure VMs (a.k.a. PEF).
>>> While it's theoretically possible to handle this on the guest side,
>>> it's really fiddly.  Given the other problems with the non-translated
>>> virtio device, let's just enable vIOMMU translation for virtio devices
>>> by default in the pseries-5.0 (and later) machine types.
>>>
>>> This does mean the new machine type will no longer support guest
>>> kernels older than 4.8, unless they have support for the virtio
>>> IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7
>>> do).
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>
>> The patch looks good but I'm not sure if we're quite ready to merge
>> it yet. With this applied, I get zero output on a virtio-serial based
>> console:
>>
>> ie.
>>   -chardev stdio,id=con0 -device virtio-serial -device 
>> virtconsole,chardev=con0 
>>
>> FYI, virtio-serial is a bit broken for spapr with iommu_platform=off already:
>>
>> (1) pressing a key in the console during SLOF or grub has no effect
>>
>> (2) the guest kernel boot stays stuck around quiesce
>>
>> These are regressions introduced by this SLOF update:
>>
>> a363e9ed8731f45674260932a340a0d81c4b0a6f is the first bad commit
>> commit a363e9ed8731f45674260932a340a0d81c4b0a6f
>> Author: Alexey Kardashevskiy <address@hidden>
>> Date:   Tue Dec 17 11:31:54 2019 +1100
>>     pseries: Update SLOF firmware image
>>
>> A trivial fix was already posted on the SLOF list for (1) :
>>
>> https://patchwork.ozlabs.org/patch/1249338/
>>
>> (2) is still under investigation but the console is _at least_
>> functional until the guest OS takes control. This is no longer
>> the case with this patch.
>>
> 
> Some progress was made on the SLOF front:
> 
> https://patchwork.ozlabs.org/project/slof/list/?series=163314
> 
> With these series applied to SLOF, I can now boot a fedora31 guest
> with a virtio-serial console and iommu_platform=on... but now
> I'm trying out other virtio devices supported by SLOF and I'm
> running into issues around virtio-pci.disable-legacy as mentioned
> in some other mail...
> 
> It seems we may not be ready to merge this series yet.


fwiw I sent a pull request:

https://lore.kernel.org/qemu-devel/address@hidden/T/#u



> 
>>>  hw/ppc/spapr.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 3cfc98ac61..5ef099536e 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -4575,6 +4575,7 @@ static void 
>>> spapr_machine_latest_class_options(MachineClass *mc)
>>>       */
>>>      static GlobalProperty compat[] = {
>>>          { TYPE_VIRTIO_PCI, "disable-legacy", "on", },
>>> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", },
>>>      };
>>>  
>>>      mc->alias = "pseries";
>>> @@ -4622,6 +4623,7 @@ static void 
>>> spapr_machine_4_2_class_options(MachineClass *mc)
>>>      SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>>>      static GlobalProperty compat[] = {
>>>          { TYPE_VIRTIO_PCI, "disable-legacy", "auto" },
>>> +        { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", },
>>>      };
>>>  
>>>      spapr_machine_5_0_class_options(mc);
>>
> 

-- 
Alexey