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