Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
Hi Vijay, On 19/03/2015 14:37, vijay.kil...@gmail.com wrote: From: Vijaya Kumar K Add ITS support for arm. Following major features are supported - GICv3 ITS support for arm64 platform - Supports multi ITS node - LPI descriptors are allocated on-demand - Only ITS Dom0 is supported Tested with single ITS node. It would have been nice to give a link to your github in this cover letter too. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
Hi Vijay, On 19/03/2015 14:37, vijay.kil...@gmail.com wrote: From: Vijaya Kumar K Add ITS support for arm. Following major features are supported - GICv3 ITS support for arm64 platform - Supports multi ITS node - LPI descriptors are allocated on-demand - Only ITS Dom0 is supported Tested with single ITS node. Some though about the whole design: Your vGIC ITS driver does too much things. In general a virtual driver should only emulate the hardware for the domain and forward the request to the physical driver. Your series adds device management (create/free) in the vITS, which is wrong. How do you check if the domain can use the device? Currently, you allow any domain to use any device. That would bring a big mess with guest using passthrough. Also, does the guess will always pass the correct devid? If not how do you plan to handle it? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
On Fri, Mar 20, 2015 at 9:53 PM, Julien Grall wrote: > Hi Vijay, > > On 19/03/2015 14:37, vijay.kil...@gmail.com wrote: >> >> From: Vijaya Kumar K >> >> Add ITS support for arm. Following major features >> are supported >> - GICv3 ITS support for arm64 platform >> - Supports multi ITS node >> - LPI descriptors are allocated on-demand >> - Only ITS Dom0 is supported >> >> Tested with single ITS node. > > > Some though about the whole design: > > Your vGIC ITS driver does too much things. In general a virtual driver > should only emulate the hardware for the domain and forward the request to > the physical driver. > > Your series adds device management (create/free) in the vITS, which is > wrong. The device is added to ITS using MAPD command. All ITS commands are based on this device added using MAPD command. So vITS driver needs to manage this. > > How do you check if the domain can use the device? > Currently, you allow any domain to use any device. That would bring a big > mess with guest using passthrough. ITS driver does not know which PCI device is assigned for which domain. I think it should be done by above layers along with pci drivers in Xen. vITS assume that the domain that sends MAPD command owns the device > > Also, does the guess will always pass the correct devid? If not how do you > plan to handle it? The only place it validate pci devid is vITS driver will call pci helper function to get ITS dt node for the BDF. Or we can add a check in MAPD command by calling pci helper function to know if the BDF is valid or not. Regards Vijay ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
On 23/03/15 12:37, Vijay Kilari wrote: > On Fri, Mar 20, 2015 at 9:53 PM, Julien Grall wrote: >> Hi Vijay, >> >> On 19/03/2015 14:37, vijay.kil...@gmail.com wrote: >>> >>> From: Vijaya Kumar K >>> >>> Add ITS support for arm. Following major features >>> are supported >>> - GICv3 ITS support for arm64 platform >>> - Supports multi ITS node >>> - LPI descriptors are allocated on-demand >>> - Only ITS Dom0 is supported >>> >>> Tested with single ITS node. >> >> >> Some though about the whole design: >> >> Your vGIC ITS driver does too much things. In general a virtual driver >> should only emulate the hardware for the domain and forward the request to >> the physical driver. >> >> Your series adds device management (create/free) in the vITS, which is >> wrong. > > The device is added to ITS using MAPD command. All ITS commands are based > on this device added using MAPD command. So vITS driver needs to manage > this. The ITS still have to manage in someway the device. There is lots of information that doesn't need to be created at every mapd (such as the number of MSI). Handling device management in ITS would help to check the validity of the access. Which you are currently ignoring... >> >> How do you check if the domain can use the device? >> Currently, you allow any domain to use any device. That would bring a big >> mess with guest using passthrough. > > ITS driver does not know which PCI device is assigned for which domain. Wrong, Xen knows which device is assigned to which domain so ITS does. > I think it should be done by above layers along with pci drivers in Xen. > vITS assume that the domain that sends MAPD command owns the device The vITS emulates hardware for a specific domain. A malicious guest could send request to a not own device. You have to think about security in the vITS otherwise we will end up with many XSA in this code... Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
On Mon, Mar 23, 2015 at 6:41 PM, Julien Grall wrote: > On 23/03/15 12:37, Vijay Kilari wrote: >> On Fri, Mar 20, 2015 at 9:53 PM, Julien Grall >> wrote: >>> Hi Vijay, >>> >>> On 19/03/2015 14:37, vijay.kil...@gmail.com wrote: From: Vijaya Kumar K Add ITS support for arm. Following major features are supported - GICv3 ITS support for arm64 platform - Supports multi ITS node - LPI descriptors are allocated on-demand - Only ITS Dom0 is supported Tested with single ITS node. >>> >>> >>> Some though about the whole design: >>> >>> Your vGIC ITS driver does too much things. In general a virtual driver >>> should only emulate the hardware for the domain and forward the request to >>> the physical driver. >>> >>> Your series adds device management (create/free) in the vITS, which is >>> wrong. >> >> The device is added to ITS using MAPD command. All ITS commands are based >> on this device added using MAPD command. So vITS driver needs to manage >> this. > > The ITS still have to manage in someway the device. There is lots of > information that doesn't need to be created at every mapd (such as the > number of MSI). First assumption is VITS driver owns converting Virtual ITS commands to Physical ITS commands. So based on this - arch_domain contains list of all the devices attached for the domain. - On MAPD command, device is created, physical LPI's and virtual LPIs are allocated and added to domains list and further all other ITS commands except MAPC, INVALL and SYNC depend on device information to convert virtual ITS commands to physical ITS commands. > > Handling device management in ITS would help to check the validity of > the access. Which you are currently ignoring... > >>> >>> How do you check if the domain can use the device? >>> Currently, you allow any domain to use any device. That would bring a big >>> mess with guest using passthrough. >> >> ITS driver does not know which PCI device is assigned for which domain. > > Wrong, Xen knows which device is assigned to which domain so ITS does. > >> I think it should be done by above layers along with pci drivers in Xen. >> vITS assume that the domain that sends MAPD command owns the device > > The vITS emulates hardware for a specific domain. A malicious guest > could send request to a not own device. OK. On MAPD command when ITS device is created, I can introduce pci helper function to know if particular device is assigned to domain or not. > > You have to think about security in the vITS otherwise we will end up > with many XSA in this code... > For every virtual ITS command parameters are validated before issuing physical command, except check on device id which I will take care in next version ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
On 23/03/15 15:18, Vijay Kilari wrote: >> The ITS still have to manage in someway the device. There is lots of >> information that doesn't need to be created at every mapd (such as the >> number of MSI). > > First assumption is VITS driver owns converting Virtual ITS commands > to Physical ITS commands. So based on this > > - arch_domain contains list of all the devices attached for the domain. > - On MAPD command, device is created, physical LPI's and virtual LPIs > are allocated >and added to domains list and further all other ITS commands except > MAPC, INVALL and SYNC >depend on device information to convert virtual ITS commands to > physical ITS commands. I didn't understand what you said. >> >> Handling device management in ITS would help to check the validity of >> the access. Which you are currently ignoring... >> How do you check if the domain can use the device? Currently, you allow any domain to use any device. That would bring a big mess with guest using passthrough. >>> >>> ITS driver does not know which PCI device is assigned for which domain. >> >> Wrong, Xen knows which device is assigned to which domain so ITS does. >> >>> I think it should be done by above layers along with pci drivers in Xen. >>> vITS assume that the domain that sends MAPD command owns the device >> >> The vITS emulates hardware for a specific domain. A malicious guest >> could send request to a not own device. > > OK. On MAPD command when ITS device is created, I can introduce pci helper > function to know if particular device is assigned to domain or not. > >> >> You have to think about security in the vITS otherwise we will end up >> with many XSA in this code... >> > > For every virtual ITS command parameters are validated > before issuing physical command, except check on device id which I will > take care in next version The check on device id is not the only check missing... You also have to validate ID, Size... with the number of bits supported by the ITS. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
On Mon, Mar 23, 2015 at 9:00 PM, Julien Grall wrote: > On 23/03/15 15:18, Vijay Kilari wrote: >>> The ITS still have to manage in someway the device. There is lots of >>> information that doesn't need to be created at every mapd (such as the >>> number of MSI). >> >> First assumption is VITS driver owns converting Virtual ITS commands >> to Physical ITS commands. So based on this >> >> - arch_domain contains list of all the devices attached for the domain. >> - On MAPD command, device is created, physical LPI's and virtual LPIs >> are allocated >>and added to domains list and further all other ITS commands except >> MAPC, INVALL and SYNC >>depend on device information to convert virtual ITS commands to >> physical ITS commands. > > I didn't understand what you said. I mean, all the virtual ITS commands has to be converted to physical ITS commands. The (most)information required to do this conversion is based on device_id for most of the commands (except MAPC, SYNC & INVALL) So vITS driver contains device management. However, all the devices managed using linked list attached to a domain. Few API's are added in physical ITS driver to query Physical Collection ID (pCID) to Virtual Collection ID (vCID) and Virtual Target address(vTA) to Physical Target Address(pTA) because pCID & pTA info is available in physical ITS driver. Refer to : 4.9.22 Command Mapping for a Guest with a GICv3 ITS of PRD03-GENC-010745 20.0 Overall the functionality was split as follows; - Physical ITS driver manages Physical LPI allocation, Sending Physical commands, Physical Collection ID & Target addresses - Virtual ITS driver manages Virtual LPI allocation, Device creation for respective domain, Conversion of virtual ITS commands to Physical ITS commands, Virtual Collection ID's Virtual Target address, GITS register emulation, LPI pending/Property table emulation. It would be better if you go through the patch set and comment under relevant code and we can discuss. > >>> >>> Handling device management in ITS would help to check the validity of >>> the access. Which you are currently ignoring... >>> > > How do you check if the domain can use the device? > Currently, you allow any domain to use any device. That would bring a big > mess with guest using passthrough. ITS driver does not know which PCI device is assigned for which domain. >>> >>> Wrong, Xen knows which device is assigned to which domain so ITS does. >>> I think it should be done by above layers along with pci drivers in Xen. vITS assume that the domain that sends MAPD command owns the device >>> >>> The vITS emulates hardware for a specific domain. A malicious guest >>> could send request to a not own device. >> >> OK. On MAPD command when ITS device is created, I can introduce pci helper >> function to know if particular device is assigned to domain or not. >> >>> >>> You have to think about security in the vITS otherwise we will end up >>> with many XSA in this code... >>> >> >> For every virtual ITS command parameters are validated >> before issuing physical command, except check on device id which I will >> take care in next version > > The check on device id is not the only check missing... You also have to > validate ID, Size... with the number of bits supported by the ITS. > > Regards, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/22] xen/arm: Add ITS support
On 23/03/15 16:09, Vijay Kilari wrote: > On Mon, Mar 23, 2015 at 9:00 PM, Julien Grall wrote: >> On 23/03/15 15:18, Vijay Kilari wrote: The ITS still have to manage in someway the device. There is lots of information that doesn't need to be created at every mapd (such as the number of MSI). >>> >>> First assumption is VITS driver owns converting Virtual ITS commands >>> to Physical ITS commands. So based on this >>> >>> - arch_domain contains list of all the devices attached for the domain. >>> - On MAPD command, device is created, physical LPI's and virtual LPIs >>> are allocated >>>and added to domains list and further all other ITS commands except >>> MAPC, INVALL and SYNC >>>depend on device information to convert virtual ITS commands to >>> physical ITS commands. >> >> I didn't understand what you said. > > I mean, all the virtual ITS commands has to be converted to > physical ITS commands. The (most)information required to do this conversion > is based on device_id for most of the commands (except MAPC, SYNC & INVALL) > So vITS driver contains device management. > However, all the devices managed using linked list attached to a domain. > > Few API's are added in physical ITS driver to query > Physical Collection ID (pCID) to Virtual Collection ID (vCID) and > Virtual Target address(vTA) > to Physical Target Address(pTA) because pCID & pTA info is available > in physical ITS driver. > > Refer to : 4.9.22 Command Mapping for a Guest with a GICv3 ITS > of PRD03-GENC-010745 20.0 A reference to this section on this commit message would have been very useful... > Overall the functionality was split as follows; > > - Physical ITS driver manages Physical LPI allocation, Sending > Physical commands, > Physical Collection ID & Target addresses > - Virtual ITS driver manages Virtual LPI allocation, Device creation > for respective domain, > Conversion of virtual ITS commands to Physical ITS commands, Virtual > Collection ID's > Virtual Target address, GITS register emulation, LPI > pending/Property table emulation. > > It would be better if you go through the patch set and comment under > relevant code > and we can discuss. It's nearly impossible to review this series one patch by one patch. I have to look all the things together. That's why I bring the discussion here. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
