Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi Jan,

> On 21 Apr 2021, at 10:33 am, Jan Beulich  wrote:
> 
> On 21.04.2021 11:15, Rahul Singh wrote:
>> Hi Roger,
>> 
>>> On 21 Apr 2021, at 9:16 am, Roger Pau Monné  wrote:
>>> 
>>> On Wed, Apr 21, 2021 at 08:07:08AM +, Rahul Singh wrote:
 Hi Jan,
 
> On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:
> 
> On 20.04.2021 15:45, Rahul Singh wrote:
>>> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
>>> On 19.04.2021 13:54, Julien Grall wrote:
 For the time being, I think move this code in x86 is a lot better than 
 #ifdef or keep the code in common code.
>>> 
>>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
>>> I would perhaps not agree if there was a new CONFIG_* which other
>>> (future) arch-es could select if desired.
>> 
>> I agree with Julien moving the code to x86 file as currently it is 
>> referenced only in x86 code
>> and as of now we are not sure how other architecture will implement the 
>> Interrupt remapping
>> (via IOMMU or any other means).  
>> 
>> Let me know if you are ok with moving the code to x86.
> 
> I can't answer this with "yes" or "no" without knowing what the 
> alternative
> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
> If a separate CONFIG_* gets introduced (and selected by X86), then a
> separate file (getting built only when that new setting is y) would seem
> better to me.
 
 I just made a quick patch. Please let me know if below patch is ok. I move 
 the definition to  "passthrough/x86/iommu.c” file.
 
 diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
 index 705137f8be..199ce08612 100644
 --- a/xen/drivers/passthrough/pci.c
 +++ b/xen/drivers/passthrough/pci.c
 @@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
 }
 __initcall(setup_dump_pcidevs);
 
 -int iommu_update_ire_from_msi(
 -struct msi_desc *msi_desc, struct msi_msg *msg)
 -{
 -return iommu_intremap
 -   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 
 0;
 -}
 -
 static int iommu_add_device(struct pci_dev *pdev)
 {
const struct domain_iommu *hd;
 diff --git a/xen/drivers/passthrough/x86/iommu.c 
 b/xen/drivers/passthrough/x86/iommu.c
 index b90bb31bfe..cf51dec564 100644
 --- a/xen/drivers/passthrough/x86/iommu.c
 +++ b/xen/drivers/passthrough/x86/iommu.c
 @@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
likely(!p2m_get_hostp2m(d)->global_logdirty));
 }
 
 +int iommu_update_ire_from_msi(
 +struct msi_desc *msi_desc, struct msi_msg *msg)
 +{
 +return iommu_intremap
 +   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 
 0;
 +}
 +
 /*
 * Local variables:
 * mode: C
 diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
 index ea0cd0f1a2..bd42d87b72 100644
 --- a/xen/include/xen/iommu.h
 +++ b/xen/include/xen/iommu.h
 @@ -243,7 +243,6 @@ struct iommu_ops {
   u8 devfn, device_t *dev);
 #ifdef CONFIG_HAS_PCI
int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
 -int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
 *msg);
 #endif /* HAS_PCI */
 
void (*teardown)(struct domain *d);
 @@ -272,6 +271,7 @@ struct iommu_ops {
int (*adjust_irq_affinities)(void);
void (*sync_cache)(const void *addr, unsigned int size);
void (*clear_root_pgtable)(struct domain *d);
 +int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
 *msg);
>>> 
>>> You also need to move the function prototype
>>> (iommu_update_ire_from_msi) from iommu.h into asm-x86/iommu.h, or
>>> maybe you could just define the function itself as static inline in
>>> asm-x86/iommu.h?
>> 
>> 
>> Ok. I will move function prototype (iommu_update_ire_from_msi) from 
>> iommu.h into asm-x86/iommu.h.
>> 
>> I first tried to make the function as static inline in "asm-x86/iommu.h" but 
>> after 
>> that I observe the compilation error for debug build because "asm/iommu.h” 
>> is included in "xen/iommu.h” before iommu_call() is defined in "xen/iommu.h”.
>> That's why I decided to move it to the "passthrough/x86/iommu.c” file.
> 
> Which in turn would be an argument for keeping it in xen/iommu.h and
> wrap it in an #ifdef.
> 

Ok. I will move the definition of iommu_update_ire_from_msi() to 
"xen/iommu.h” and will wrap it under CONFIG_X86

If everyone is ok with the above approach then I will send the next
version of the patch series for review.

Regards,
Rahul

> Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 21.04.2021 11:15, Rahul Singh wrote:
> Hi Roger,
> 
>> On 21 Apr 2021, at 9:16 am, Roger Pau Monné  wrote:
>>
>> On Wed, Apr 21, 2021 at 08:07:08AM +, Rahul Singh wrote:
>>> Hi Jan,
>>>
 On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:

 On 20.04.2021 15:45, Rahul Singh wrote:
>> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
>> On 19.04.2021 13:54, Julien Grall wrote:
>>> For the time being, I think move this code in x86 is a lot better than 
>>> #ifdef or keep the code in common code.
>>
>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
>> I would perhaps not agree if there was a new CONFIG_* which other
>> (future) arch-es could select if desired.
>
> I agree with Julien moving the code to x86 file as currently it is 
> referenced only in x86 code
> and as of now we are not sure how other architecture will implement the 
> Interrupt remapping
> (via IOMMU or any other means).  
>
> Let me know if you are ok with moving the code to x86.

 I can't answer this with "yes" or "no" without knowing what the alternative
 would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
 If a separate CONFIG_* gets introduced (and selected by X86), then a
 separate file (getting built only when that new setting is y) would seem
 better to me.
>>>
>>> I just made a quick patch. Please let me know if below patch is ok. I move 
>>> the definition to  "passthrough/x86/iommu.c” file.
>>>
>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>> index 705137f8be..199ce08612 100644
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
>>> }
>>> __initcall(setup_dump_pcidevs);
>>>
>>> -int iommu_update_ire_from_msi(
>>> -struct msi_desc *msi_desc, struct msi_msg *msg)
>>> -{
>>> -return iommu_intremap
>>> -   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 
>>> 0;
>>> -}
>>> -
>>> static int iommu_add_device(struct pci_dev *pdev)
>>> {
>>> const struct domain_iommu *hd;
>>> diff --git a/xen/drivers/passthrough/x86/iommu.c 
>>> b/xen/drivers/passthrough/x86/iommu.c
>>> index b90bb31bfe..cf51dec564 100644
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
>>> likely(!p2m_get_hostp2m(d)->global_logdirty));
>>> }
>>>
>>> +int iommu_update_ire_from_msi(
>>> +struct msi_desc *msi_desc, struct msi_msg *msg)
>>> +{
>>> +return iommu_intremap
>>> +   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 
>>> 0;
>>> +}
>>> +
>>> /*
>>>  * Local variables:
>>>  * mode: C
>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>> index ea0cd0f1a2..bd42d87b72 100644
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -243,7 +243,6 @@ struct iommu_ops {
>>>u8 devfn, device_t *dev);
>>> #ifdef CONFIG_HAS_PCI
>>> int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
>>> -int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
>>> *msg);
>>> #endif /* HAS_PCI */
>>>
>>> void (*teardown)(struct domain *d);
>>> @@ -272,6 +271,7 @@ struct iommu_ops {
>>> int (*adjust_irq_affinities)(void);
>>> void (*sync_cache)(const void *addr, unsigned int size);
>>> void (*clear_root_pgtable)(struct domain *d);
>>> +int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
>>> *msg);
>>
>> You also need to move the function prototype
>> (iommu_update_ire_from_msi) from iommu.h into asm-x86/iommu.h, or
>> maybe you could just define the function itself as static inline in
>> asm-x86/iommu.h?
> 
> 
> Ok. I will move function prototype (iommu_update_ire_from_msi) from 
> iommu.h into asm-x86/iommu.h.
> 
> I first tried to make the function as static inline in "asm-x86/iommu.h" but 
> after 
> that I observe the compilation error for debug build because "asm/iommu.h” 
> is included in "xen/iommu.h” before iommu_call() is defined in "xen/iommu.h”.
> That's why I decided to move it to the "passthrough/x86/iommu.c” file.

Which in turn would be an argument for keeping it in xen/iommu.h and
wrap it in an #ifdef.

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 21.04.2021 10:16, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 08:07:08AM +, Rahul Singh wrote:
>> Hi Jan,
>>
>>> On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:
>>>
>>> On 20.04.2021 15:45, Rahul Singh wrote:
> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
> On 19.04.2021 13:54, Julien Grall wrote:
>> For the time being, I think move this code in x86 is a lot better than 
>> #ifdef or keep the code in common code.
>
> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
> I would perhaps not agree if there was a new CONFIG_* which other
> (future) arch-es could select if desired.

 I agree with Julien moving the code to x86 file as currently it is 
 referenced only in x86 code
 and as of now we are not sure how other architecture will implement the 
 Interrupt remapping
 (via IOMMU or any other means).  

 Let me know if you are ok with moving the code to x86.
>>>
>>> I can't answer this with "yes" or "no" without knowing what the alternative
>>> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
>>> If a separate CONFIG_* gets introduced (and selected by X86), then a
>>> separate file (getting built only when that new setting is y) would seem
>>> better to me.
>>
>> I just made a quick patch. Please let me know if below patch is ok. I move 
>> the definition to  "passthrough/x86/iommu.c” file.
>>
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 705137f8be..199ce08612 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
>>  }
>>  __initcall(setup_dump_pcidevs);
>>  
>> -int iommu_update_ire_from_msi(
>> -struct msi_desc *msi_desc, struct msi_msg *msg)
>> -{
>> -return iommu_intremap
>> -   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> -}
>> -
>>  static int iommu_add_device(struct pci_dev *pdev)
>>  {
>>  const struct domain_iommu *hd;
>> diff --git a/xen/drivers/passthrough/x86/iommu.c 
>> b/xen/drivers/passthrough/x86/iommu.c
>> index b90bb31bfe..cf51dec564 100644
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
>>  likely(!p2m_get_hostp2m(d)->global_logdirty));
>>  }
>>  
>> +int iommu_update_ire_from_msi(
>> +struct msi_desc *msi_desc, struct msi_msg *msg)
>> +{
>> +return iommu_intremap
>> +   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index ea0cd0f1a2..bd42d87b72 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -243,7 +243,6 @@ struct iommu_ops {
>> u8 devfn, device_t *dev);
>>  #ifdef CONFIG_HAS_PCI
>>  int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
>> -int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
>> *msg);
>>  #endif /* HAS_PCI */
>>  
>>  void (*teardown)(struct domain *d);
>> @@ -272,6 +271,7 @@ struct iommu_ops {
>>  int (*adjust_irq_affinities)(void);
>>  void (*sync_cache)(const void *addr, unsigned int size);
>>  void (*clear_root_pgtable)(struct domain *d);
>> +int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
>> *msg);
> 
> You also need to move the function prototype
> (iommu_update_ire_from_msi) from iommu.h into asm-x86/iommu.h,

The prototype can, in principle at least, remain where it is.

> or maybe you could just define the function itself as static inline in
> asm-x86/iommu.h?

Possibly (and in that case it would perhaps indeed be better to
move it there, compared to needing another #ifdef).

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi Roger,

> On 21 Apr 2021, at 9:16 am, Roger Pau Monné  wrote:
> 
> On Wed, Apr 21, 2021 at 08:07:08AM +, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:
>>> 
>>> On 20.04.2021 15:45, Rahul Singh wrote:
> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
> On 19.04.2021 13:54, Julien Grall wrote:
>> For the time being, I think move this code in x86 is a lot better than 
>> #ifdef or keep the code in common code.
> 
> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
> I would perhaps not agree if there was a new CONFIG_* which other
> (future) arch-es could select if desired.
 
 I agree with Julien moving the code to x86 file as currently it is 
 referenced only in x86 code
 and as of now we are not sure how other architecture will implement the 
 Interrupt remapping
 (via IOMMU or any other means).  
 
 Let me know if you are ok with moving the code to x86.
>>> 
>>> I can't answer this with "yes" or "no" without knowing what the alternative
>>> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
>>> If a separate CONFIG_* gets introduced (and selected by X86), then a
>>> separate file (getting built only when that new setting is y) would seem
>>> better to me.
>> 
>> I just made a quick patch. Please let me know if below patch is ok. I move 
>> the definition to  "passthrough/x86/iommu.c” file.
>> 
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 705137f8be..199ce08612 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
>> }
>> __initcall(setup_dump_pcidevs);
>> 
>> -int iommu_update_ire_from_msi(
>> -struct msi_desc *msi_desc, struct msi_msg *msg)
>> -{
>> -return iommu_intremap
>> -   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> -}
>> -
>> static int iommu_add_device(struct pci_dev *pdev)
>> {
>> const struct domain_iommu *hd;
>> diff --git a/xen/drivers/passthrough/x86/iommu.c 
>> b/xen/drivers/passthrough/x86/iommu.c
>> index b90bb31bfe..cf51dec564 100644
>> --- a/xen/drivers/passthrough/x86/iommu.c
>> +++ b/xen/drivers/passthrough/x86/iommu.c
>> @@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
>> likely(!p2m_get_hostp2m(d)->global_logdirty));
>> }
>> 
>> +int iommu_update_ire_from_msi(
>> +struct msi_desc *msi_desc, struct msi_msg *msg)
>> +{
>> +return iommu_intremap
>> +   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> +}
>> +
>> /*
>>  * Local variables:
>>  * mode: C
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index ea0cd0f1a2..bd42d87b72 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -243,7 +243,6 @@ struct iommu_ops {
>>u8 devfn, device_t *dev);
>> #ifdef CONFIG_HAS_PCI
>> int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
>> -int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
>> *msg);
>> #endif /* HAS_PCI */
>> 
>> void (*teardown)(struct domain *d);
>> @@ -272,6 +271,7 @@ struct iommu_ops {
>> int (*adjust_irq_affinities)(void);
>> void (*sync_cache)(const void *addr, unsigned int size);
>> void (*clear_root_pgtable)(struct domain *d);
>> +int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
>> *msg);
> 
> You also need to move the function prototype
> (iommu_update_ire_from_msi) from iommu.h into asm-x86/iommu.h, or
> maybe you could just define the function itself as static inline in
> asm-x86/iommu.h?


Ok. I will move function prototype (iommu_update_ire_from_msi) from 
iommu.h into asm-x86/iommu.h.

I first tried to make the function as static inline in "asm-x86/iommu.h" but 
after 
that I observe the compilation error for debug build because "asm/iommu.h” 
is included in "xen/iommu.h” before iommu_call() is defined in "xen/iommu.h”.
That's why I decided to move it to the "passthrough/x86/iommu.c” file.

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/xen/iommu.h;h=ea0cd0f1a2a4558eab4488468272f3ed35dd1dc0;hb=HEAD#l298

Regards,
Rahul
> 
> Thanks, Roger.

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 21.04.2021 10:07, Rahul Singh wrote:
> Hi Jan,
> 
>> On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:
>>
>> On 20.04.2021 15:45, Rahul Singh wrote:
 On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
 On 19.04.2021 13:54, Julien Grall wrote:
> For the time being, I think move this code in x86 is a lot better than 
> #ifdef or keep the code in common code.

 Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
 I would perhaps not agree if there was a new CONFIG_* which other
 (future) arch-es could select if desired.
>>>
>>> I agree with Julien moving the code to x86 file as currently it is 
>>> referenced only in x86 code
>>> and as of now we are not sure how other architecture will implement the 
>>> Interrupt remapping
>>> (via IOMMU or any other means).  
>>>
>>> Let me know if you are ok with moving the code to x86.
>>
>> I can't answer this with "yes" or "no" without knowing what the alternative
>> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
>> If a separate CONFIG_* gets introduced (and selected by X86), then a
>> separate file (getting built only when that new setting is y) would seem
>> better to me.
> 
> I just made a quick patch. Please let me know if below patch is ok. I move 
> the definition to  "passthrough/x86/iommu.c” file.

This patch on its own looks okay, but assumes you've already taken the
decision that no proper new CONFIG_* would want introducing. That
decision, however, touches (aiui) more than just this one hook.

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné]

On Wed, Apr 21, 2021 at 08:07:08AM +, Rahul Singh wrote:
> Hi Jan,
> 
> > On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:
> > 
> > On 20.04.2021 15:45, Rahul Singh wrote:
> >>> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
> >>> On 19.04.2021 13:54, Julien Grall wrote:
>  For the time being, I think move this code in x86 is a lot better than 
>  #ifdef or keep the code in common code.
> >>> 
> >>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
> >>> I would perhaps not agree if there was a new CONFIG_* which other
> >>> (future) arch-es could select if desired.
> >> 
> >> I agree with Julien moving the code to x86 file as currently it is 
> >> referenced only in x86 code
> >> and as of now we are not sure how other architecture will implement the 
> >> Interrupt remapping
> >> (via IOMMU or any other means).  
> >> 
> >> Let me know if you are ok with moving the code to x86.
> > 
> > I can't answer this with "yes" or "no" without knowing what the alternative
> > would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
> > If a separate CONFIG_* gets introduced (and selected by X86), then a
> > separate file (getting built only when that new setting is y) would seem
> > better to me.
> 
> I just made a quick patch. Please let me know if below patch is ok. I move 
> the definition to  "passthrough/x86/iommu.c” file.
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 705137f8be..199ce08612 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
>  }
>  __initcall(setup_dump_pcidevs);
>  
> -int iommu_update_ire_from_msi(
> -struct msi_desc *msi_desc, struct msi_msg *msg)
> -{
> -return iommu_intremap
> -   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
> -}
> -
>  static int iommu_add_device(struct pci_dev *pdev)
>  {
>  const struct domain_iommu *hd;
> diff --git a/xen/drivers/passthrough/x86/iommu.c 
> b/xen/drivers/passthrough/x86/iommu.c
> index b90bb31bfe..cf51dec564 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
>  likely(!p2m_get_hostp2m(d)->global_logdirty));
>  }
>  
> +int iommu_update_ire_from_msi(
> +struct msi_desc *msi_desc, struct msi_msg *msg)
> +{
> +return iommu_intremap
> +   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index ea0cd0f1a2..bd42d87b72 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -243,7 +243,6 @@ struct iommu_ops {
> u8 devfn, device_t *dev);
>  #ifdef CONFIG_HAS_PCI
>  int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
> -int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
> *msg);
>  #endif /* HAS_PCI */
>  
>  void (*teardown)(struct domain *d);
> @@ -272,6 +271,7 @@ struct iommu_ops {
>  int (*adjust_irq_affinities)(void);
>  void (*sync_cache)(const void *addr, unsigned int size);
>  void (*clear_root_pgtable)(struct domain *d);
> +int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
> *msg);

You also need to move the function prototype
(iommu_update_ire_from_msi) from iommu.h into asm-x86/iommu.h, or
maybe you could just define the function itself as static inline in
asm-x86/iommu.h?

Thanks, Roger.

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi Jan,

> On 20 Apr 2021, at 4:36 pm, Jan Beulich  wrote:
> 
> On 20.04.2021 15:45, Rahul Singh wrote:
>>> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
>>> On 19.04.2021 13:54, Julien Grall wrote:
 For the time being, I think move this code in x86 is a lot better than 
 #ifdef or keep the code in common code.
>>> 
>>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
>>> I would perhaps not agree if there was a new CONFIG_* which other
>>> (future) arch-es could select if desired.
>> 
>> I agree with Julien moving the code to x86 file as currently it is 
>> referenced only in x86 code
>> and as of now we are not sure how other architecture will implement the 
>> Interrupt remapping
>> (via IOMMU or any other means).  
>> 
>> Let me know if you are ok with moving the code to x86.
> 
> I can't answer this with "yes" or "no" without knowing what the alternative
> would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
> If a separate CONFIG_* gets introduced (and selected by X86), then a
> separate file (getting built only when that new setting is y) would seem
> better to me.

I just made a quick patch. Please let me know if below patch is ok. I move the 
definition to  "passthrough/x86/iommu.c” file.

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..199ce08612 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1303,13 +1303,6 @@ static int __init setup_dump_pcidevs(void)
 }
 __initcall(setup_dump_pcidevs);
 
-int iommu_update_ire_from_msi(
-struct msi_desc *msi_desc, struct msi_msg *msg)
-{
-return iommu_intremap
-   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
-}
-
 static int iommu_add_device(struct pci_dev *pdev)
 {
 const struct domain_iommu *hd;
diff --git a/xen/drivers/passthrough/x86/iommu.c 
b/xen/drivers/passthrough/x86/iommu.c
index b90bb31bfe..cf51dec564 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -340,6 +340,13 @@ bool arch_iommu_use_permitted(const struct domain *d)
 likely(!p2m_get_hostp2m(d)->global_logdirty));
 }
 
+int iommu_update_ire_from_msi(
+struct msi_desc *msi_desc, struct msi_msg *msg)
+{
+return iommu_intremap
+   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index ea0cd0f1a2..bd42d87b72 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -243,7 +243,6 @@ struct iommu_ops {
u8 devfn, device_t *dev);
 #ifdef CONFIG_HAS_PCI
 int (*get_device_group_id)(u16 seg, u8 bus, u8 devfn);
-int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
 #endif /* HAS_PCI */
 
 void (*teardown)(struct domain *d);
@@ -272,6 +271,7 @@ struct iommu_ops {
 int (*adjust_irq_affinities)(void);
 void (*sync_cache)(const void *addr, unsigned int size);
 void (*clear_root_pgtable)(struct domain *d);
+int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg *msg);
 #endif /* CONFIG_X86 */
 
 int __must_check (*suspend)(void)

Regards,
Rahul
> 
> Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 20.04.2021 15:45, Rahul Singh wrote:
>> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
>> On 19.04.2021 13:54, Julien Grall wrote:
>>> For the time being, I think move this code in x86 is a lot better than 
>>> #ifdef or keep the code in common code.
>>
>> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
>> I would perhaps not agree if there was a new CONFIG_* which other
>> (future) arch-es could select if desired.
> 
> I agree with Julien moving the code to x86 file as currently it is referenced 
> only in x86 code
> and as of now we are not sure how other architecture will implement the 
> Interrupt remapping
> (via IOMMU or any other means).  
> 
> Let me know if you are ok with moving the code to x86.

I can't answer this with "yes" or "no" without knowing what the alternative
would be. As said, if the alternative is CONFIG_X86 #ifdef-ary, then yes.
If a separate CONFIG_* gets introduced (and selected by X86), then a
separate file (getting built only when that new setting is y) would seem
better to me.

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi Jan

> On 19 Apr 2021, at 1:33 pm, Jan Beulich  wrote:
> 
> On 19.04.2021 13:54, Julien Grall wrote:
>> For the time being, I think move this code in x86 is a lot better than 
>> #ifdef or keep the code in common code.
> 
> Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
> I would perhaps not agree if there was a new CONFIG_* which other
> (future) arch-es could select if desired.

I agree with Julien moving the code to x86 file as currently it is referenced 
only in x86 code
and as of now we are not sure how other architecture will implement the 
Interrupt remapping
(via IOMMU or any other means).  

Let me know if you are ok with moving the code to x86.

Regards,
Rahul
> 
> Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 19.04.2021 13:54, Julien Grall wrote:
> For the time being, I think move this code in x86 is a lot better than 
> #ifdef or keep the code in common code.

Well, I would perhaps agree if it ended up being #ifdef CONFIG_X86.
I would perhaps not agree if there was a new CONFIG_* which other
(future) arch-es could select if desired.

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Julien Grall]

On 19/04/2021 12:16, Jan Beulich wrote:

On 19.04.2021 10:40, Roger Pau Monné wrote:

On Mon, Apr 19, 2021 at 07:16:52AM +, Rahul Singh wrote:

Thanks you everyone for reviewing the code. I will summarise what I have 
understood from all the comments
and what I will be doing for the next version of the patch.  Please let me know 
your view on this.

1. Create a separate non-arch specific file "msi-intercept.c"  for the below 
newly introduced function and
 compile that file if CONFIG_PCI_MSI_INTERCEPT is 
enabled.CONFIG_PCI_MSI_INTERCEPT  will  be
 enabled for x86 by default.


Everything up to here wants to be separate from ...


Also Mention in the commit message that these function will be needed for Xen to
 support MSI interrupt within XEN.

pdev_msi_initi(..)
pdev_msi_deiniti(..)


... this (if all of these functions really are needed beyond the
purpose of intercepting MSI accesses).


I would drop the last 'i' from both function names above, as we use
init/deinit in the rest of the code base.


+1


pdev_dump_msi(..),
pdev_msix_assign(..)

2. Create separate patch for iommu_update_ire_from_msi() related code. There 
are two suggestion please help me which one to choose.
  
  - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and also move the hook from iommu_ops under CONFIG_X86.


I would go for this one.


Strictly speaking this isn't x86-specific and hence shouldn't move there.
It merely depends on whether full MSI support is wanted by an arch. 


As I pointed out before, Arm doesn't use the IOMMU to setup the MSIs. So 
the naming and using an IOMMU callback is definitely wrong for Arm.



I'd
therefore guard the declaration by an #ifdef (if needed at all - have a
declaration without implementation isn't really that problematic). For
the definition question is going to be whether you introduce another new
file for the pdev_*() functions above. If not, #ifdef may again be better
than moving to an x86-specific file.
AFAIK, this helper is only called by x86 specific code and it will not 
be used as-is by Arm.


I can't tell for other arch (e.g RISCv, PowerPC). However... we can take 
the decision to move the code back to common back when it is necessary.


For the time being, I think move this code in x86 is a lot better than 
#ifdef or keep the code in common code.


Cheers,

--
Julien Grall

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 19.04.2021 10:40, Roger Pau Monné wrote:
> On Mon, Apr 19, 2021 at 07:16:52AM +, Rahul Singh wrote:
>> Thanks you everyone for reviewing the code. I will summarise what I have 
>> understood from all the comments 
>> and what I will be doing for the next version of the patch.  Please let me 
>> know your view on this.
>>
>> 1. Create a separate non-arch specific file "msi-intercept.c"  for the below 
>> newly introduced function and 
>> compile that file if CONFIG_PCI_MSI_INTERCEPT is 
>> enabled.CONFIG_PCI_MSI_INTERCEPT  will  be 
>> enabled for x86 by default.

Everything up to here wants to be separate from ...

>> Also Mention in the commit message that these function will be needed for 
>> Xen to 
>> support MSI interrupt within XEN.
>>
>>  pdev_msi_initi(..)
>>  pdev_msi_deiniti(..)

... this (if all of these functions really are needed beyond the
purpose of intercepting MSI accesses).

> I would drop the last 'i' from both function names above, as we use
> init/deinit in the rest of the code base.

+1

>>  pdev_dump_msi(..),
>>  pdev_msix_assign(..)
>>
>> 2. Create separate patch for iommu_update_ire_from_msi() related code. There 
>> are two suggestion please help me which one to choose. 
>>  
>>  - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and 
>> also move the hook from iommu_ops under CONFIG_X86.
> 
> I would go for this one.

Strictly speaking this isn't x86-specific and hence shouldn't move there.
It merely depends on whether full MSI support is wanted by an arch. I'd
therefore guard the declaration by an #ifdef (if needed at all - have a
declaration without implementation isn't really that problematic). For
the definition question is going to be whether you introduce another new
file for the pdev_*() functions above. If not, #ifdef may again be better
than moving to an x86-specific file.

>>   - Implement a more generic function "arch_register_msi()"). This could 
>> call iommu_update_ire_from_msi() on x86 and the 
>> ITS related code once implemented on Arm. Introduce the new Kconfig 
>> CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option.
> 
> I think it's best to introduce this hook when you actually have to
> implement the Arm version of it.

Plus arch_register_msi() sounds like a one-time operation, whereas (iirc)
iommu_update_ire_from_msi() may get called many times during the lifetime
of a single MSI interrupt.

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné]

On Mon, Apr 19, 2021 at 07:16:52AM +, Rahul Singh wrote:
> Thanks you everyone for reviewing the code. I will summarise what I have 
> understood from all the comments 
> and what I will be doing for the next version of the patch.  Please let me 
> know your view on this.
> 
> 1. Create a separate non-arch specific file "msi-intercept.c"  for the below 
> newly introduced function and 
> compile that file if CONFIG_PCI_MSI_INTERCEPT is 
> enabled.CONFIG_PCI_MSI_INTERCEPT  will  be 
> enabled for x86 by default. Also Mention in the commit message that these 
> function will be needed for Xen to 
> support MSI interrupt within XEN.
> 
>   pdev_msi_initi(..)
>   pdev_msi_deiniti(..)

I would drop the last 'i' from both function names above, as we use
init/deinit in the rest of the code base.

>   pdev_dump_msi(..),
>   pdev_msix_assign(..)
> 
> 2. Create separate patch for iommu_update_ire_from_msi() related code. There 
> are two suggestion please help me which one to choose. 
>  
>  - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and 
> also move the hook from iommu_ops under CONFIG_X86.

I would go for this one.

> 
>   - Implement a more generic function "arch_register_msi()"). This could 
> call iommu_update_ire_from_msi() on x86 and the 
> ITS related code once implemented on Arm. Introduce the new Kconfig 
> CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option.

I think it's best to introduce this hook when you actually have to
implement the Arm version of it.

Thanks, Roger.

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi All,

> On 15 Apr 2021, at 2:31 pm, Julien Grall  wrote:
> 
> Hi Roger,
> 
> On 15/04/2021 14:26, Roger Pau Monné wrote:
>> On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
>>> Hi Roger,
>>> 
>>> On 14/04/2021 09:05, Roger Pau Monné wrote:
 On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 12/04/2021 11:49, Roger Pau Monné wrote:
>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>> diff --git a/xen/drivers/passthrough/pci.c 
>>> b/xen/drivers/passthrough/pci.c
>>> index 705137f8be..1b473502a1 100644
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>>}
>>>__initcall(setup_dump_pcidevs);
>>> +
>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>int iommu_update_ire_from_msi(
>>>struct msi_desc *msi_desc, struct msi_msg *msg)
>>>{
>>>return iommu_intremap
>>>   ? iommu_call(_ops, update_ire_from_msi, msi_desc, 
>>> msg) : 0;
>>>}
>>> +#endif
>> 
>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>> remapping table will also be used for interrupt entries for devices
>> being used by Xen directly, where no intercept is required.
> 
> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
> MSI controller (such as the ITS).
> 
>> 
>> And then you also want to gate the hook from iommu_ops itself with
>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
> 
> 
> All the callers are in the x86 code. So how about moving the function in 
> an
> x86 specific file?
 
 Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
 update_ire_from_msi wasn't moved when the rest of the x86 specific
 functions where moved there.
>>> 
>>> I am guessing it is because the function was protected by CONFIG_HAS_PCI
>>> rather than CONFIG_X86. So it was deferred until another arch enables
>>> CONFIG_HAS_PCI (it is easier to know what code should be moved).
>>> 
 Was there an aim to use that in other
 arches?
>>> 
>>> In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
>>> MSI interrupt).
>> Then I think some of the bits that are moved from
>> xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
>> dump_pci_msi) need to be protected by a Kconfig option different than
>> CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
>> but to MSI handling in general (ie: Xen devices using MSI need
>> those).
> 
> Well... x86-specific devices yes. We don't know in what form MSI will be 
> added on for other arch-specific devices.
> 
> The same with the msi_list and msix fields of pci_dev, those
>> are not only used for MSI interception.
>> Or at least might be worth mentioning that the functions will be
>> needed for Xen to be able to use MSI interrupts itself, even if not
>> for interception purposes.
> 
> My preference would be to comment in the commit message (although, there is 
> no promise they will ever get re-used). We can then modify the #ifdef once we 
> introduce any use.
> 

Thanks you everyone for reviewing the code. I will summarise what I have 
understood from all the comments 
and what I will be doing for the next version of the patch.  Please let me know 
your view on this.

1. Create a separate non-arch specific file "msi-intercept.c"  for the below 
newly introduced function and 
compile that file if CONFIG_PCI_MSI_INTERCEPT is 
enabled.CONFIG_PCI_MSI_INTERCEPT  will  be 
enabled for x86 by default. Also Mention in the commit message that these 
function will be needed for Xen to 
support MSI interrupt within XEN.

pdev_msi_initi(..)
pdev_msi_deiniti(..)
pdev_dump_msi(..),
pdev_msix_assign(..)

2. Create separate patch for iommu_update_ire_from_msi() related code. There 
are two suggestion please help me which one to choose. 
 
 - Move the iommu_update_ire_from_msi() function to asm-x86/iommu.h and 
also move the hook from iommu_ops under CONFIG_X86.

  - Implement a more generic function "arch_register_msi()"). This could 
call iommu_update_ire_from_msi() on x86 and the 
ITS related code once implemented on Arm. Introduce the new Kconfig 
CONFIG_HAS_IOMMU_INTERRUPT_REMAP for this option.

3. As per the discussion I will gate the struct vpci_msi {..} and struct 
vpci_msix{..} using CONFIG_PCI_MSI_INTERCEPT so that 
they will not will be accessible on ARM.

Regards,
Rahul
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Julien Grall]

Hi Roger,

On 15/04/2021 14:26, Roger Pau Monné wrote:

On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:

Hi Roger,

On 14/04/2021 09:05, Roger Pau Monné wrote:

On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:

Hi Roger,

On 12/04/2021 11:49, Roger Pau Monné wrote:

On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..1b473502a1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
}
__initcall(setup_dump_pcidevs);
+
+#ifdef CONFIG_PCI_MSI_INTERCEPT
int iommu_update_ire_from_msi(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
return iommu_intremap
   ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
}
+#endif


This is not exactly related to MSI intercepts, the IOMMU interrupt
remapping table will also be used for interrupt entries for devices
being used by Xen directly, where no intercept is required.


On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
MSI controller (such as the ITS).



And then you also want to gate the hook from iommu_ops itself with
CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.



All the callers are in the x86 code. So how about moving the function in an
x86 specific file?


Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
update_ire_from_msi wasn't moved when the rest of the x86 specific
functions where moved there.


I am guessing it is because the function was protected by CONFIG_HAS_PCI
rather than CONFIG_X86. So it was deferred until another arch enables
CONFIG_HAS_PCI (it is easier to know what code should be moved).


Was there an aim to use that in other
arches?


In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
MSI interrupt).


Then I think some of the bits that are moved from
xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
dump_pci_msi) need to be protected by a Kconfig option different than
CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
but to MSI handling in general (ie: Xen devices using MSI need
those).


Well... x86-specific devices yes. We don't know in what form MSI will be 
added on for other arch-specific devices.


 The same with the msi_list and msix fields of pci_dev, those

are not only used for MSI interception.

Or at least might be worth mentioning that the functions will be
needed for Xen to be able to use MSI interrupts itself, even if not
for interception purposes.


My preference would be to comment in the commit message (although, there 
is no promise they will ever get re-used). We can then modify the #ifdef 
once we introduce any use.


Cheers,

--
Julien Grall

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné]

On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 14/04/2021 09:05, Roger Pau Monné wrote:
> > On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 12/04/2021 11:49, Roger Pau Monné wrote:
> > > > On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> > > > > diff --git a/xen/drivers/passthrough/pci.c 
> > > > > b/xen/drivers/passthrough/pci.c
> > > > > index 705137f8be..1b473502a1 100644
> > > > > --- a/xen/drivers/passthrough/pci.c
> > > > > +++ b/xen/drivers/passthrough/pci.c
> > > > > @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
> > > > >}
> > > > >__initcall(setup_dump_pcidevs);
> > > > > +
> > > > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > > > >int iommu_update_ire_from_msi(
> > > > >struct msi_desc *msi_desc, struct msi_msg *msg)
> > > > >{
> > > > >return iommu_intremap
> > > > >   ? iommu_call(_ops, update_ire_from_msi, msi_desc, 
> > > > > msg) : 0;
> > > > >}
> > > > > +#endif
> > > > 
> > > > This is not exactly related to MSI intercepts, the IOMMU interrupt
> > > > remapping table will also be used for interrupt entries for devices
> > > > being used by Xen directly, where no intercept is required.
> > > 
> > > On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
> > > MSI controller (such as the ITS).
> > > 
> > > > 
> > > > And then you also want to gate the hook from iommu_ops itself with
> > > > CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
> > > 
> > > 
> > > All the callers are in the x86 code. So how about moving the function in 
> > > an
> > > x86 specific file?
> > 
> > Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
> > update_ire_from_msi wasn't moved when the rest of the x86 specific
> > functions where moved there.
> 
> I am guessing it is because the function was protected by CONFIG_HAS_PCI
> rather than CONFIG_X86. So it was deferred until another arch enables
> CONFIG_HAS_PCI (it is easier to know what code should be moved).
> 
> > Was there an aim to use that in other
> > arches?
> 
> In the future we may need to use MSIs in Xen (IIRC some SMMUs only support
> MSI interrupt).

Then I think some of the bits that are moved from
xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
dump_pci_msi) need to be protected by a Kconfig option different than
CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
but to MSI handling in general (ie: Xen devices using MSI need
those). The same with the msi_list and msix fields of pci_dev, those
are not only used for MSI interception.

Or at least might be worth mentioning that the functions will be
needed for Xen to be able to use MSI interrupts itself, even if not
for interception purposes.

Thanks, Roger.

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Julien Grall]

Hi Roger,

On 14/04/2021 09:05, Roger Pau Monné wrote:

On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:

Hi Roger,

On 12/04/2021 11:49, Roger Pau Monné wrote:

On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..1b473502a1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
   }
   __initcall(setup_dump_pcidevs);
+
+#ifdef CONFIG_PCI_MSI_INTERCEPT
   int iommu_update_ire_from_msi(
   struct msi_desc *msi_desc, struct msi_msg *msg)
   {
   return iommu_intremap
  ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
   }
+#endif


This is not exactly related to MSI intercepts, the IOMMU interrupt
remapping table will also be used for interrupt entries for devices
being used by Xen directly, where no intercept is required.


On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
MSI controller (such as the ITS).



And then you also want to gate the hook from iommu_ops itself with
CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.



All the callers are in the x86 code. So how about moving the function in an
x86 specific file?


Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
update_ire_from_msi wasn't moved when the rest of the x86 specific
functions where moved there.


I am guessing it is because the function was protected by CONFIG_HAS_PCI 
rather than CONFIG_X86. So it was deferred until another arch enables 
CONFIG_HAS_PCI (it is easier to know what code should be moved).



Was there an aim to use that in other
arches?


In the future we may need to use MSIs in Xen (IIRC some SMMUs only 
support MSI interrupt). But I think the naming would be misleading as 
the IOMMU will not be used for the remapping.


So most likely, we would want a more generic name (maybe 
"arch_register_msi()"). This could call iommu_update_ire_from_msi() on 
x86 and the ITS on Arm.


I don't know how RISCv and PowerPC remap the interrupt. But if they are 
using the IOMMU then we could provide a generic helper protected by 
CONFIG_HAS_IOMMU_INTERRUPT_REMAP (or a similar name).




The hook in iommu_ops also need to be moved inside the x86 region.
Please do this iommu change in a separate patch.


+1

Cheers,

--
Julien Grall

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 14.04.2021 10:28, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 09:08:53AM +0200, Jan Beulich wrote:
>> On 13.04.2021 19:12, Julien Grall wrote:
>>> On 12/04/2021 11:49, Roger Pau Monné wrote:
 On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -91,6 +91,7 @@ struct vpci {
>   /* FIXME: currently there's no support for SR-IOV. */
>   } header;
>   
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>   /* MSI data. */
>   struct vpci_msi {
> /* Address. */
> @@ -136,6 +137,7 @@ struct vpci {
>   struct vpci_arch_msix_entry arch;
>   } entries[];
>   } *msix;
> +#endif /* CONFIG_PCI_MSI_INTERCEPT */

 Note that here you just remove two pointers from the struct, not that
 I'm opposed to it, but it's not that much space that's saved anyway.
 Ie: it might also be fine to just leave them as NULL unconditionally
 on Arm.
>>>
>>> Can the two pointers be NULL on x86? If not, then I would prefer if they 
>>> disappear on Arm so there is less chance to make any mistake (such as 
>>> unconditionally accessing the pointer in common code).
>>
>> Alternative proposal: How about making it effectively impossible to
>> de-reference the pointer on Arm by leaving the field there, but having
>> the struct definition available on non-Arm only?
> 
> We could place the struct definitions somewhere else protected by
> CONFIG_PCI_MSI_INTERCEPT, but I'm not sure that would be much
> different than the current proposal, and overall I think I prefer this
> approach then, as we keep the definition and the usage closer
> together.
> 
> Maybe we could slightly modify the current layout so that
> the field is always present, but the struct definition is made
> conditional to CONFIG_PCI_MSI_INTERCEPT?

You mean like this

/* MSI data. */
struct vpci_msi {
#ifdef CONFIG_PCI_MSI_INTERCEPT
/* Address. */
...
struct vpci_arch_msix_entry arch;
} entries[];
#endif /* CONFIG_PCI_MSI_INTERCEPT */
} *msix;

? I could live with it, but this would have the compiler not
refuse e.g. sizeof(struct vpci_msi) or instantiation of the
struct as a (local) variable, unlike my proposal.

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Julien Grall]

Hi Jan,

On 14/04/2021 08:08, Jan Beulich wrote:

On 13.04.2021 19:12, Julien Grall wrote:

On 12/04/2021 11:49, Roger Pau Monné wrote:

On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:

--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -91,6 +91,7 @@ struct vpci {
   /* FIXME: currently there's no support for SR-IOV. */
   } header;
   
+#ifdef CONFIG_PCI_MSI_INTERCEPT

   /* MSI data. */
   struct vpci_msi {
 /* Address. */
@@ -136,6 +137,7 @@ struct vpci {
   struct vpci_arch_msix_entry arch;
   } entries[];
   } *msix;
+#endif /* CONFIG_PCI_MSI_INTERCEPT */


Note that here you just remove two pointers from the struct, not that
I'm opposed to it, but it's not that much space that's saved anyway.
Ie: it might also be fine to just leave them as NULL unconditionally
on Arm.


Can the two pointers be NULL on x86? If not, then I would prefer if they
disappear on Arm so there is less chance to make any mistake (such as
unconditionally accessing the pointer in common code).


Alternative proposal: How about making it effectively impossible to
de-reference the pointer on Arm by leaving the field there, but having
the struct definition available on non-Arm only?


Meh, it making more difficult for the reader because it has to go 
through one more hop to find out why the structure is not defined.


IOW, I still prefer Rahul's version.

Cheers,

--
Julien Grall

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné]

On Wed, Apr 14, 2021 at 09:08:53AM +0200, Jan Beulich wrote:
> On 13.04.2021 19:12, Julien Grall wrote:
> > On 12/04/2021 11:49, Roger Pau Monné wrote:
> >> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> >>> --- a/xen/include/xen/vpci.h
> >>> +++ b/xen/include/xen/vpci.h
> >>> @@ -91,6 +91,7 @@ struct vpci {
> >>>   /* FIXME: currently there's no support for SR-IOV. */
> >>>   } header;
> >>>   
> >>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> >>>   /* MSI data. */
> >>>   struct vpci_msi {
> >>> /* Address. */
> >>> @@ -136,6 +137,7 @@ struct vpci {
> >>>   struct vpci_arch_msix_entry arch;
> >>>   } entries[];
> >>>   } *msix;
> >>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
> >>
> >> Note that here you just remove two pointers from the struct, not that
> >> I'm opposed to it, but it's not that much space that's saved anyway.
> >> Ie: it might also be fine to just leave them as NULL unconditionally
> >> on Arm.
> > 
> > Can the two pointers be NULL on x86? If not, then I would prefer if they 
> > disappear on Arm so there is less chance to make any mistake (such as 
> > unconditionally accessing the pointer in common code).
> 
> Alternative proposal: How about making it effectively impossible to
> de-reference the pointer on Arm by leaving the field there, but having
> the struct definition available on non-Arm only?

We could place the struct definitions somewhere else protected by
CONFIG_PCI_MSI_INTERCEPT, but I'm not sure that would be much
different than the current proposal, and overall I think I prefer this
approach then, as we keep the definition and the usage closer
together.

Maybe we could slightly modify the current layout so that
the field is always present, but the struct definition is made
conditional to CONFIG_PCI_MSI_INTERCEPT?

Thanks, Roger.

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné]

On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 12/04/2021 11:49, Roger Pau Monné wrote:
> > On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> > > index 705137f8be..1b473502a1 100644
> > > --- a/xen/drivers/passthrough/pci.c
> > > +++ b/xen/drivers/passthrough/pci.c
> > > @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
> > >   }
> > >   __initcall(setup_dump_pcidevs);
> > > +
> > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > >   int iommu_update_ire_from_msi(
> > >   struct msi_desc *msi_desc, struct msi_msg *msg)
> > >   {
> > >   return iommu_intremap
> > >  ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) 
> > > : 0;
> > >   }
> > > +#endif
> > 
> > This is not exactly related to MSI intercepts, the IOMMU interrupt
> > remapping table will also be used for interrupt entries for devices
> > being used by Xen directly, where no intercept is required.
> 
> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
> MSI controller (such as the ITS).
> 
> > 
> > And then you also want to gate the hook from iommu_ops itself with
> > CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
> 
> 
> All the callers are in the x86 code. So how about moving the function in an
> x86 specific file?

Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
update_ire_from_msi wasn't moved when the rest of the x86 specific
functions where moved there. Was there an aim to use that in other
arches?

The hook in iommu_ops also need to be moved inside the x86 region.
Please do this iommu change in a separate patch.

> > > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> > > index 9f5b5d52e1..a6b12717b1 100644
> > > --- a/xen/include/xen/vpci.h
> > > +++ b/xen/include/xen/vpci.h
> > > @@ -91,6 +91,7 @@ struct vpci {
> > >   /* FIXME: currently there's no support for SR-IOV. */
> > >   } header;
> > > +#ifdef CONFIG_PCI_MSI_INTERCEPT
> > >   /* MSI data. */
> > >   struct vpci_msi {
> > > /* Address. */
> > > @@ -136,6 +137,7 @@ struct vpci {
> > >   struct vpci_arch_msix_entry arch;
> > >   } entries[];
> > >   } *msix;
> > > +#endif /* CONFIG_PCI_MSI_INTERCEPT */
> > 
> > Note that here you just remove two pointers from the struct, not that
> > I'm opposed to it, but it's not that much space that's saved anyway.
> > Ie: it might also be fine to just leave them as NULL unconditionally
> > on Arm.
> 
> Can the two pointers be NULL on x86?

Yes, they can be NULL on x86.

> If not, then I would prefer if they
> disappear on Arm so there is less chance to make any mistake (such as
> unconditionally accessing the pointer in common code).

Any access to them needs to be protected anyway, or else we would be
in trouble. I don't think Xen ever accesses them based on the PCI
capabilities of the device.

Thanks, Roger.

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 13.04.2021 19:12, Julien Grall wrote:
> On 12/04/2021 11:49, Roger Pau Monné wrote:
>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>> --- a/xen/include/xen/vpci.h
>>> +++ b/xen/include/xen/vpci.h
>>> @@ -91,6 +91,7 @@ struct vpci {
>>>   /* FIXME: currently there's no support for SR-IOV. */
>>>   } header;
>>>   
>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>   /* MSI data. */
>>>   struct vpci_msi {
>>> /* Address. */
>>> @@ -136,6 +137,7 @@ struct vpci {
>>>   struct vpci_arch_msix_entry arch;
>>>   } entries[];
>>>   } *msix;
>>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
>>
>> Note that here you just remove two pointers from the struct, not that
>> I'm opposed to it, but it's not that much space that's saved anyway.
>> Ie: it might also be fine to just leave them as NULL unconditionally
>> on Arm.
> 
> Can the two pointers be NULL on x86? If not, then I would prefer if they 
> disappear on Arm so there is less chance to make any mistake (such as 
> unconditionally accessing the pointer in common code).

Alternative proposal: How about making it effectively impossible to
de-reference the pointer on Arm by leaving the field there, but having
the struct definition available on non-Arm only?

Jan

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Julien Grall]

Hi Roger,

On 12/04/2021 11:49, Roger Pau Monné wrote:

On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:

MSI related code in the "passthrough/pci.c” file is not useful for ARM
when MSI interrupts are injected via GICv3 ITS.

Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
MSI code for ARM in common code. Also move the arch-specific code to an
arch-specific directory and implement the stub version for the unused
code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.

Signed-off-by: Rahul Singh 
---
Changes since v1:
  - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
  - Implement stub version of the unused function for ARM.
  - Move unneeded function to x86 file.
---
  xen/arch/x86/msi.c| 64 +++
  xen/drivers/passthrough/pci.c | 51 +++-
  xen/drivers/pci/Kconfig   |  4 +++
  xen/drivers/vpci/Makefile |  3 +-
  xen/drivers/vpci/header.c | 19 +++
  xen/drivers/vpci/msix.c   | 25 ++
  xen/drivers/vpci/vpci.c   |  3 +-
  xen/include/asm-arm/msi.h | 44 
  xen/include/asm-x86/msi.h |  4 +++
  xen/include/xen/pci.h | 11 +++---
  xen/include/xen/vpci.h| 24 -
  xen/xsm/flask/hooks.c |  8 ++---
  12 files changed, 195 insertions(+), 65 deletions(-)
  create mode 100644 xen/include/asm-arm/msi.h

diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 5febc0ea4b..a6356f4a63 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
  return;
  }
  
+int alloc_pci_msi(struct pci_dev *pdev)


I would rather name it pci_msi_init...


+{
+unsigned int pos;
+
+INIT_LIST_HEAD(>msi_list);
+
+pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+  PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
+if ( pos )
+{
+uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
+
+pdev->msi_maxvec = multi_msi_capable(ctrl);
+}
+
+pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+  PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
+if ( pos )
+{
+struct arch_msix *msix = xzalloc(struct arch_msix);
+uint16_t ctrl;
+
+if ( !msix )
+return -ENOMEM;
+
+spin_lock_init(>table_lock);
+
+ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
+msix->nr_entries = msix_table_size(ctrl);
+
+pdev->msix = msix;
+}
+
+return 0;
+}
+
+void free_pci_msi(struct pci_dev *pdev)


...and pci_msi_free.


+{
+xfree(pdev->msix);
+}
+
+int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)


This kind of a confusing name - what about pci_msix_assign?


+{
+int rc;
+
+if ( pdev->msix )
+{
+rc = pci_reset_msix_state(pdev);
+if ( rc )
+return rc;
+msixtbl_init(d);
+}
+
+return 0;
+}
+
+void dump_pci_msi(struct pci_dev *pdev)
+{
+struct msi_desc *msi;
+
+list_for_each_entry ( msi, >msi_list, list )
+printk("%d ", msi->irq);
+}


I wonder, those those function really want to be in a x86 specific
file? There's nothing x86 specific about them AFAICT.

Would it make sense to create a separate msi-intercept.c file with
those that gets included when CONFIG_PCI_MSI_INTERCEPT?


  static void dump_msi(unsigned char key)
  {
  unsigned int irq;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 705137f8be..1b473502a1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 
bus, u8 devfn)
  {
  struct pci_dev *pdev;
  unsigned int pos;
+int rc;
  
  list_for_each_entry ( pdev, >alldevs_list, alldevs_list )

  if ( pdev->bus == bus && pdev->devfn == devfn )
@@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
u8 bus, u8 devfn)
  *((u8*) >bus) = bus;
  *((u8*) >devfn) = devfn;
  pdev->domain = NULL;
-INIT_LIST_HEAD(>msi_list);
-
-pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-  PCI_CAP_ID_MSI);
-if ( pos )
-{
-uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
-
-pdev->msi_maxvec = multi_msi_capable(ctrl);
-}
  
-pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),

-  PCI_CAP_ID_MSIX);
-if ( pos )
+rc = alloc_pci_msi(pdev);
+if ( rc )
  {
-struct arch_msix *msix = xzalloc(struct arch_msix);
-uint16_t ctrl;
-
-if ( !msix )
-{
-xfree(pdev);
-return NULL;
-}
-spin_lock_init(>table_lock);
-
-ctrl = pci_conf_read16(pdev->sbdf, 

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi Jan,

> On 12 Apr 2021, at 12:28 pm, Jan Beulich  wrote:
> 
> On 12.04.2021 12:49, Roger Pau Monné wrote:
>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>> --- a/xen/arch/x86/msi.c
>>> +++ b/xen/arch/x86/msi.c
>>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>>> return;
>>> }
>>> 
>>> +int alloc_pci_msi(struct pci_dev *pdev)
>> 
>> I would rather name it pci_msi_init...
> 
> Or maybe pdev_msi_init(), as pci_msi_init() looks more like a one-
> time, global init function?
> 
Ok. I will use pdev_msi_init().

>>> +void free_pci_msi(struct pci_dev *pdev)
>> 
>> ...and pci_msi_free.
> 
> The counterpart of "init" really would be "deinit", IOW I'd like to
> ask for either alloc/free or init/deinit.

Ok. I will use pdev_msi_deinit().
> 
>>> +{
>>> +xfree(pdev->msix);
> 
> Could this maybe become XFREE() at this occasion?

Ok.
> 
>>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
>> 
>> This kind of a confusing name - what about pci_msix_assign?
>> 
>>> +{
>>> +int rc;
>>> +
>>> +if ( pdev->msix )
> 
> Wouldn't this check better live in the sole caller?

Ok.
> 
> 
>>> +void dump_pci_msi(struct pci_dev *pdev)
> 
> pdev_dump_msi() or some such?
> 
> Also - const here and ...

Ok.
> 
>>> +{
>>> +struct msi_desc *msi;
> 
> ... here please, while you already move this code.
Ok.
> 
>>> +list_for_each_entry ( msi, >msi_list, list )
>>> +printk("%d ", msi->irq);
>>> +}
>> 
>> I wonder, those those function really want to be in a x86 specific
>> file? There's nothing x86 specific about them AFAICT.
>> 
>> Would it make sense to create a separate msi-intercept.c file with
>> those that gets included when CONFIG_PCI_MSI_INTERCEPT?
> 
> +1
> 
>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>> }
>>> __initcall(setup_dump_pcidevs);
>>> 
>>> +
>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> 
> No double blank lines please.

Ok.
> 
>>> int iommu_update_ire_from_msi(
>>> struct msi_desc *msi_desc, struct msi_msg *msg)
>>> {
>>> return iommu_intremap
>>>? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>> }
>>> +#endif
>> 
>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>> remapping table will also be used for interrupt entries for devices
>> being used by Xen directly, where no intercept is required.
>> 
>> And then you also want to gate the hook from iommu_ops itself with
>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
> 
> I think the two aspects of MSI should be kept separate.

Ok. I will split the patch in two patches.
> 
>>> --- a/xen/drivers/pci/Kconfig
>>> +++ b/xen/drivers/pci/Kconfig
>>> @@ -1,3 +1,7 @@
>>> 
>>> config HAS_PCI
>>> bool
>>> +
>>> +config PCI_MSI_INTERCEPT
>>> +   def_bool y
>>> +   depends on X86 && HAS_PCI
> 
> Depending on HAS_PCI is fine (albeit not strictly needed afaict),
> but this shouldn't have a default (like HAS_PCI doesn't) and
> instead be selected by x86.

Ok.
> 
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>>> xfree(r);
>>> }
>>> spin_unlock(>vpci->lock);
>>> -xfree(pdev->vpci->msix);
>>> -xfree(pdev->vpci->msi);
>>> +free_vpci_msi(pdev);
> 
> I don't think the function needs to be passed a pdev, and ...

Ok.
>>> xfree(pdev->vpci);
>>> pdev->vpci = NULL;
> 
> ... it would only be consistent with this if pdev->vpci was passed
> instead.
OK.
> 
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/msi.h
>>> @@ -0,0 +1,44 @@
>>> +/*
>>> + * Copyright (C) 2021 Arm Ltd.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.  If not, see .
>>> + */
>>> +
>>> +#ifndef __ASM_MSI_H_
>>> +#define __ASM_MSI_H_
>>> +
>>> +static inline int alloc_pci_msi(struct pci_dev *pdev)
>>> +{
>>> +return 0;
>>> +}
>>> +
>>> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev 
>>> *pdev)
>>> +{
>>> +return 0;
>>> +}
>>> +
>>> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
>>> +static inline void free_pci_msi(struct pci_dev *pdev) {}
>>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>>> +
>>> +#endif /* __ASM_MSI_H */
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * tab-width: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>> 
>> Should you 

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Rahul Singh]

Hi Roger,

> On 12 Apr 2021, at 11:49 am, Roger Pau Monné  wrote:
> 
> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>> MSI related code in the "passthrough/pci.c” file is not useful for ARM
>> when MSI interrupts are injected via GICv3 ITS.
>> 
>> Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
>> MSI code for ARM in common code. Also move the arch-specific code to an
>> arch-specific directory and implement the stub version for the unused
>> code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
>> 
>> Signed-off-by: Rahul Singh 
>> ---
>> Changes since v1:
>> - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>> - Implement stub version of the unused function for ARM.
>> - Move unneeded function to x86 file. 
>> ---
>> xen/arch/x86/msi.c| 64 +++
>> xen/drivers/passthrough/pci.c | 51 +++-
>> xen/drivers/pci/Kconfig   |  4 +++
>> xen/drivers/vpci/Makefile |  3 +-
>> xen/drivers/vpci/header.c | 19 +++
>> xen/drivers/vpci/msix.c   | 25 ++
>> xen/drivers/vpci/vpci.c   |  3 +-
>> xen/include/asm-arm/msi.h | 44 
>> xen/include/asm-x86/msi.h |  4 +++
>> xen/include/xen/pci.h | 11 +++---
>> xen/include/xen/vpci.h| 24 -
>> xen/xsm/flask/hooks.c |  8 ++---
>> 12 files changed, 195 insertions(+), 65 deletions(-)
>> create mode 100644 xen/include/asm-arm/msi.h
>> 
>> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
>> index 5febc0ea4b..a6356f4a63 100644
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>> return;
>> }
>> 
>> +int alloc_pci_msi(struct pci_dev *pdev)
> 
> I would rather name it pci_msi_init...
> 
>> +{
>> +unsigned int pos;
>> +
>> +INIT_LIST_HEAD(>msi_list);
>> +
>> +pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> +  PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
>> +if ( pos )
>> +{
>> +uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> +
>> +pdev->msi_maxvec = multi_msi_capable(ctrl);
>> +}
>> +
>> +pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> +  PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
>> +if ( pos )
>> +{
>> +struct arch_msix *msix = xzalloc(struct arch_msix);
>> +uint16_t ctrl;
>> +
>> +if ( !msix )
>> +return -ENOMEM;
>> +
>> +spin_lock_init(>table_lock);
>> +
>> +ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> +msix->nr_entries = msix_table_size(ctrl);
>> +
>> +pdev->msix = msix;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +void free_pci_msi(struct pci_dev *pdev)
> 
> ...and pci_msi_free.

Ok. I will change the name in next version. 
> 
>> +{
>> +xfree(pdev->msix);
>> +}
>> +
>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
> 
> This kind of a confusing name - what about pci_msix_assign?

Ok.
> 
>> +{
>> +int rc;
>> +
>> +if ( pdev->msix )
>> +{
>> +rc = pci_reset_msix_state(pdev);
>> +if ( rc )
>> +return rc;
>> +msixtbl_init(d);
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +void dump_pci_msi(struct pci_dev *pdev)
>> +{
>> +struct msi_desc *msi;
>> +
>> +list_for_each_entry ( msi, >msi_list, list )
>> +printk("%d ", msi->irq);
>> +}
> 
> I wonder, those those function really want to be in a x86 specific
> file? There's nothing x86 specific about them AFAICT.
> 
> Would it make sense to create a separate msi-intercept.c file with
> those that gets included when CONFIG_PCI_MSI_INTERCEPT? 

Yes that will be fine to create the separate file msi-intercept.c.
> 
> 
>> static void dump_msi(unsigned char key)
>> {
>> unsigned int irq;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 705137f8be..1b473502a1 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
>> u8 bus, u8 devfn)
>> {
>> struct pci_dev *pdev;
>> unsigned int pos;
>> +int rc;
>> 
>> list_for_each_entry ( pdev, >alldevs_list, alldevs_list )
>> if ( pdev->bus == bus && pdev->devfn == devfn )
>> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg 
>> *pseg, u8 bus, u8 devfn)
>> *((u8*) >bus) = bus;
>> *((u8*) >devfn) = devfn;
>> pdev->domain = NULL;
>> -INIT_LIST_HEAD(>msi_list);
>> -
>> -pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), 
>> PCI_FUNC(devfn),
>> -  PCI_CAP_ID_MSI);
>> -if ( pos )
>> -{
>> -uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> -
>> -pdev->msi_maxvec = 

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich]

On 12.04.2021 12:49, Roger Pau Monné wrote:
> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>>  return;
>>  }
>>  
>> +int alloc_pci_msi(struct pci_dev *pdev)
> 
> I would rather name it pci_msi_init...

Or maybe pdev_msi_init(), as pci_msi_init() looks more like a one-
time, global init function?

>> +void free_pci_msi(struct pci_dev *pdev)
> 
> ...and pci_msi_free.

The counterpart of "init" really would be "deinit", IOW I'd like to
ask for either alloc/free or init/deinit.

>> +{
>> +xfree(pdev->msix);

Could this maybe become XFREE() at this occasion?

>> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)
> 
> This kind of a confusing name - what about pci_msix_assign?
> 
>> +{
>> +int rc;
>> +
>> +if ( pdev->msix )

Wouldn't this check better live in the sole caller?


>> +void dump_pci_msi(struct pci_dev *pdev)

pdev_dump_msi() or some such?

Also - const here and ...

>> +{
>> +struct msi_desc *msi;

... here please, while you already move this code.

>> +list_for_each_entry ( msi, >msi_list, list )
>> +printk("%d ", msi->irq);
>> +}
> 
> I wonder, those those function really want to be in a x86 specific
> file? There's nothing x86 specific about them AFAICT.
> 
> Would it make sense to create a separate msi-intercept.c file with
> those that gets included when CONFIG_PCI_MSI_INTERCEPT?

+1

>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>  }
>>  __initcall(setup_dump_pcidevs);
>>  
>> +
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT

No double blank lines please.

>>  int iommu_update_ire_from_msi(
>>  struct msi_desc *msi_desc, struct msi_msg *msg)
>>  {
>>  return iommu_intremap
>> ? iommu_call(_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>  }
>> +#endif
> 
> This is not exactly related to MSI intercepts, the IOMMU interrupt
> remapping table will also be used for interrupt entries for devices
> being used by Xen directly, where no intercept is required.
> 
> And then you also want to gate the hook from iommu_ops itself with
> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.

I think the two aspects of MSI should be kept separate.

>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,7 @@
>>  
>>  config HAS_PCI
>>  bool
>> +
>> +config PCI_MSI_INTERCEPT
>> +def_bool y
>> +depends on X86 && HAS_PCI

Depending on HAS_PCI is fine (albeit not strictly needed afaict),
but this shouldn't have a default (like HAS_PCI doesn't) and
instead be selected by x86.

>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -48,8 +48,7 @@ void vpci_remove_device(struct pci_dev *pdev)
>>  xfree(r);
>>  }
>>  spin_unlock(>vpci->lock);
>> -xfree(pdev->vpci->msix);
>> -xfree(pdev->vpci->msi);
>> +free_vpci_msi(pdev);

I don't think the function needs to be passed a pdev, and ...
>>  xfree(pdev->vpci);
>>  pdev->vpci = NULL;

... it would only be consistent with this if pdev->vpci was passed
instead.

>> --- /dev/null
>> +++ b/xen/include/asm-arm/msi.h
>> @@ -0,0 +1,44 @@
>> +/*
>> + * Copyright (C) 2021 Arm Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#ifndef __ASM_MSI_H_
>> +#define __ASM_MSI_H_
>> +
>> +static inline int alloc_pci_msi(struct pci_dev *pdev)
>> +{
>> +return 0;
>> +}
>> +
>> +static inline int pci_assign_msix_init(struct domain *d, struct pci_dev 
>> *pdev)
>> +{
>> +return 0;
>> +}
>> +
>> +static inline void dump_pci_msi(struct pci_dev *pdev) {}
>> +static inline void free_pci_msi(struct pci_dev *pdev) {}
>> +static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>> +
>> +#endif /* __ASM_MSI_H */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> 
> Should you instead have a non-arch specific file with those dummy
> helpers that get used when !CONFIG_PCI_MSI_INTERCEPT?

+1

>> --- a/xen/include/asm-x86/msi.h
>> +++ b/xen/include/asm-x86/msi.h
>> @@ -252,5 +252,9 @@ void unmask_msi_irq(struct irq_desc *);
>>  void guest_mask_msi_irq(struct irq_desc *, bool mask);
>>  void ack_nonmaskable_msi_irq(struct irq_desc *);
>>  void set_msi_affinity(struct irq_desc *, 

Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Roger Pau Monné]

On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> MSI related code in the "passthrough/pci.c” file is not useful for ARM
> when MSI interrupts are injected via GICv3 ITS.
> 
> Therefore introducing the new flag CONFIG_PCI_MSI_INTERCEPT to gate the
> MSI code for ARM in common code. Also move the arch-specific code to an
> arch-specific directory and implement the stub version for the unused
> code for ARM to avoid compilation error when HAS_PCI is enabled for ARM.
> 
> Signed-off-by: Rahul Singh 
> ---
> Changes since v1:
>  - Rename CONFIG_HAS_PCI_MSI to CONFIG_PCI_MSI_INTERCEPT
>  - Implement stub version of the unused function for ARM.
>  - Move unneeded function to x86 file. 
> ---
>  xen/arch/x86/msi.c| 64 +++
>  xen/drivers/passthrough/pci.c | 51 +++-
>  xen/drivers/pci/Kconfig   |  4 +++
>  xen/drivers/vpci/Makefile |  3 +-
>  xen/drivers/vpci/header.c | 19 +++
>  xen/drivers/vpci/msix.c   | 25 ++
>  xen/drivers/vpci/vpci.c   |  3 +-
>  xen/include/asm-arm/msi.h | 44 
>  xen/include/asm-x86/msi.h |  4 +++
>  xen/include/xen/pci.h | 11 +++---
>  xen/include/xen/vpci.h| 24 -
>  xen/xsm/flask/hooks.c |  8 ++---
>  12 files changed, 195 insertions(+), 65 deletions(-)
>  create mode 100644 xen/include/asm-arm/msi.h
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 5febc0ea4b..a6356f4a63 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1411,6 +1411,70 @@ void __init early_msi_init(void)
>  return;
>  }
>  
> +int alloc_pci_msi(struct pci_dev *pdev)

I would rather name it pci_msi_init...

> +{
> +unsigned int pos;
> +
> +INIT_LIST_HEAD(>msi_list);
> +
> +pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +  PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
> +if ( pos )
> +{
> +uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> +
> +pdev->msi_maxvec = multi_msi_capable(ctrl);
> +}
> +
> +pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +  PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
> +if ( pos )
> +{
> +struct arch_msix *msix = xzalloc(struct arch_msix);
> +uint16_t ctrl;
> +
> +if ( !msix )
> +return -ENOMEM;
> +
> +spin_lock_init(>table_lock);
> +
> +ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> +msix->nr_entries = msix_table_size(ctrl);
> +
> +pdev->msix = msix;
> +}
> +
> +return 0;
> +}
> +
> +void free_pci_msi(struct pci_dev *pdev)

...and pci_msi_free.

> +{
> +xfree(pdev->msix);
> +}
> +
> +int pci_assign_msix_init(struct domain *d, struct pci_dev *pdev)

This kind of a confusing name - what about pci_msix_assign?

> +{
> +int rc;
> +
> +if ( pdev->msix )
> +{
> +rc = pci_reset_msix_state(pdev);
> +if ( rc )
> +return rc;
> +msixtbl_init(d);
> +}
> +
> +return 0;
> +}
> +
> +void dump_pci_msi(struct pci_dev *pdev)
> +{
> +struct msi_desc *msi;
> +
> +list_for_each_entry ( msi, >msi_list, list )
> +printk("%d ", msi->irq);
> +}

I wonder, those those function really want to be in a x86 specific
file? There's nothing x86 specific about them AFAICT.

Would it make sense to create a separate msi-intercept.c file with
those that gets included when CONFIG_PCI_MSI_INTERCEPT?

>  static void dump_msi(unsigned char key)
>  {
>  unsigned int irq;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 705137f8be..1b473502a1 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
> u8 bus, u8 devfn)
>  {
>  struct pci_dev *pdev;
>  unsigned int pos;
> +int rc;
>  
>  list_for_each_entry ( pdev, >alldevs_list, alldevs_list )
>  if ( pdev->bus == bus && pdev->devfn == devfn )
> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
> u8 bus, u8 devfn)
>  *((u8*) >bus) = bus;
>  *((u8*) >devfn) = devfn;
>  pdev->domain = NULL;
> -INIT_LIST_HEAD(>msi_list);
> -
> -pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), 
> PCI_FUNC(devfn),
> -  PCI_CAP_ID_MSI);
> -if ( pos )
> -{
> -uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> -
> -pdev->msi_maxvec = multi_msi_capable(ctrl);
> -}
>  
> -pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), 
> PCI_FUNC(devfn),
> -  PCI_CAP_ID_MSIX);
> -if ( pos )
> +rc = alloc_pci_msi(pdev);
> +if ( rc )
>  {
> -struct arch_msix *msix = xzalloc(struct arch_msix);
> -uint16_t