Re: [PATCH 09/28] ARM: GICv3 ITS: map device and LPIs to the ITS on physdev_op hypercall

From: Julien Grall <julien.grall@arm.com>
To: "Jaggi, Manish" <Manish.Jaggi@cavium.com>,
	Andre Przywara <andre.przywara@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Nair, Jayachandran" <Jayachandran.Nair@cavium.com>,
	Vijay Kilari <vijay.kilari@gmail.com>,
	"Kapoor, Prasun" <Prasun.Kapoor@cavium.com>
Subject: Re: [PATCH 09/28] ARM: GICv3 ITS: map device and LPIs to the ITS on physdev_op hypercall
Date: Tue, 31 Jan 2017 15:17:54 +0000	[thread overview]
Message-ID: <74d4169b-468c-3260-b7c1-3f055294b904@arm.com> (raw)
In-Reply-To: <15cd2249-ed90-3be7-dac0-96caca762016@caviumnetworks.com>

On 31/01/17 14:08, Jaggi, Manish wrote:
> Hi Julien,
>
> On 1/31/2017 7:16 PM, Julien Grall wrote:
>> On 31/01/17 13:19, Jaggi, Manish wrote:
>>> On 1/31/2017 6:13 PM, Julien Grall wrote:
>>>> On 31/01/17 10:29, Jaggi, Manish wrote:
>>>>>>
>>>>>> From: Xen-devel <xen-devel-bounces@lists.xen.org> on behalf of Andre
>>>>>> Przywara <andre.przywara@arm.com>
>>>>>> Sent: Tuesday, January 31, 2017 12:01 AM
>>>>>> To: Stefano Stabellini; Julien Grall
>>>>>> Cc: xen-devel@lists.xenproject.org; Vijay Kilari
>>>>>> Subject: [Xen-devel] [PATCH 09/28] ARM: GICv3 ITS: map device and
>>>>>> LPIs to the ITS on physdev_op hypercall
>>>>>>
>>>>> [snip]
>>>>>>
>>>>>>
>>>>>>  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>  {
>>>>>> +    struct physdev_manage_pci manage;
>>>>>> +    u32 devid;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    switch (cmd)
>>>>>> +    {
>>>>>
>>>>> You might alos need to  PHYSDEVOP_pci_device_add hypercall also.
>>>>>
>>>>>> +        case PHYSDEVOP_manage_pci_add:
>>>>>> +        case PHYSDEVOP_manage_pci_remove:
>>>>>> +            if ( copy_from_guest(&manage, arg, 1) != 0 )
>>>>>> +                return -EFAULT;
>>>>>> +
>>>>>> +            devid = manage.bus << 8 | manage.devfn;
>>>>>> +            /* Allocate an ITS device table with space for 32 MSIs */
>>>>>> +            ret = gicv3_its_map_guest_device(hardware_domain,
>>>>>> devid, devid, 5,
>>>>>> +                                             cmd ==
>>>>>> PHYSDEVOP_manage_pci_add);
>>>>>
>>>>> Based on 4.9 kernel, is the deivce ID plain sBDF or it is
>>>>> returnedfrom of_msi_map_rid /  iort_msi_map_rid ?
>>>>> I believe there needs to be set this as requirement on the calle of
>>>>> hypercall.
>>>>
>>>> The requirement of the hypercall is already defined and cannot be
>>>> changed. So if it does not provide the correct information, then we
>>>> need to find another way to get the DeviceID.
>>>>
>>> Do you think sbdf and device ID are same ? If you recollect your
>>> comments last year sbdf != DeviceID.
>>> for this series it has to be passed correctly otherwise ITS would be programmed incorrectly.
>>> I suggest this series to include another way as well.
>>
>> Thank you sherlock, if you had read my e-mail entirely you would have noticed I never said sbdf == DeviceID and actually provided insight on the problem and suggest solutions.
>>
> If you please read 4 lines above I wrote sbdf != DeviceID.

I think there is a miscommunication problem here. By "my e-mail" I was 
referring to the e-mail on this thread 
(4a8e35dc-57e5-e493-9a9a-4a91bb8e1a2f@arm.com). On your e-mail you 
implied I was not aware of sbdf != DeviceID (see "Do you think sbdf and 
device ID are same").

>> I would recommend you to do the same in the future. It would help to get the code much faster in Xen.
>>
>>>
>>>> In case of ACPI, we should be able to get those informations from the
>>>> IORT as the segment number is defined in the firmware tables. But for
>>>> Device Tree, we would need DOM0 and Xen to agree on the segment number.
>>>>
>>> Is there any agreement hypercall used with this series ?
>>
>> From xen/include/public/physdev.h
>>
>> struct physdev_manage_pci {
>>     /* IN */
>>     uint8_t bus;
>>     uint8_t devfn;
>> };
>>
>> struct physdev_manage_pci_ext {
>>     /* IN */
>>     uint8_t bus;
>>     uint8_t devfn;
>>     unsigned is_extfn;
>>     unsigned is_virtfn;
>>     struct {
>>         uint8_t bus;
>>         uint8_t devfn;
>>     } physfn;
>> };
>>
>> Let me know how you could encode a DeviceID in those hypercalls.
>>
> If you please go back to your comment where you wrote "we need to find another way to get the DeviceID", I was referring that we should add that another way in this series so that correct DeviceID is programmed in ITS.

This is not the first time I am saying this, just saying "we should add 
that another way..." is not helpful. You should also provide some 
details on what you would do.

For now, you gave no feedbacks on my suggestions and I have no clue what 
you mean by "agreement hypercall".

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel