Re: [PATCH v3 1/2] s390x/pci: add support for guests that request direct mapping

[Matthew Rosato]

>>  
>>  static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn)
>> @@ -1488,6 +1518,8 @@ static const Property s390_pci_device_properties[] = {
>>      DEFINE_PROP_BOOL("interpret", S390PCIBusDevice, interp, true),
>>      DEFINE_PROP_BOOL("forwarding-assist", S390PCIBusDevice, 
>> forwarding_assist,
>>                       true),
>> +    DEFINE_PROP_BOOL("relaxed-translation", S390PCIBusDevice, rtr_allowed,
>> +                     true),
> 
> Question: Do we maybe want to default rtr_allowed to false for ISM
> devices? Performance wise it doesn't matter much since they keep their
> mappings fairly static and it would help us catch bugs in the handling
> of rtr_allowed == false devices, the ISM driver and increase security.
> 

I've been asking myself the same.  Believe we've discussed it in the past 
(off-list) too.

I'd be fine with that.  I think it doesn't change the line above, but I can add 
a check against pft in s390_pcihost_plug() after clp info is read in, along 
with a small comment, that changes the setting to false for ISM devices.

Note that there is one side effect I can think of based on the kernel 
implementation - if a guest has, say, NVMe and ISM passed through with 
iommu.passthrough=1 specified then they would always see the "Falling back to 
IOMMU_DOMAIN_DMA" message for the ISM device(s) because of rtr_allowed == false.


>>      /*
>>       * If appropriate, reduce the size of the supported DMA aperture 
>> reported
>> -     * to the guest based upon the vfio DMA limit.
>> +     * to the guest based upon the vfio DMA limit.  This is applicable for
>> +     * devices that are guaranteed to not use relaxed translation.  If the
>> +     * device is capable of relaxed translation then we must advertise the
>> +     * full aperture.  In this case, if translation is used then we will
>> +     * rely on the vfio DMA limit counting and use RPCIT CC1 / status 16
>> +     * to request the guest free DMA mappings when necessary.
> 
> Not a native speaker but I think there is a "to" missing in the last
> sentence and I'd have used "as necessary".

'request that the' would probably work too...  But yeah, something is missing, 
I'll re-word.


>> @@ -362,6 +364,7 @@ struct S390PCIBusDevice {
>>      bool interp;
>>      bool forwarding_assist;
>>      bool aif;
>> +    bool rtr_allowed;
> 
> Nit: In the kernel in struct zpci_dev you used rtr_avail but "allowed"
> in the comment, just for gerppability I'd prefer the names to match.

I guess in my head, QEMU is the one allowing it and the guest kernel is 
checking whether or not it's available.

But I have no strong opinion on the name; can rename the QEMU variable to 
rtr_avail

> 
>>      QTAILQ_ENTRY(S390PCIBusDevice) link;
>>  };
>>  
>> @@ -389,6 +392,7 @@ int pci_chsc_sei_nt2_have_event(void);
>>  void s390_pci_sclp_configure(SCCB *sccb);
>>  void s390_pci_sclp_deconfigure(SCCB *sccb);
>>  void s390_pci_iommu_enable(S390PCIIOMMU *iommu);
>> +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu);
> 
> Nit: I find "_dm_" a bit hard to map to "direct map". If you want two
> letters I'd go for "_pt_" for "_iommu_pass_through_" or maybe
> "_direct_map_".

OK