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

2022-01-28 Thread Andrew Cooper 
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

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

2022-01-28 Thread Rahul Singh 
Hi Jan,

> On 27 Jan 2022, at 2:47 pm, Jan Beulich  wrote:
> 
> This is, once again, to limit the number of indirect calls as much as
> possible. The only hook invocation which isn't sensible to convert is
> setup(). And of course Arm-only use sites are left alone as well.
> 
> Note regarding the introduction / use of local variables in pci.c:
> struct pci_dev's involved fields are const. This const propagates, via
> typeof(), to the local helper variables in the altcall macros. These
> helper variables are, however, used as outputs (and hence can't be
> const). In iommu_get_device_group() make use of the new local variables
> to also simplify some adjacent code.
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Rahul Singh 
Tested-by: Rahul Singh 

Regards,
Rahul
> 
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -198,7 +198,7 @@ int iommu_domain_init(struct domain *d,
> return ret;
> 
> hd->platform_ops = iommu_get_ops();
> -ret = hd->platform_ops->init(d);
> +ret = iommu_call(hd->platform_ops, init, d);
> if ( ret || is_system_domain(d) )
> return ret;
> 
> @@ -233,7 +233,7 @@ void __hwdom_init iommu_hwdom_init(struc
> 
> register_keyhandler('o', _dump_page_tables, "dump iommu page 
> tables", 0);
> 
> -hd->platform_ops->hwdom_init(d);
> +iommu_vcall(hd->platform_ops, hwdom_init, d);
> }
> 
> static void iommu_teardown(struct domain *d)
> @@ -576,7 +576,7 @@ int iommu_get_reserved_device_memory(iom
> if ( !ops->get_reserved_device_memory )
> return 0;
> 
> -return ops->get_reserved_device_memory(func, ctxt);
> +return iommu_call(ops, get_reserved_device_memory, func, ctxt);
> }
> 
> bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
> @@ -603,7 +603,7 @@ static void iommu_dump_page_tables(unsig
> continue;
> }
> 
> -dom_iommu(d)->platform_ops->dump_page_tables(d);
> +iommu_vcall(dom_iommu(d)->platform_ops, dump_page_tables, d);
> }
> 
> rcu_read_unlock(_read_lock);
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -861,15 +861,15 @@ static int deassign_device(struct domain
> devfn += pdev->phantom_stride;
> if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
> break;
> -ret = hd->platform_ops->reassign_device(d, target, devfn,
> -pci_to_dev(pdev));
> +ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
> + pci_to_dev(pdev));
> if ( ret )
> goto out;
> }
> 
> devfn = pdev->devfn;
> -ret = hd->platform_ops->reassign_device(d, target, devfn,
> -pci_to_dev(pdev));
> +ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
> + pci_to_dev(pdev));
> if ( ret )
> goto out;
> 
> @@ -1300,7 +1300,7 @@ static int iommu_add_device(struct pci_d
> {
> const struct domain_iommu *hd;
> int rc;
> -u8 devfn;
> +unsigned int devfn = pdev->devfn;
> 
> if ( !pdev->domain )
> return -EINVAL;
> @@ -1311,16 +1311,16 @@ static int iommu_add_device(struct pci_d
> if ( !is_iommu_enabled(pdev->domain) )
> return 0;
> 
> -rc = hd->platform_ops->add_device(pdev->devfn, pci_to_dev(pdev));
> +rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
> if ( rc || !pdev->phantom_stride )
> return rc;
> 
> -for ( devfn = pdev->devfn ; ; )
> +for ( ; ; )
> {
> devfn += pdev->phantom_stride;
> if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
> return 0;
> -rc = hd->platform_ops->add_device(devfn, pci_to_dev(pdev));
> +rc = iommu_call(hd->platform_ops, add_device, devfn, 
> pci_to_dev(pdev));
> if ( rc )
> printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>>sbdf, rc);
> @@ -1341,7 +1341,7 @@ static int iommu_enable_device(struct pc
>  !hd->platform_ops->enable_device )
> return 0;
> 
> -return hd->platform_ops->enable_device(pci_to_dev(pdev));
> +return iommu_call(hd->platform_ops, enable_device, pci_to_dev(pdev));
> }
> 
> static int iommu_remove_device(struct pci_dev *pdev)
> @@ -1363,7 +1363,8 @@ static int iommu_remove_device(struct pc
> devfn += pdev->phantom_stride;
> if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
> break;
> -rc = hd->platform_ops->remove_device(devfn, pci_to_dev(pdev));
> +rc = iommu_call(hd->platform_ops, remove_device, devfn,
> +pci_to_dev(pdev));
> if ( !rc )
> continue;
> 
> @@ -1371,7 +1372,9 @@ static int iommu_remove_device(struct pc
> return rc;
> }
> 
> -return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));
> +devfn = pdev->devfn;
> +
> +return

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

2022-01-28 Thread Jan Beulich 
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() ...

> Reviewed-by: Paul Durrant 

Thanks.

Jan

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

2022-01-28 Thread Durrant, Paul 

On 27/01/2022 14:47, Jan Beulich wrote:

This is, once again, to limit the number of indirect calls as much as
possible. The only hook invocation which isn't sensible to convert is
setup(). And of course Arm-only use sites are left alone as well.

Note regarding the introduction / use of local variables in pci.c:
struct pci_dev's involved fields are const. This const propagates, via
typeof(), to the local helper variables in the altcall macros. These
helper variables are, however, used as outputs (and hence can't be
const). In iommu_get_device_group() make use of the new local variables
to also simplify some adjacent code.

Signed-off-by: Jan Beulich 

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -198,7 +198,7 @@ int iommu_domain_init(struct domain *d,
  return ret;
  
  hd->platform_ops = iommu_get_ops();

-ret = hd->platform_ops->init(d);
+ret = iommu_call(hd->platform_ops, init, d);
  if ( ret || is_system_domain(d) )
  return ret;
  
@@ -233,7 +233,7 @@ void __hwdom_init iommu_hwdom_init(struc
  
  register_keyhandler('o', _dump_page_tables, "dump iommu page tables", 0);
  
-hd->platform_ops->hwdom_init(d);

+iommu_vcall(hd->platform_ops, hwdom_init, d);
  }
  
  static void iommu_teardown(struct domain *d)

@@ -576,7 +576,7 @@ int iommu_get_reserved_device_memory(iom
  if ( !ops->get_reserved_device_memory )
  return 0;
  
-return ops->get_reserved_device_memory(func, ctxt);

+return iommu_call(ops, get_reserved_device_memory, func, ctxt);
  }
  
  bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)

@@ -603,7 +603,7 @@ static void iommu_dump_page_tables(unsig
  continue;
  }
  
-dom_iommu(d)->platform_ops->dump_page_tables(d);

+iommu_vcall(dom_iommu(d)->platform_ops, dump_page_tables, d);
  }
  
  rcu_read_unlock(_read_lock);

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -861,15 +861,15 @@ static int deassign_device(struct domain
  devfn += pdev->phantom_stride;
  if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
  break;
-ret = hd->platform_ops->reassign_device(d, target, devfn,
-pci_to_dev(pdev));
+ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
+ pci_to_dev(pdev));
  if ( ret )
  goto out;
  }
  
  devfn = pdev->devfn;

-ret = hd->platform_ops->reassign_device(d, target, devfn,
-pci_to_dev(pdev));
+ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
+ pci_to_dev(pdev));
  if ( ret )
  goto out;
  
@@ -1300,7 +1300,7 @@ static int iommu_add_device(struct pci_d

  {
  const struct domain_iommu *hd;
  int rc;
-u8 devfn;
+unsigned int devfn = pdev->devfn;
  
  if ( !pdev->domain )

  return -EINVAL;
@@ -1311,16 +1311,16 @@ static int iommu_add_device(struct pci_d
  if ( !is_iommu_enabled(pdev->domain) )
  return 0;
  
-rc = hd->platform_ops->add_device(pdev->devfn, pci_to_dev(pdev));

+rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
  if ( rc || !pdev->phantom_stride )
  return rc;
  
-for ( devfn = pdev->devfn ; ; )

+for ( ; ; )
  {
  devfn += pdev->phantom_stride;
  if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
  return 0;
-rc = hd->platform_ops->add_device(devfn, pci_to_dev(pdev));
+rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
  if ( rc )
  printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
 >sbdf, rc);
@@ -1341,7 +1341,7 @@ static int iommu_enable_device(struct pc
   !hd->platform_ops->enable_device )
  return 0;
  
-return hd->platform_ops->enable_device(pci_to_dev(pdev));

+return iommu_call(hd->platform_ops, enable_device, pci_to_dev(pdev));
  }
  
  static int iommu_remove_device(struct pci_dev *pdev)

@@ -1363,7 +1363,8 @@ static int iommu_remove_device(struct pc
  devfn += pdev->phantom_stride;
  if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
  break;
-rc = hd->platform_ops->remove_device(devfn, pci_to_dev(pdev));
+rc = iommu_call(hd->platform_ops, remove_device, devfn,
+pci_to_dev(pdev));
  if ( !rc )
  continue;
  
@@ -1371,7 +1372,9 @@ static int iommu_remove_device(struct pc

  return rc;
  }
  
-return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));

+devfn = pdev->devfn;
+
+return iommu_call(hd->platform_ops, remove_device, devfn, 
pci_to_dev(pdev));
  }
  
  static int device_assigned(u16 seg, u8 bus, u8 devfn)

@@ -1421,7 +1424,8 @@ static int assign_device(struct domain *

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

2022-01-27 Thread Jan Beulich 
This is, once again, to limit the number of indirect calls as much as
possible. The only hook invocation which isn't sensible to convert is
setup(). And of course Arm-only use sites are left alone as well.

Note regarding the introduction / use of local variables in pci.c:
struct pci_dev's involved fields are const. This const propagates, via
typeof(), to the local helper variables in the altcall macros. These
helper variables are, however, used as outputs (and hence can't be
const). In iommu_get_device_group() make use of the new local variables
to also simplify some adjacent code.

Signed-off-by: Jan Beulich 

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -198,7 +198,7 @@ int iommu_domain_init(struct domain *d,
 return ret;
 
 hd->platform_ops = iommu_get_ops();
-ret = hd->platform_ops->init(d);
+ret = iommu_call(hd->platform_ops, init, d);
 if ( ret || is_system_domain(d) )
 return ret;
 
@@ -233,7 +233,7 @@ void __hwdom_init iommu_hwdom_init(struc
 
 register_keyhandler('o', _dump_page_tables, "dump iommu page 
tables", 0);
 
-hd->platform_ops->hwdom_init(d);
+iommu_vcall(hd->platform_ops, hwdom_init, d);
 }
 
 static void iommu_teardown(struct domain *d)
@@ -576,7 +576,7 @@ int iommu_get_reserved_device_memory(iom
 if ( !ops->get_reserved_device_memory )
 return 0;
 
-return ops->get_reserved_device_memory(func, ctxt);
+return iommu_call(ops, get_reserved_device_memory, func, ctxt);
 }
 
 bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
@@ -603,7 +603,7 @@ static void iommu_dump_page_tables(unsig
 continue;
 }
 
-dom_iommu(d)->platform_ops->dump_page_tables(d);
+iommu_vcall(dom_iommu(d)->platform_ops, dump_page_tables, d);
 }
 
 rcu_read_unlock(_read_lock);
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -861,15 +861,15 @@ static int deassign_device(struct domain
 devfn += pdev->phantom_stride;
 if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
 break;
-ret = hd->platform_ops->reassign_device(d, target, devfn,
-pci_to_dev(pdev));
+ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
+ pci_to_dev(pdev));
 if ( ret )
 goto out;
 }
 
 devfn = pdev->devfn;
-ret = hd->platform_ops->reassign_device(d, target, devfn,
-pci_to_dev(pdev));
+ret = iommu_call(hd->platform_ops, reassign_device, d, target, devfn,
+ pci_to_dev(pdev));
 if ( ret )
 goto out;
 
@@ -1300,7 +1300,7 @@ static int iommu_add_device(struct pci_d
 {
 const struct domain_iommu *hd;
 int rc;
-u8 devfn;
+unsigned int devfn = pdev->devfn;
 
 if ( !pdev->domain )
 return -EINVAL;
@@ -1311,16 +1311,16 @@ static int iommu_add_device(struct pci_d
 if ( !is_iommu_enabled(pdev->domain) )
 return 0;
 
-rc = hd->platform_ops->add_device(pdev->devfn, pci_to_dev(pdev));
+rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
 if ( rc || !pdev->phantom_stride )
 return rc;
 
-for ( devfn = pdev->devfn ; ; )
+for ( ; ; )
 {
 devfn += pdev->phantom_stride;
 if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
 return 0;
-rc = hd->platform_ops->add_device(devfn, pci_to_dev(pdev));
+rc = iommu_call(hd->platform_ops, add_device, devfn, pci_to_dev(pdev));
 if ( rc )
 printk(XENLOG_WARNING "IOMMU: add %pp failed (%d)\n",
>sbdf, rc);
@@ -1341,7 +1341,7 @@ static int iommu_enable_device(struct pc
  !hd->platform_ops->enable_device )
 return 0;
 
-return hd->platform_ops->enable_device(pci_to_dev(pdev));
+return iommu_call(hd->platform_ops, enable_device, pci_to_dev(pdev));
 }
 
 static int iommu_remove_device(struct pci_dev *pdev)
@@ -1363,7 +1363,8 @@ static int iommu_remove_device(struct pc
 devfn += pdev->phantom_stride;
 if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
 break;
-rc = hd->platform_ops->remove_device(devfn, pci_to_dev(pdev));
+rc = iommu_call(hd->platform_ops, remove_device, devfn,
+pci_to_dev(pdev));
 if ( !rc )
 continue;
 
@@ -1371,7 +1372,9 @@ static int iommu_remove_device(struct pc
 return rc;
 }
 
-return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));
+devfn = pdev->devfn;
+
+return iommu_call(hd->platform_ops, remove_device, devfn, 
pci_to_dev(pdev));
 }
 
 static int device_assigned(u16 seg, u8 bus, u8 devfn)
@@ -1421,7 +1424,8 @@ static int assign_device(struct domain *
 
 pdev->fault.count = 0;
 
-if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
flag)) )
+if (