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

["Jaggi, Manish" <Manish.Jaggi@xxxxxxxxxx>]

On 1/31/2017 8:47 PM, Julien Grall wrote:
>
>
> 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@xxxxxxxxxxxxx> on behalf of Andre
>>>>>>> Przywara <andre.przywara@xxxxxxx>
>>>>>>> Sent: Tuesday, January 31, 2017 12:01 AM
>>>>>>> To: Stefano Stabellini; Julien Grall
>>>>>>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 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@xxxxxxx). 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.
>
Julien, As you suggested we need to find another way, I assumed you had 
something in mind.
Since we both agree that sbdf!=deviceID, the current series of ITS patches will 
program the incorrect deviceID so there is a need to
have a way to map sbdf with deviceID in xen.

One option could be to add a new hypercall to supply sbdf and deviceID to xen.

 #define PHYSDEVOP_pci_dev_map_msi_specifier    33
 struct physdev_pci_dev_map_msi_specifier {
    /* IN */
    uint16_t seg;
    uint8_t bus;
    uint8_t devfn;
    uint32_t msi_specifier; //DeviceID
 };

(https://lists.xen.org/archives/html/xen-devel/2016-12/msg00224.html)

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


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