Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450483 > unrolled thread

Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when  requested Thomas Gleixner <tglx@linutronix.de> - 2016-07-26 11:10 +0200
    Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when  requested Auger Eric <eric.auger@redhat.com> - 2016-07-26 12:10 +0200

#1450483 — Re: [PATCH v11 10/10] genirq/msi: use the MSI doorbell's IOVA when requested

Eric,

On Mon, 25 Jul 2016, Auger Eric wrote:
> On 20/07/2016 11:09, Thomas Gleixner wrote:
> > On Tue, 19 Jul 2016, Eric Auger wrote:
> >> @@ -63,10 +63,18 @@ static int msi_compose(struct irq_data *irq_data,
> >>  {
> >>  	int ret = 0;
> >>  
> >> -	if (erase)
> >> +	if (erase) {
> >>  		memset(msg, 0, sizeof(*msg));
> >> -	else
> >> +	} else {
> >> +		struct device *dev;
> >> +
> >>  		ret = irq_chip_compose_msi_msg(irq_data, msg);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		dev = msi_desc_to_dev(irq_data_get_msi_desc(irq_data));
> >> +		WARN_ON(iommu_msi_msg_pa_to_va(dev, msg));
> > 
> > What the heck is this call doing? And why is there only a WARN_ON and not a
> > proper error return code handling?
> 
> iommu_msi_msg_pa_to_va is part of the new iommu-msi API introduced in PART I
> of this series. This helper function detects the physical address found in
> the MSI message has a corresponding allocated IOVA. This happens if the MSI
> doorbell is accessed through an IOMMU and this IOMMU do not bypass the MSI
> addresses (ARM case). Allocation of this IOVA was performed in the previous
> patch.
>
> So, if this is the case, the physical address is swapped with the IOVA
> address. That way the PCIe device will send the MSI with this IOVA and
> the address will be translated by the IOMMU into the target MSI doorbell PA.
> 
> Hope this clarifies

No, it does not. You are explaining in great length what that function is
doing, but you are not explaining WHY your don't do a proper return code
handling and just do a WARN_ON() and happily proceed. If that function fails
then the interrupt will not be functional, so WHY on earth are you continuing?

Thanks,

	tglx

[toc] | [next] | [standalone]

#1450519

Hi Thomas,

On 26/07/2016 11:04, Thomas Gleixner wrote:
> Eric,
> 
> On Mon, 25 Jul 2016, Auger Eric wrote:
>> On 20/07/2016 11:09, Thomas Gleixner wrote:
>>> On Tue, 19 Jul 2016, Eric Auger wrote:
>>>> @@ -63,10 +63,18 @@ static int msi_compose(struct irq_data *irq_data,
>>>>  {
>>>>  	int ret = 0;
>>>>  
>>>> -	if (erase)
>>>> +	if (erase) {
>>>>  		memset(msg, 0, sizeof(*msg));
>>>> -	else
>>>> +	} else {
>>>> +		struct device *dev;
>>>> +
>>>>  		ret = irq_chip_compose_msi_msg(irq_data, msg);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		dev = msi_desc_to_dev(irq_data_get_msi_desc(irq_data));
>>>> +		WARN_ON(iommu_msi_msg_pa_to_va(dev, msg));
>>>
>>> What the heck is this call doing? And why is there only a WARN_ON and not a
>>> proper error return code handling?
>>
>> iommu_msi_msg_pa_to_va is part of the new iommu-msi API introduced in PART I
>> of this series. This helper function detects the physical address found in
>> the MSI message has a corresponding allocated IOVA. This happens if the MSI
>> doorbell is accessed through an IOMMU and this IOMMU do not bypass the MSI
>> addresses (ARM case). Allocation of this IOVA was performed in the previous
>> patch.
>>
>> So, if this is the case, the physical address is swapped with the IOVA
>> address. That way the PCIe device will send the MSI with this IOVA and
>> the address will be translated by the IOMMU into the target MSI doorbell PA.
>>
>> Hope this clarifies
> 
> No, it does not. You are explaining in great length what that function is
> doing, but you are not explaining WHY your don't do a proper return code
> handling and just do a WARN_ON() and happily proceed. If that function fails
> then the interrupt will not be functional, so WHY on earth are you continuing?
Oh sorry I focused on the function's goal. Originally I could not return an
error since there is a BUG_ON(ret) afterwards. And typically the userspace can
willingly omit to pass IPA range that map MSIs. But now we have this 2 phases where
we first map the MSIs on pci_enable_msi_range and use the IOVA at compose time
I need to analyze again if the userspace can induce a BUG_ON.

Thanks

Eric
> 
> Thanks,
> 
> 	tglx
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web