On 10/17/25 3:15 PM, Shameer Kolothum wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <[email protected]>
>> Sent: 17 October 2025 13:47
>> To: Shameer Kolothum <[email protected]>; qemu-
>> [email protected]; [email protected]
>> Cc: [email protected]; Jason Gunthorpe <[email protected]>; Nicolin
>> Chen <[email protected]>; [email protected]; [email protected];
>> Nathan Chen <[email protected]>; Matt Ochs <[email protected]>;
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v4 06/27] hw/arm/smmuv3-accel: Restrict accelerated
>> SMMUv3 to vfio-pci endpoints with iommufd
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hi Shameer,
>>
>> On 10/2/25 11:30 AM, Shameer Kolothum wrote:
>>> Hi Eric,
>>>
>>>> -----Original Message-----
>>>> From: Eric Auger <[email protected]>
>>>> Sent: 01 October 2025 18:32
>>>> To: Shameer Kolothum <[email protected]>; qemu-
>>>> [email protected]; [email protected]
>>>> Cc: [email protected]; Jason Gunthorpe <[email protected]>; Nicolin
>>>> Chen <[email protected]>; [email protected]; [email protected];
>>>> Nathan Chen <[email protected]>; Matt Ochs <[email protected]>;
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; [email protected]; [email protected];
>>>> [email protected]
>>>> Subject: Re: [PATCH v4 06/27] hw/arm/smmuv3-accel: Restrict accelerated
>>>> SMMUv3 to vfio-pci endpoints with iommufd
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>> Hi Shameer,
>>>>
>>>> On 9/29/25 3:36 PM, Shameer Kolothum wrote:
>>>>> Accelerated SMMUv3 is only useful when the device can take advantage
>>>>> of the host's SMMUv3 in nested mode. To keep things simple and
>>>>> correct, we only allow this feature for vfio-pci endpoint devices that
>>>>> use the iommufd backend. We also allow non-endpoint emulated devices
>>>>> like PCI bridges and root ports, so that users can plug in these
>>>>> vfio-pci devices. We can only enforce this if devices are cold
>>>>> plugged. For hotplug cases, give appropriate
>>>> "We can only enforce this if devices are cold plugged": I don't really
>>>> understand that statement.
>>> By "enforce" here I meant, we can prevent user from starting a Guest
>>> with a non "vfio-pci/iommufd dev" with accel=one case.
>> Ah OK I misread the code. I thought you were also exiting in case of
>> hotplug but you only issue a warn_report.
>> From a user point of view, the assigned device will succeed attachment
>> but won't work. Will we get subsequent messages?
> It will work. But as the warning says, it may degrade the performance
> especially
> If the SMMUv3 has other vfio-pci devices. Because the TLB invalidations
> from emulated one will be issued to host SMMUv3 as well.
>
> I understand the pain
>> of propagating the error but if the user experience is bad I think it
>> should weight over ?
> I am not against it. But can be taken up as a separate one if required.
>
>>> Please let me know your thoughts.
>> Can't you move the assignment of bus->devices[devfn] before the call and
>> unset it in case of failure?
>>
>> Or if you propagate errors from
>>
>> get_address_space() you could retry the call later?
> For now, I have a fix like below that seems to do the
> Job.
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index c9932c87e3..9693d7f10c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1370,9 +1370,6 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
> pci_dev->bus_master_as.max_bounce_buffer_size =
> pci_dev->max_bounce_buffer_size;
>
> - if (phase_check(PHASE_MACHINE_READY)) {
> - pci_init_bus_master(pci_dev);
> - }
> pci_dev->irq_state = 0;
> pci_config_alloc(pci_dev);
>
> @@ -1416,6 +1413,9 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
> pci_dev->config_write = config_write;
> bus->devices[devfn] = pci_dev;
> pci_dev->version_id = 2; /* Current pci device vmstate version */
> + if (phase_check(PHASE_MACHINE_READY)) {
> + pci_init_bus_master(pci_dev);
> + }
> return pci_dev;
OK worth putting it in a separate patch to allow finer review of PCI
maintainers.
Thanks
Eric
> }
>
> Thanks,
> Shameer
