| Message ID | 20170130183153.28566-10-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, > >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. As sbdf and deviceID returned from msi_map calls might not be same. >+ >+ return ret; >+ } >+ > gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd); > return -ENOSYS; > } >-- >2.9.0 > -Manish
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. 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. However, I am not sure whether we are going to need those hypercalls when Xen will gain support of PCI. There are some discussion to let Xen scanning the PCI devices, and therefore the hypercalls will be used. Today, the hypercall is called by Linux on ARM, but it might not be the case in the future. If we decide to implement it today, it means that we will not be able to remove it from Linux from compatibility reasons. So I would be more in favor of having a per-platform list of devices to support for the time being. So we can get GICv3 ITS working with Device Tree until Xen gain support of PCI. Stefano, Andre, any opinions? Cheers,
Hi Julien, 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. > 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 ? > However, I am not sure whether we are going to need those hypercalls > when Xen will gain support of PCI. There are some discussion to let > Xen scanning the PCI devices, and therefore the hypercalls will be used. > > Today, the hypercall is called by Linux on ARM, but it might not be > the case in the future. If we decide to implement it today, it means > that we will not be able to remove it from Linux from compatibility > reasons. > > So I would be more in favor of having a per-platform list of devices > to support for the time being. So we can get GICv3 ITS working with > Device Tree until Xen gain support of PCI. Stefano, Andre, any opinions? > Cheers, >
Hi Julien, 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 is programmed with wrong deviceID. I suggest this series to include another way as well. > 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 ? > However, I am not sure whether we are going to need those hypercalls > when Xen will gain support of PCI. There are some discussion to let > Xen scanning the PCI devices, and therefore the hypercalls will be used. > > Today, the hypercall is called by Linux on ARM, but it might not be > the case in the future. If we decide to implement it today, it means > that we will not be able to remove it from Linux from compatibility > reasons. > > So I would be more in favor of having a per-platform list of devices > to support for the time being. So we can get GICv3 ITS working with > Device Tree until Xen gain support of PCI. Stefano, Andre, any opinions? > Cheers, >
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. 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. Cheers,
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 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. > Cheers, >
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,
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@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. > 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, >
On 31/01/17 16:02, Jaggi, Manish wrote: > > > 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: >>> 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. I gave suggestions on my e-mail but you may have missed it... > 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. ... as well as the part where I am saying that I am not in favor to implement an hypercall temporarily, and against adding a new hypercall for only a couple of weeks. As you may know PHYSDEV hypercall are part of the stable ABI and once they are added they cannot be removed. So we need to be sure the hypercall is necessary. In this case, the hypercall is not necessary as all the information can be found in the firmware tables. However this is not implemented yet and part of the discussion on PCI Passthrough (see [1]). We need a temporary solution that does not involve any commitment on the ABI until Xen is able to discover PCI. Regards, [1] <5cf9128e-e845-2a89-f7c7-ac8616941ab9@linaro.org> > > > >
On Mon, 30 Jan 2017, Andre Przywara wrote: > To get MSIs from devices forwarded to a CPU, we need to name the device > and its MSIs by mapping them to an ITS. > Since this involves queueing commands to the ITS command queue, we can't > really afford to do this during the guest's runtime, as this would open > up a denial-of-service attack vector. > So we require every device with MSI interrupts to be mapped explicitly by > Dom0. For Dom0 itself we can just use the existing PCI physdev_op > hypercalls, which the existing Linux kernel issues already. > So upon receipt of this hypercall we map the device to the hardware ITS > and prepare it to be later mapped by the virtual ITS by using the very > same device ID (for Dom0 only). > Also we ask for mapping 32 LPIs to cover 32 MSIs that the device may > use. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/physdev.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c > index 27bbbda..6e02de4 100644 > --- a/xen/arch/arm/physdev.c > +++ b/xen/arch/arm/physdev.c > @@ -9,11 +9,32 @@ > #include <xen/lib.h> > #include <xen/errno.h> > #include <xen/sched.h> > +#include <xen/guest_access.h> > +#include <asm/gic_v3_its.h> > #include <asm/hypercall.h> > > > int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > { > + struct physdev_manage_pci manage; > + u32 devid; > + int ret; > + > + switch (cmd) > + { > + case PHYSDEVOP_manage_pci_add: > + case PHYSDEVOP_manage_pci_remove: > + if ( copy_from_guest(&manage, arg, 1) != 0 ) > + return -EFAULT; You need to check that current is the hardware domain first. > + devid = manage.bus << 8 | manage.devfn; > + /* Allocate an ITS device table with space for 32 MSIs */ Please explain why 32 > + ret = gicv3_its_map_guest_device(hardware_domain, devid, devid, 5, > + cmd == PHYSDEVOP_manage_pci_add); > + > + return ret; > + } > + > gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd); > return -ENOSYS; > } > -- > 2.9.0 >
Hi Julien, On 01/31/2017 10:18 AM, Julien Grall wrote: > > > On 31/01/17 16:02, Jaggi, Manish wrote: >> >> >> 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: >>>> 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. > > I gave suggestions on my e-mail but you may have missed it... > >> 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. > > ... as well as the part where I am saying that I am not in favor to > implement an hypercall temporarily, and against adding a new hypercall > for only a couple of weeks. As you may know PHYSDEV hypercall are part > of the stable ABI and once they are added they cannot be removed. > > So we need to be sure the hypercall is necessary. In this case, the > hypercall is not necessary as all the information can be found in the > firmware tables. However this is not implemented yet and part of the > discussion on PCI Passthrough (see [1]). > > We need a temporary solution that does not involve any commitment on the > ABI until Xen is able to discover PCI. > Why can't we handle ITS device creation whenever a virtual ITS driver receives the MAPD command from dom0/domU. In case of dom0, it's straight forward dom0 always passes the real ITS device through MAPD command. This way we can support PCIe devices without hard-coded MSI(x) limit 32, and platform devices transparently. I used the below code to platform and PCIe device MSI(x) functionality on QDF2400 server platform. @@ -383,10 +384,17 @@ static int its_handle_mapd(struct virt_its *its, uint64_t *cmdptr) int size = its_cmd_get_size(cmdptr); bool valid = its_cmd_get_validbit(cmdptr); paddr_t itt_addr = its_cmd_mask_field(cmdptr, 2, 0, 52) & GENMASK(51, 8); + int ret; if ( !its->dev_table ) return -1; + size = size < 4 ? 4 : size; + ret = gicv3_its_map_guest_device(hardware_domain, devid, devid, size + 1, + valid); + if (ret < 0) + return ret; +
On 24/02/2017 19:57, Shanker Donthineni wrote: > Hi Julien, Hi Shanker, > > On 01/31/2017 10:18 AM, Julien Grall wrote: >> >> >> On 31/01/17 16:02, Jaggi, Manish wrote: >>> >>> >>> 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: >>>>> 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. >> >> I gave suggestions on my e-mail but you may have missed it... >> >>> 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. >> >> ... as well as the part where I am saying that I am not in favor to >> implement an hypercall temporarily, and against adding a new hypercall >> for only a couple of weeks. As you may know PHYSDEV hypercall are part >> of the stable ABI and once they are added they cannot be removed. >> >> So we need to be sure the hypercall is necessary. In this case, the >> hypercall is not necessary as all the information can be found in the >> firmware tables. However this is not implemented yet and part of the >> discussion on PCI Passthrough (see [1]). >> >> We need a temporary solution that does not involve any commitment on the >> ABI until Xen is able to discover PCI. >> > > Why can't we handle ITS device creation whenever a virtual ITS driver > receives the MAPD command from dom0/domU. In case of dom0, it's straight > forward dom0 always passes the real ITS device through MAPD command. I guess this can work. Note that, on a separate thread (see [1]), I suggested to decouple the virtual DeviceID to the physical one for DOM0 to simply the generation of the IORT. So we would have to be a bit more clever here. But that's probably a separate subject and can go in Xen in separate series. > This way we can support PCIe devices without hard-coded MSI(x) limit 32, > and platform devices transparently. I used the below code to platform > and PCIe device MSI(x) functionality on QDF2400 server platform. > > @@ -383,10 +384,17 @@ static int its_handle_mapd(struct virt_its *its, > uint64_t *cmdptr) > int size = its_cmd_get_size(cmdptr); > bool valid = its_cmd_get_validbit(cmdptr); > paddr_t itt_addr = its_cmd_mask_field(cmdptr, 2, 0, 52) & > GENMASK(51, 8); > + int ret; > > if ( !its->dev_table ) > return -1; > > + size = size < 4 ? 4 : size; > + ret = gicv3_its_map_guest_device(hardware_domain, devid, devid, > size + 1, > + valid); > + if (ret < 0) > + return ret; > + > Cheers, [1] https://lists.xen.org/archives/html/xen-devel/2017-02/msg02782.html
Hi, On 24/02/17 19:57, Shanker Donthineni wrote: > Hi Julien, > > > On 01/31/2017 10:18 AM, Julien Grall wrote: >> >> >> On 31/01/17 16:02, Jaggi, Manish wrote: >>> >>> >>> 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: >>>>> 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. >> >> I gave suggestions on my e-mail but you may have missed it... >> >>> 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. >> >> ... as well as the part where I am saying that I am not in favor to >> implement an hypercall temporarily, and against adding a new hypercall >> for only a couple of weeks. As you may know PHYSDEV hypercall are part >> of the stable ABI and once they are added they cannot be removed. >> >> So we need to be sure the hypercall is necessary. In this case, the >> hypercall is not necessary as all the information can be found in the >> firmware tables. However this is not implemented yet and part of the >> discussion on PCI Passthrough (see [1]). >> >> We need a temporary solution that does not involve any commitment on the >> ABI until Xen is able to discover PCI. >> > > Why can't we handle ITS device creation whenever a virtual ITS driver > receives the MAPD command from dom0/domU. In case of dom0, it's straight > forward dom0 always passes the real ITS device through MAPD command. > This way we can support PCIe devices without hard-coded MSI(x) limit 32, > and platform devices transparently. I used the below code to platform > and PCIe device MSI(x) functionality on QDF2400 server platform. But this breaks our assumption that no ITS commands can ever be propagated at guest's runtime, which is the cornerstone of this series. I agree that this is unfortunate and allowing it would simplify things, but after long discussions we came to the conclusion that it's not feasible to do so: A malicious guest could flood the virtual ITS with MAPD commands. Xen would need to propagate those to the hardware, which relies on the host command queue to have free slots, which we can't guarantee. For technical reasons we can't reschedule the guest (because this is an MMIO trap), also the domain actually triggering the "final" MAPD might not be the culprit, but an actual legitimate user. So we agreed upon issuing all hardware ITS commands before a guest actually starts (DomUs), respectively on hypercalls for Dom0. I think we can do exceptions for Dom0, since it's not supposed to be malicious. So I'd suggest the following: - To make Dom0 run in this version of the patches, especially with platform devices, we allow MAPDs to propagate from Dom0. - We check whether this device has already been mapped. If yes, we map the virtual side and return. - If not mapped already, we possibly somehow sanitize the device ID (using some platform-specific function, for instance) and issue the MAPD and all the possible MAPTIs to the hardware ITS. We might avoid this in the future, when we have proper passthrough support in place. So PCI devices would be mapped by the PHYSOPS hypercall as before, but platform devices would be handled via this way. Does this make sense? I need to work out the details, keep you posted ... Cheers, Andre. > > @@ -383,10 +384,17 @@ static int its_handle_mapd(struct virt_its *its, > uint64_t *cmdptr) > int size = its_cmd_get_size(cmdptr); > bool valid = its_cmd_get_validbit(cmdptr); > paddr_t itt_addr = its_cmd_mask_field(cmdptr, 2, 0, 52) & > GENMASK(51, 8); > + int ret; > > if ( !its->dev_table ) > return -1; > > + size = size < 4 ? 4 : size; > + ret = gicv3_its_map_guest_device(hardware_domain, devid, devid, > size + 1, > + valid); > + if (ret < 0) > + return ret; > + > >
On 27/02/17 17:20, Andre Przywara wrote: > Hi, Hi Andre, > On 24/02/17 19:57, Shanker Donthineni wrote: >> Hi Julien, >> >> >> On 01/31/2017 10:18 AM, Julien Grall wrote: >>> >>> >>> On 31/01/17 16:02, Jaggi, Manish wrote: >>>> >>>> >>>> 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: >>>>>> 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. >>> >>> I gave suggestions on my e-mail but you may have missed it... >>> >>>> 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. >>> >>> ... as well as the part where I am saying that I am not in favor to >>> implement an hypercall temporarily, and against adding a new hypercall >>> for only a couple of weeks. As you may know PHYSDEV hypercall are part >>> of the stable ABI and once they are added they cannot be removed. >>> >>> So we need to be sure the hypercall is necessary. In this case, the >>> hypercall is not necessary as all the information can be found in the >>> firmware tables. However this is not implemented yet and part of the >>> discussion on PCI Passthrough (see [1]). >>> >>> We need a temporary solution that does not involve any commitment on the >>> ABI until Xen is able to discover PCI. >>> >> >> Why can't we handle ITS device creation whenever a virtual ITS driver >> receives the MAPD command from dom0/domU. In case of dom0, it's straight >> forward dom0 always passes the real ITS device through MAPD command. >> This way we can support PCIe devices without hard-coded MSI(x) limit 32, >> and platform devices transparently. I used the below code to platform >> and PCIe device MSI(x) functionality on QDF2400 server platform. > > But this breaks our assumption that no ITS commands can ever be > propagated at guest's runtime, which is the cornerstone of this series. > I agree that this is unfortunate and allowing it would simplify things, > but after long discussions we came to the conclusion that it's not > feasible to do so: > A malicious guest could flood the virtual ITS with MAPD commands. Xen > would need to propagate those to the hardware, which relies on the host > command queue to have free slots, which we can't guarantee. For > technical reasons we can't reschedule the guest (because this is an MMIO > trap), also the domain actually triggering the "final" MAPD might not be > the culprit, but an actual legitimate user. > So we agreed upon issuing all hardware ITS commands before a guest > actually starts (DomUs), respectively on hypercalls for Dom0. > I think we can do exceptions for Dom0, since it's not supposed to be > malicious. Thank you for summarizing the problem :). > So I'd suggest the following: > - To make Dom0 run in this version of the patches, especially with > platform devices, we allow MAPDs to propagate from Dom0. > - We check whether this device has already been mapped. If yes, we > map the virtual side and return. > - If not mapped already, we possibly somehow sanitize the device ID > (using some platform-specific function, for instance) and issue the MAPD > and all the possible MAPTIs to the hardware ITS. We might avoid this in > the future, when we have proper passthrough support in place. I am not sure why you would need per-platform code to sanitize the Device ID. I think a first approach is to trust all input from dom0, we can refine this later one by either reading the configuration space for PCI, for platform device we would need to come up for possibly a new hypercall (this could be discussed in a separate thread). > > So PCI devices would be mapped by the PHYSOPS hypercall as before, but > platform devices would be handled via this way. I don't understand why you still want to implement physdevop hypercalls knowing that they will likely get ditched for ARM and don't provide all the information we need. It is not possible to know the DeviceID from RID without parsing DT and we don't have the number of MSI supported in hand. So it makes no sense to implement those hypercalls. > > Does this make sense? Looking at the implementation of gicv3_its_map_guest_device, for each virtual MAPD issued, you will issue one host MAPD command, one host MAPTI and INV per event. This will potentially fill up the host command queue and takes time to executed (imagine a SYNC at the end). So what will you do if the queue is full? Xen is not preemptible and if you busy loop, dom0 may have its watchdog raised or the RCU stalls. Cheers,
Hi Julien, On 02/28/2017 12:29 PM, Julien Grall wrote: > > > On 27/02/17 17:20, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 24/02/17 19:57, Shanker Donthineni wrote: >>> Hi Julien, >>> >>> >>> On 01/31/2017 10:18 AM, Julien Grall wrote: >>>> >>>> >>>> On 31/01/17 16:02, Jaggi, Manish wrote: >>>>> >>>>> >>>>> 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: >>>>>>> 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. >>>> >>>> I gave suggestions on my e-mail but you may have missed it... >>>> >>>>> 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. >>>> >>>> ... as well as the part where I am saying that I am not in favor to >>>> implement an hypercall temporarily, and against adding a new hypercall >>>> for only a couple of weeks. As you may know PHYSDEV hypercall are part >>>> of the stable ABI and once they are added they cannot be removed. >>>> >>>> So we need to be sure the hypercall is necessary. In this case, the >>>> hypercall is not necessary as all the information can be found in the >>>> firmware tables. However this is not implemented yet and part of the >>>> discussion on PCI Passthrough (see [1]). >>>> >>>> We need a temporary solution that does not involve any commitment >>>> on the >>>> ABI until Xen is able to discover PCI. >>>> >>> >>> Why can't we handle ITS device creation whenever a virtual ITS driver >>> receives the MAPD command from dom0/domU. In case of dom0, it's >>> straight >>> forward dom0 always passes the real ITS device through MAPD command. >>> This way we can support PCIe devices without hard-coded MSI(x) limit >>> 32, >>> and platform devices transparently. I used the below code to platform >>> and PCIe device MSI(x) functionality on QDF2400 server platform. >> >> But this breaks our assumption that no ITS commands can ever be >> propagated at guest's runtime, which is the cornerstone of this series. >> I agree that this is unfortunate and allowing it would simplify things, >> but after long discussions we came to the conclusion that it's not >> feasible to do so: >> A malicious guest could flood the virtual ITS with MAPD commands. Xen >> would need to propagate those to the hardware, which relies on the host >> command queue to have free slots, which we can't guarantee. For >> technical reasons we can't reschedule the guest (because this is an MMIO >> trap), also the domain actually triggering the "final" MAPD might not be >> the culprit, but an actual legitimate user. >> So we agreed upon issuing all hardware ITS commands before a guest >> actually starts (DomUs), respectively on hypercalls for Dom0. >> I think we can do exceptions for Dom0, since it's not supposed to be >> malicious. > > Thank you for summarizing the problem :). > Direct VLPI injection feature is included in GICv4 architecture. A new set of VLPI commands are introduced to map ITS vpend/vprop tables, ITTE setup, and maintenance operations for VLPIs. In case of direct VLPI injection, domU/dom0 LPI commands are mapped to VLPI commands. Some of these commands must be applied to a real ITS hardware whenever XEN receives the ITS commands during runtime. Any thought on this, how we are going to support a direct VLPI injection without prolongating dom0/domU ITS commands to hardware at runtime?
On 01/03/17 19:42, Shanker Donthineni wrote: > Hi Julien, Hi Shanker, > On 02/28/2017 12:29 PM, Julien Grall wrote: >> On 27/02/17 17:20, Andre Przywara wrote: > Direct VLPI injection feature is included in GICv4 architecture. A new > set of VLPI commands are introduced to map ITS vpend/vprop tables, ITTE > setup, and maintenance operations for VLPIs. In case of direct VLPI > injection, domU/dom0 LPI commands are mapped to VLPI commands. Some of > these commands must be applied to a real ITS hardware whenever XEN > receives the ITS commands during runtime. > > > Any thought on this, how we are going to support a direct VLPI injection > without prolongating dom0/domU ITS commands to hardware at runtime? direct vLPI injection will indeed require to propagate commands. But as the host command queue is shared among multiple guest, we have to prevent a guest to overflow the host command queue and affecting other guests. During the discussion for GICv3 ITS support in Xen, we looked at various solution (see the various design doc sent by Ian Campbell [1]) and the only suitable one for it was to decouple vITS and ITS. This is what Andre has implemented in this series. I don't know yet how we can make things secure for direct vLPI injection. For the time being, I think we should focus to get GICv3 ITS supported as it is a requirement to get MSI supported. Once this is done, we can think about integrating directly vLPI in the code. Feel free to start a new thread about this. Cheers, [1] https://xenbits.xen.org/people/ianc/vits/
diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c index 27bbbda..6e02de4 100644 --- a/xen/arch/arm/physdev.c +++ b/xen/arch/arm/physdev.c @@ -9,11 +9,32 @@ #include <xen/lib.h> #include <xen/errno.h> #include <xen/sched.h> +#include <xen/guest_access.h> +#include <asm/gic_v3_its.h> #include <asm/hypercall.h> int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { + struct physdev_manage_pci manage; + u32 devid; + int ret; + + switch (cmd) + { + 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); + + return ret; + } + gdprintk(XENLOG_DEBUG, "PHYSDEVOP cmd=%d: not implemented\n", cmd); return -ENOSYS; }
To get MSIs from devices forwarded to a CPU, we need to name the device and its MSIs by mapping them to an ITS. Since this involves queueing commands to the ITS command queue, we can't really afford to do this during the guest's runtime, as this would open up a denial-of-service attack vector. So we require every device with MSI interrupts to be mapped explicitly by Dom0. For Dom0 itself we can just use the existing PCI physdev_op hypercalls, which the existing Linux kernel issues already. So upon receipt of this hypercall we map the device to the hardware ITS and prepare it to be later mapped by the virtual ITS by using the very same device ID (for Dom0 only). Also we ask for mapping 32 LPIs to cover 32 MSIs that the device may use. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/physdev.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)