Re: [PATCH v2 1/4] IOMMU/x86: switch to alternatives-call patching in further instances

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>, "paul@xen.org" <paul@xen.org>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 1/4] IOMMU/x86: switch to alternatives-call patching in further instances
Date: Fri, 28 Jan 2022 10:54:10 +0000	[thread overview]
Message-ID: <06b427ba-e353-cd54-a212-c0b82c78632f@citrix.com> (raw)
In-Reply-To: <ecaf1c37-0d70-77e6-2343-68bb25c167b8@suse.com>

On 28/01/2022 10:36, Jan Beulich wrote:
> On 28.01.2022 10:28, Durrant, Paul wrote:
>> On 27/01/2022 14:47, Jan Beulich wrote:
>>> @@ -1457,24 +1462,24 @@ static int iommu_get_device_group(
>>>       if ( !is_iommu_enabled(d) || !ops->get_device_group_id )
>>>           return 0;
>>>   
>>> -    group_id = ops->get_device_group_id(seg, bus, devfn);
>>> +    group_id = iommu_call(ops, get_device_group_id, seg, bus, devfn);
>>>   
>>>       pcidevs_lock();
>>>       for_each_pdev( d, pdev )
>>>       {
>>> -        if ( (pdev->seg != seg) ||
>>> -             ((pdev->bus == bus) && (pdev->devfn == devfn)) )
>>> +        unsigned int b = pdev->bus;
>>> +        unsigned int df = pdev->devfn;
>>> +
>>> +        if ( (pdev->seg != seg) || ((b == bus) && (df == devfn)) )
>>>               continue;
>>>   
>>> -        if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (pdev->bus << 8) | pdev->devfn) )
>>> +        if ( xsm_get_device_group(XSM_HOOK, (seg << 16) | (b << 8) | df) )
>>>               continue;
>>>   
>>> -        sdev_id = ops->get_device_group_id(seg, pdev->bus, pdev->devfn);
>>> +        sdev_id = iommu_call(ops, get_device_group_id, seg, b, df);
>>>           if ( (sdev_id == group_id) && (i < max_sdevs) )
>>>           {
>>> -            bdf = 0;
>>> -            bdf |= (pdev->bus & 0xff) << 16;
>>> -            bdf |= (pdev->devfn & 0xff) << 8;
>>> +            bdf = (b << 16) | (df << 8);
>> Don't we have a macro for this now? Probably best to start using it 
>> whilst modifying the code.
> We don't. And it would feel somewhat misleading to use PCI_BDF2(b, df) << 8
> here. The situation is even worse imo: Besides there not being a macro, I
> also cannot seem to find any documentation on this non-standard layout (BDF
> shifted left by 8). Yet then again I also can't spot any caller of
> xc_get_device_group() ...

I'm sure I already did the archaeology.

device groups were broken by a hypercall bounce buffering change 2 years
before the only caller was dropped with Xend.

This mess of a hypercall has demonstrably not been used in a decade.  I
firmly suggest dropping it, rather than wasting effort trying to unbreak
an interface which needs deleting anyway as the first step to doing
IOMMU groups.

~Andrew