Message ID | d292392268df2c74c4103a82ef917072643407a8.1633540842.git.rahul.singh@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | PCI devices passthrough on Arm | expand |
On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > ARM architecture does not implement I/O ports. Ignore this call on ARM > to avoid the overhead of making a hypercall just for Xen to return > -ENOSYS. What is the cal trace of this function actually on Arm? AFAICT libxl will only call xc_domain_ioport_permission if there are IO ports explicitly defined in the guest configuration, or if any of the BARs of the PCI device is in the IO space, which is not possible on Arm. Thanks, Roger.
Hi Roger, > On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >> ARM architecture does not implement I/O ports. Ignore this call on ARM >> to avoid the overhead of making a hypercall just for Xen to return >> -ENOSYS. > > What is the cal trace of this function actually on Arm? > > AFAICT libxl will only call xc_domain_ioport_permission if there are > IO ports explicitly defined in the guest configuration, or if any of > the BARs of the PCI device is in the IO space, which is not possible > on Arm. PCI devices BARs can be in the IO space as the PCI devices are not Arm specific. There is not ioports on arm so to be used those can be in some cases remapped and accessed as MMIOs or are not possible to use at all. But the IO space does appear when BARs are listed even on Arm. Regards Bertrand > > Thanks, Roger.
On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: > Hi Roger, > > > On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: > > > > On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > >> ARM architecture does not implement I/O ports. Ignore this call on ARM > >> to avoid the overhead of making a hypercall just for Xen to return > >> -ENOSYS. > > > > What is the cal trace of this function actually on Arm? > > > > AFAICT libxl will only call xc_domain_ioport_permission if there are > > IO ports explicitly defined in the guest configuration, or if any of > > the BARs of the PCI device is in the IO space, which is not possible > > on Arm. > > PCI devices BARs can be in the IO space as the PCI devices are not > Arm specific. There is not ioports on arm so to be used those can be > in some cases remapped and accessed as MMIOs or are not possible > to use at all. > > But the IO space does appear when BARs are listed even on Arm. Urg, I wonder whether those devices with IO BARs will work correctly under Arm then. How do you know whether the BAR has been remapped from IO space into MMIO? IMO instead of faking a successful return value from xc_domain_ioport_permission we should avoid the call completely in the first place, specially if we need to instead issue a call to xc_domain_iomem_permission. Thanks, Roger.
Hi Roger, + Oleksandr to have a better PCI expert then me. > On 11 Oct 2021, at 14:20, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: >> Hi Roger, >> >>> On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >>>> ARM architecture does not implement I/O ports. Ignore this call on ARM >>>> to avoid the overhead of making a hypercall just for Xen to return >>>> -ENOSYS. >>> >>> What is the cal trace of this function actually on Arm? >>> >>> AFAICT libxl will only call xc_domain_ioport_permission if there are >>> IO ports explicitly defined in the guest configuration, or if any of >>> the BARs of the PCI device is in the IO space, which is not possible >>> on Arm. >> >> PCI devices BARs can be in the IO space as the PCI devices are not >> Arm specific. There is not ioports on arm so to be used those can be >> in some cases remapped and accessed as MMIOs or are not possible >> to use at all. >> >> But the IO space does appear when BARs are listed even on Arm. > > Urg, I wonder whether those devices with IO BARs will work correctly > under Arm then. > > How do you know whether the BAR has been remapped from IO space into > MMIO? We cannot, I think the platform will define if this is the case and where. @oleksandr: I remember that this was discussed during some of our meetings but I have no idea of the details here, can you help ? > > IMO instead of faking a successful return value from > xc_domain_ioport_permission we should avoid the call completely in the > first place, specially if we need to instead issue a call to > xc_domain_iomem_permission. At the end we will never have to issue this because this will never be a matter of “iomem” permission as there would not be any way to cut on something under the page. If this is to be supported one day, it will probably have to be fully emulated to keep the isolation. Right now on arm you can just make the more simple assumption that ioports are just not supported. Regards Bertrand > > Thanks, Roger.
On Mon, Oct 11, 2021 at 01:40:30PM +0000, Bertrand Marquis wrote: > Hi Roger, > > + Oleksandr to have a better PCI expert then me. > > > On 11 Oct 2021, at 14:20, Roger Pau Monné <roger.pau@citrix.com> wrote: > > > > On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: > >> Hi Roger, > >> > >>> On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: > >>> > >>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > >>>> ARM architecture does not implement I/O ports. Ignore this call on ARM > >>>> to avoid the overhead of making a hypercall just for Xen to return > >>>> -ENOSYS. > >>> > >>> What is the cal trace of this function actually on Arm? > >>> > >>> AFAICT libxl will only call xc_domain_ioport_permission if there are > >>> IO ports explicitly defined in the guest configuration, or if any of > >>> the BARs of the PCI device is in the IO space, which is not possible > >>> on Arm. > >> > >> PCI devices BARs can be in the IO space as the PCI devices are not > >> Arm specific. There is not ioports on arm so to be used those can be > >> in some cases remapped and accessed as MMIOs or are not possible > >> to use at all. > >> > >> But the IO space does appear when BARs are listed even on Arm. > > > > Urg, I wonder whether those devices with IO BARs will work correctly > > under Arm then. > > > > How do you know whether the BAR has been remapped from IO space into > > MMIO? > > We cannot, I think the platform will define if this is the case and where. > @oleksandr: I remember that this was discussed during some of our > meetings but I have no idea of the details here, can you help ? > > > > > IMO instead of faking a successful return value from > > xc_domain_ioport_permission we should avoid the call completely in the > > first place, specially if we need to instead issue a call to > > xc_domain_iomem_permission. > > At the end we will never have to issue this because this will never be a matter > of “iomem” permission as there would not be any way to cut on something under > the page. If this is to be supported one day, it will probably have to be fully emulated > to keep the isolation. So you have a set of memory pages that map accesses from MMIO into IO space but it's not possible to isolate specific IO port regions as they are all contiguous in the same page(s). > Right now on arm you can just make the more simple assumption that ioports are > just not supported. Would it make sense in the future to provide a memory region to guests in order to use for IO port accesses, and call xc_domain_ioport_permission to set which ports would be allowed? I think the commit message needs to at least be expanded in order to contain the information provided here. It might also be helpful to figure out whether we would have to handle IO port accesses in the future on Arm, or if it's fine to just ignore them. Thanks, Roger.
Hi, all On 11.10.21 16:40, Bertrand Marquis wrote: > Hi Roger, > > + Oleksandr to have a better PCI expert then me. > >> On 11 Oct 2021, at 14:20, Roger Pau Monné <roger.pau@citrix.com> wrote: >> >> On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: >>> Hi Roger, >>> >>>> On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>> >>>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >>>>> ARM architecture does not implement I/O ports. Ignore this call on ARM >>>>> to avoid the overhead of making a hypercall just for Xen to return >>>>> -ENOSYS. >>>> What is the cal trace of this function actually on Arm? >>>> >>>> AFAICT libxl will only call xc_domain_ioport_permission if there are >>>> IO ports explicitly defined in the guest configuration, or if any of >>>> the BARs of the PCI device is in the IO space, which is not possible >>>> on Arm. >>> PCI devices BARs can be in the IO space as the PCI devices are not >>> Arm specific. There is not ioports on arm so to be used those can be >>> in some cases remapped and accessed as MMIOs or are not possible >>> to use at all. >>> >>> But the IO space does appear when BARs are listed even on Arm. >> Urg, I wonder whether those devices with IO BARs will work correctly >> under Arm then. >> >> How do you know whether the BAR has been remapped from IO space into >> MMIO? > We cannot, I think the platform will define if this is the case and where. > @oleksandr: I remember that this was discussed during some of our > meetings but I have no idea of the details here, can you help ? > For the guest domains we emulate a host bridge without IO, so the guest won't be able to assign any IO memory at all.
Hi Roger, > On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Mon, Oct 11, 2021 at 01:40:30PM +0000, Bertrand Marquis wrote: >> Hi Roger, >> >> + Oleksandr to have a better PCI expert then me. >> >>> On 11 Oct 2021, at 14:20, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: >>>> Hi Roger, >>>> >>>>> On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> >>>>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >>>>>> ARM architecture does not implement I/O ports. Ignore this call on ARM >>>>>> to avoid the overhead of making a hypercall just for Xen to return >>>>>> -ENOSYS. >>>>> >>>>> What is the cal trace of this function actually on Arm? >>>>> >>>>> AFAICT libxl will only call xc_domain_ioport_permission if there are >>>>> IO ports explicitly defined in the guest configuration, or if any of >>>>> the BARs of the PCI device is in the IO space, which is not possible >>>>> on Arm. >>>> >>>> PCI devices BARs can be in the IO space as the PCI devices are not >>>> Arm specific. There is not ioports on arm so to be used those can be >>>> in some cases remapped and accessed as MMIOs or are not possible >>>> to use at all. >>>> >>>> But the IO space does appear when BARs are listed even on Arm. >>> >>> Urg, I wonder whether those devices with IO BARs will work correctly >>> under Arm then. >>> >>> How do you know whether the BAR has been remapped from IO space into >>> MMIO? >> >> We cannot, I think the platform will define if this is the case and where. >> @oleksandr: I remember that this was discussed during some of our >> meetings but I have no idea of the details here, can you help ? >> >>> >>> IMO instead of faking a successful return value from >>> xc_domain_ioport_permission we should avoid the call completely in the >>> first place, specially if we need to instead issue a call to >>> xc_domain_iomem_permission. >> >> At the end we will never have to issue this because this will never be a matter >> of “iomem” permission as there would not be any way to cut on something under >> the page. If this is to be supported one day, it will probably have to be fully emulated >> to keep the isolation. > > So you have a set of memory pages that map accesses from > MMIO into IO space but it's not possible to isolate specific IO port > regions as they are all contiguous in the same page(s). Exact. > >> Right now on arm you can just make the more simple assumption that ioports are >> just not supported. > > Would it make sense in the future to provide a memory region to guests > in order to use for IO port accesses, and call > xc_domain_ioport_permission to set which ports would be allowed? Right now we do not plan to support this at all and we will have to figure this out if we do this one day. > > I think the commit message needs to at least be expanded in order to > contain the information provided here. It might also be helpful to > figure out whether we would have to handle IO port accesses in the > future on Arm, or if it's fine to just ignore them. All our investigations and tests have been done without supporting it without any issues so this is not a critical feature (most devices can be operated without using the I/O ports). I can add the following to the commit message: I/O ports accessible through MMIO are currently not supported by Xen. Regards Bertrand > > Thanks, Roger.
On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: > Hi Roger, > > > On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: > > > > On Mon, Oct 11, 2021 at 01:40:30PM +0000, Bertrand Marquis wrote: > >> Hi Roger, > >> > >> + Oleksandr to have a better PCI expert then me. > >> > >>> On 11 Oct 2021, at 14:20, Roger Pau Monné <roger.pau@citrix.com> wrote: > >>> > >>> On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: > >>>> Hi Roger, > >>>> > >>>>> On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: > >>>>> > >>>>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > >>>>>> ARM architecture does not implement I/O ports. Ignore this call on ARM > >>>>>> to avoid the overhead of making a hypercall just for Xen to return > >>>>>> -ENOSYS. > >>>>> > >>>>> What is the cal trace of this function actually on Arm? > >>>>> > >>>>> AFAICT libxl will only call xc_domain_ioport_permission if there are > >>>>> IO ports explicitly defined in the guest configuration, or if any of > >>>>> the BARs of the PCI device is in the IO space, which is not possible > >>>>> on Arm. > >>>> > >>>> PCI devices BARs can be in the IO space as the PCI devices are not > >>>> Arm specific. There is not ioports on arm so to be used those can be > >>>> in some cases remapped and accessed as MMIOs or are not possible > >>>> to use at all. > >>>> > >>>> But the IO space does appear when BARs are listed even on Arm. > >>> > >>> Urg, I wonder whether those devices with IO BARs will work correctly > >>> under Arm then. > >>> > >>> How do you know whether the BAR has been remapped from IO space into > >>> MMIO? > >> > >> We cannot, I think the platform will define if this is the case and where. > >> @oleksandr: I remember that this was discussed during some of our > >> meetings but I have no idea of the details here, can you help ? > >> > >>> > >>> IMO instead of faking a successful return value from > >>> xc_domain_ioport_permission we should avoid the call completely in the > >>> first place, specially if we need to instead issue a call to > >>> xc_domain_iomem_permission. > >> > >> At the end we will never have to issue this because this will never be a matter > >> of “iomem” permission as there would not be any way to cut on something under > >> the page. If this is to be supported one day, it will probably have to be fully emulated > >> to keep the isolation. > > > > So you have a set of memory pages that map accesses from > > MMIO into IO space but it's not possible to isolate specific IO port > > regions as they are all contiguous in the same page(s). > > Exact. > > > > >> Right now on arm you can just make the more simple assumption that ioports are > >> just not supported. > > > > Would it make sense in the future to provide a memory region to guests > > in order to use for IO port accesses, and call > > xc_domain_ioport_permission to set which ports would be allowed? > > Right now we do not plan to support this at all and we will have to > figure this out if we do this one day. > > > > > I think the commit message needs to at least be expanded in order to > > contain the information provided here. It might also be helpful to > > figure out whether we would have to handle IO port accesses in the > > future on Arm, or if it's fine to just ignore them. > > All our investigations and tests have been done without supporting it > without any issues so this is not a critical feature (most devices can > be operated without using the I/O ports). IMO we should let the users know they attempted to use a device with BARs in the IO space, and that those BARs won't be accessible which could make the device not function as expected. Do you think it would be reasonable to attempt the hypercall on Arm also, and in case of error (on Arm) just print a warning message and continue operations as normal? Thanks, Roger.
Hi Roger, > On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >> Hi Roger, >> >>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> >>> On Mon, Oct 11, 2021 at 01:40:30PM +0000, Bertrand Marquis wrote: >>>> Hi Roger, >>>> >>>> + Oleksandr to have a better PCI expert then me. >>>> >>>>> On 11 Oct 2021, at 14:20, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> >>>>> On Mon, Oct 11, 2021 at 12:11:04PM +0000, Bertrand Marquis wrote: >>>>>> Hi Roger, >>>>>> >>>>>>> On 11 Oct 2021, at 12:47, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>> >>>>>>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >>>>>>>> ARM architecture does not implement I/O ports. Ignore this call on ARM >>>>>>>> to avoid the overhead of making a hypercall just for Xen to return >>>>>>>> -ENOSYS. >>>>>>> >>>>>>> What is the cal trace of this function actually on Arm? >>>>>>> >>>>>>> AFAICT libxl will only call xc_domain_ioport_permission if there are >>>>>>> IO ports explicitly defined in the guest configuration, or if any of >>>>>>> the BARs of the PCI device is in the IO space, which is not possible >>>>>>> on Arm. >>>>>> >>>>>> PCI devices BARs can be in the IO space as the PCI devices are not >>>>>> Arm specific. There is not ioports on arm so to be used those can be >>>>>> in some cases remapped and accessed as MMIOs or are not possible >>>>>> to use at all. >>>>>> >>>>>> But the IO space does appear when BARs are listed even on Arm. >>>>> >>>>> Urg, I wonder whether those devices with IO BARs will work correctly >>>>> under Arm then. >>>>> >>>>> How do you know whether the BAR has been remapped from IO space into >>>>> MMIO? >>>> >>>> We cannot, I think the platform will define if this is the case and where. >>>> @oleksandr: I remember that this was discussed during some of our >>>> meetings but I have no idea of the details here, can you help ? >>>> >>>>> >>>>> IMO instead of faking a successful return value from >>>>> xc_domain_ioport_permission we should avoid the call completely in the >>>>> first place, specially if we need to instead issue a call to >>>>> xc_domain_iomem_permission. >>>> >>>> At the end we will never have to issue this because this will never be a matter >>>> of “iomem” permission as there would not be any way to cut on something under >>>> the page. If this is to be supported one day, it will probably have to be fully emulated >>>> to keep the isolation. >>> >>> So you have a set of memory pages that map accesses from >>> MMIO into IO space but it's not possible to isolate specific IO port >>> regions as they are all contiguous in the same page(s). >> >> Exact. >> >>> >>>> Right now on arm you can just make the more simple assumption that ioports are >>>> just not supported. >>> >>> Would it make sense in the future to provide a memory region to guests >>> in order to use for IO port accesses, and call >>> xc_domain_ioport_permission to set which ports would be allowed? >> >> Right now we do not plan to support this at all and we will have to >> figure this out if we do this one day. >> >>> >>> I think the commit message needs to at least be expanded in order to >>> contain the information provided here. It might also be helpful to >>> figure out whether we would have to handle IO port accesses in the >>> future on Arm, or if it's fine to just ignore them. >> >> All our investigations and tests have been done without supporting it >> without any issues so this is not a critical feature (most devices can >> be operated without using the I/O ports). > > IMO we should let the users know they attempted to use a device with > BARs in the IO space, and that those BARs won't be accessible which > could make the device not function as expected. > > Do you think it would be reasonable to attempt the hypercall on Arm > also, and in case of error (on Arm) just print a warning message and > continue operations as normal? I think this would lead to a warning printed on lots of devices where in fact there would be no issues. If this is an issue for a device driver because it cannot operate without I/O ports, this will be raised by the driver inside the guest. So in the current state I think the way to do it is right. Regards Bertrand > > Thanks, Roger.
On 11.10.2021 19:11, Bertrand Marquis wrote: >> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>> I think the commit message needs to at least be expanded in order to >>>> contain the information provided here. It might also be helpful to >>>> figure out whether we would have to handle IO port accesses in the >>>> future on Arm, or if it's fine to just ignore them. >>> >>> All our investigations and tests have been done without supporting it >>> without any issues so this is not a critical feature (most devices can >>> be operated without using the I/O ports). >> >> IMO we should let the users know they attempted to use a device with >> BARs in the IO space, and that those BARs won't be accessible which >> could make the device not function as expected. >> >> Do you think it would be reasonable to attempt the hypercall on Arm >> also, and in case of error (on Arm) just print a warning message and >> continue operations as normal? > > I think this would lead to a warning printed on lots of devices where in > fact there would be no issues. > > If this is an issue for a device driver because it cannot operate without > I/O ports, this will be raised by the driver inside the guest. On what basis would the driver complain? The kernel might know of the MMIO equivalent for ports, and hence might allow the driver to properly obtain whatever is needed to later access the ports. Just that the port accesses then wouldn't work (possibly crashing the guest, or making it otherwise misbehave). Jan
Hi Jan, > On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: > > On 11.10.2021 19:11, Bertrand Marquis wrote: >>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> I think the commit message needs to at least be expanded in order to >>>>> contain the information provided here. It might also be helpful to >>>>> figure out whether we would have to handle IO port accesses in the >>>>> future on Arm, or if it's fine to just ignore them. >>>> >>>> All our investigations and tests have been done without supporting it >>>> without any issues so this is not a critical feature (most devices can >>>> be operated without using the I/O ports). >>> >>> IMO we should let the users know they attempted to use a device with >>> BARs in the IO space, and that those BARs won't be accessible which >>> could make the device not function as expected. >>> >>> Do you think it would be reasonable to attempt the hypercall on Arm >>> also, and in case of error (on Arm) just print a warning message and >>> continue operations as normal? >> >> I think this would lead to a warning printed on lots of devices where in >> fact there would be no issues. >> >> If this is an issue for a device driver because it cannot operate without >> I/O ports, this will be raised by the driver inside the guest. > > On what basis would the driver complain? The kernel might know of > the MMIO equivalent for ports, and hence might allow the driver > to properly obtain whatever is needed to later access the ports. > Just that the port accesses then wouldn't work (possibly crashing > the guest, or making it otherwise misbehave). As ECAM and Arm does not support I/O ports, a driver requesting access to them would get an error back. So in practice it is not possible to try to access the ioports as there is no way on arm to use them (no instructions). A driver could misbehave by ignoring the fact that ioports are not there but I am not quite sure how we could solve that as it would be a bug in the driver. Regards Bertrand > > Jan >
On 12.10.2021 10:41, Bertrand Marquis wrote: > Hi Jan, > >> On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 11.10.2021 19:11, Bertrand Marquis wrote: >>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>> I think the commit message needs to at least be expanded in order to >>>>>> contain the information provided here. It might also be helpful to >>>>>> figure out whether we would have to handle IO port accesses in the >>>>>> future on Arm, or if it's fine to just ignore them. >>>>> >>>>> All our investigations and tests have been done without supporting it >>>>> without any issues so this is not a critical feature (most devices can >>>>> be operated without using the I/O ports). >>>> >>>> IMO we should let the users know they attempted to use a device with >>>> BARs in the IO space, and that those BARs won't be accessible which >>>> could make the device not function as expected. >>>> >>>> Do you think it would be reasonable to attempt the hypercall on Arm >>>> also, and in case of error (on Arm) just print a warning message and >>>> continue operations as normal? >>> >>> I think this would lead to a warning printed on lots of devices where in >>> fact there would be no issues. >>> >>> If this is an issue for a device driver because it cannot operate without >>> I/O ports, this will be raised by the driver inside the guest. >> >> On what basis would the driver complain? The kernel might know of >> the MMIO equivalent for ports, and hence might allow the driver >> to properly obtain whatever is needed to later access the ports. >> Just that the port accesses then wouldn't work (possibly crashing >> the guest, or making it otherwise misbehave). > > As ECAM and Arm does not support I/O ports, a driver requesting access > to them would get an error back. > So in practice it is not possible to try to access the ioports as there is no > way on arm to use them (no instructions). > > A driver could misbehave by ignoring the fact that ioports are not there but > I am not quite sure how we could solve that as it would be a bug in the driver. The minimal thing I'd suggest (or maybe you're doing this already) would be to expose such BARs to the guest as r/o zero, rather than letting their port nature "shine through". Jan
On 12.10.21 12:32, Jan Beulich wrote: > On 12.10.2021 10:41, Bertrand Marquis wrote: >> Hi Jan, >> >>> On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 11.10.2021 19:11, Bertrand Marquis wrote: >>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>> I think the commit message needs to at least be expanded in order to >>>>>>> contain the information provided here. It might also be helpful to >>>>>>> figure out whether we would have to handle IO port accesses in the >>>>>>> future on Arm, or if it's fine to just ignore them. >>>>>> All our investigations and tests have been done without supporting it >>>>>> without any issues so this is not a critical feature (most devices can >>>>>> be operated without using the I/O ports). >>>>> IMO we should let the users know they attempted to use a device with >>>>> BARs in the IO space, and that those BARs won't be accessible which >>>>> could make the device not function as expected. >>>>> >>>>> Do you think it would be reasonable to attempt the hypercall on Arm >>>>> also, and in case of error (on Arm) just print a warning message and >>>>> continue operations as normal? >>>> I think this would lead to a warning printed on lots of devices where in >>>> fact there would be no issues. >>>> >>>> If this is an issue for a device driver because it cannot operate without >>>> I/O ports, this will be raised by the driver inside the guest. >>> On what basis would the driver complain? The kernel might know of >>> the MMIO equivalent for ports, and hence might allow the driver >>> to properly obtain whatever is needed to later access the ports. >>> Just that the port accesses then wouldn't work (possibly crashing >>> the guest, or making it otherwise misbehave). >> As ECAM and Arm does not support I/O ports, a driver requesting access >> to them would get an error back. >> So in practice it is not possible to try to access the ioports as there is no >> way on arm to use them (no instructions). >> >> A driver could misbehave by ignoring the fact that ioports are not there but >> I am not quite sure how we could solve that as it would be a bug in the driver. > The minimal thing I'd suggest (or maybe you're doing this already) > would be to expose such BARs to the guest as r/o zero, rather than > letting their port nature "shine through". If we have the same, but baremetal then which entity disallows those BARs to shine? I mean that if guest wants to crash... why should we stop it and try emulating something special for it? > > Jan >
Hi Jan, > On 12 Oct 2021, at 10:32, Jan Beulich <jbeulich@suse.com> wrote: > > On 12.10.2021 10:41, Bertrand Marquis wrote: >> Hi Jan, >> >>> On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 11.10.2021 19:11, Bertrand Marquis wrote: >>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>> I think the commit message needs to at least be expanded in order to >>>>>>> contain the information provided here. It might also be helpful to >>>>>>> figure out whether we would have to handle IO port accesses in the >>>>>>> future on Arm, or if it's fine to just ignore them. >>>>>> >>>>>> All our investigations and tests have been done without supporting it >>>>>> without any issues so this is not a critical feature (most devices can >>>>>> be operated without using the I/O ports). >>>>> >>>>> IMO we should let the users know they attempted to use a device with >>>>> BARs in the IO space, and that those BARs won't be accessible which >>>>> could make the device not function as expected. >>>>> >>>>> Do you think it would be reasonable to attempt the hypercall on Arm >>>>> also, and in case of error (on Arm) just print a warning message and >>>>> continue operations as normal? >>>> >>>> I think this would lead to a warning printed on lots of devices where in >>>> fact there would be no issues. >>>> >>>> If this is an issue for a device driver because it cannot operate without >>>> I/O ports, this will be raised by the driver inside the guest. >>> >>> On what basis would the driver complain? The kernel might know of >>> the MMIO equivalent for ports, and hence might allow the driver >>> to properly obtain whatever is needed to later access the ports. >>> Just that the port accesses then wouldn't work (possibly crashing >>> the guest, or making it otherwise misbehave). >> >> As ECAM and Arm does not support I/O ports, a driver requesting access >> to them would get an error back. >> So in practice it is not possible to try to access the ioports as there is no >> way on arm to use them (no instructions). >> >> A driver could misbehave by ignoring the fact that ioports are not there but >> I am not quite sure how we could solve that as it would be a bug in the driver. > > The minimal thing I'd suggest (or maybe you're doing this already) > would be to expose such BARs to the guest as r/o zero, rather than > letting their port nature "shine through". We are emulating an ECAM PCI which does not support I/O ports so I do not think we are (and can) expose those to guests. Anyway I will mark this as a point to check for Rahul when he is back. Cheers Bertrand > > Jan
On 12.10.2021 11:38, Oleksandr Andrushchenko wrote: > On 12.10.21 12:32, Jan Beulich wrote: >> On 12.10.2021 10:41, Bertrand Marquis wrote: >>>> On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 11.10.2021 19:11, Bertrand Marquis wrote: >>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>>> I think the commit message needs to at least be expanded in order to >>>>>>>> contain the information provided here. It might also be helpful to >>>>>>>> figure out whether we would have to handle IO port accesses in the >>>>>>>> future on Arm, or if it's fine to just ignore them. >>>>>>> All our investigations and tests have been done without supporting it >>>>>>> without any issues so this is not a critical feature (most devices can >>>>>>> be operated without using the I/O ports). >>>>>> IMO we should let the users know they attempted to use a device with >>>>>> BARs in the IO space, and that those BARs won't be accessible which >>>>>> could make the device not function as expected. >>>>>> >>>>>> Do you think it would be reasonable to attempt the hypercall on Arm >>>>>> also, and in case of error (on Arm) just print a warning message and >>>>>> continue operations as normal? >>>>> I think this would lead to a warning printed on lots of devices where in >>>>> fact there would be no issues. >>>>> >>>>> If this is an issue for a device driver because it cannot operate without >>>>> I/O ports, this will be raised by the driver inside the guest. >>>> On what basis would the driver complain? The kernel might know of >>>> the MMIO equivalent for ports, and hence might allow the driver >>>> to properly obtain whatever is needed to later access the ports. >>>> Just that the port accesses then wouldn't work (possibly crashing >>>> the guest, or making it otherwise misbehave). >>> As ECAM and Arm does not support I/O ports, a driver requesting access >>> to them would get an error back. >>> So in practice it is not possible to try to access the ioports as there is no >>> way on arm to use them (no instructions). >>> >>> A driver could misbehave by ignoring the fact that ioports are not there but >>> I am not quite sure how we could solve that as it would be a bug in the driver. >> The minimal thing I'd suggest (or maybe you're doing this already) >> would be to expose such BARs to the guest as r/o zero, rather than >> letting their port nature "shine through". > If we have the same, but baremetal then which entity disallows > those BARs to shine? I'm sorry, but I don't understand what you're trying to say. > I mean that if guest wants to crash... why > should we stop it and try emulating something special for it? This isn't about a guest "wanting to crash", but a driver potentially getting mislead into thinking that it can driver a device a certain way. Jan
On 12.10.2021 11:40, Bertrand Marquis wrote: > Hi Jan, > >> On 12 Oct 2021, at 10:32, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 12.10.2021 10:41, Bertrand Marquis wrote: >>> Hi Jan, >>> >>>> On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 11.10.2021 19:11, Bertrand Marquis wrote: >>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>>> I think the commit message needs to at least be expanded in order to >>>>>>>> contain the information provided here. It might also be helpful to >>>>>>>> figure out whether we would have to handle IO port accesses in the >>>>>>>> future on Arm, or if it's fine to just ignore them. >>>>>>> >>>>>>> All our investigations and tests have been done without supporting it >>>>>>> without any issues so this is not a critical feature (most devices can >>>>>>> be operated without using the I/O ports). >>>>>> >>>>>> IMO we should let the users know they attempted to use a device with >>>>>> BARs in the IO space, and that those BARs won't be accessible which >>>>>> could make the device not function as expected. >>>>>> >>>>>> Do you think it would be reasonable to attempt the hypercall on Arm >>>>>> also, and in case of error (on Arm) just print a warning message and >>>>>> continue operations as normal? >>>>> >>>>> I think this would lead to a warning printed on lots of devices where in >>>>> fact there would be no issues. >>>>> >>>>> If this is an issue for a device driver because it cannot operate without >>>>> I/O ports, this will be raised by the driver inside the guest. >>>> >>>> On what basis would the driver complain? The kernel might know of >>>> the MMIO equivalent for ports, and hence might allow the driver >>>> to properly obtain whatever is needed to later access the ports. >>>> Just that the port accesses then wouldn't work (possibly crashing >>>> the guest, or making it otherwise misbehave). >>> >>> As ECAM and Arm does not support I/O ports, a driver requesting access >>> to them would get an error back. >>> So in practice it is not possible to try to access the ioports as there is no >>> way on arm to use them (no instructions). >>> >>> A driver could misbehave by ignoring the fact that ioports are not there but >>> I am not quite sure how we could solve that as it would be a bug in the driver. >> >> The minimal thing I'd suggest (or maybe you're doing this already) >> would be to expose such BARs to the guest as r/o zero, rather than >> letting their port nature "shine through". > > We are emulating an ECAM PCI which does not support I/O ports so I do not > think we are (and can) expose those to guests. I'm afraid I don't follow: What do you mean by "expose to guests" in relation to what I've said? I did specifically suggest to hide the real nature of such BARs, and instead surface them such that the guest would have to conclude the BAR is actually not in use at all. Jan
On 12.10.21 13:01, Jan Beulich wrote: > On 12.10.2021 11:38, Oleksandr Andrushchenko wrote: >> On 12.10.21 12:32, Jan Beulich wrote: >>> On 12.10.2021 10:41, Bertrand Marquis wrote: >>>>> On 12 Oct 2021, at 09:29, Jan Beulich <jbeulich@suse.com> wrote: >>>>> On 11.10.2021 19:11, Bertrand Marquis wrote: >>>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote: >>>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <roger.pau@citrix.com> wrote: >>>>>>>>> I think the commit message needs to at least be expanded in order to >>>>>>>>> contain the information provided here. It might also be helpful to >>>>>>>>> figure out whether we would have to handle IO port accesses in the >>>>>>>>> future on Arm, or if it's fine to just ignore them. >>>>>>>> All our investigations and tests have been done without supporting it >>>>>>>> without any issues so this is not a critical feature (most devices can >>>>>>>> be operated without using the I/O ports). >>>>>>> IMO we should let the users know they attempted to use a device with >>>>>>> BARs in the IO space, and that those BARs won't be accessible which >>>>>>> could make the device not function as expected. >>>>>>> >>>>>>> Do you think it would be reasonable to attempt the hypercall on Arm >>>>>>> also, and in case of error (on Arm) just print a warning message and >>>>>>> continue operations as normal? >>>>>> I think this would lead to a warning printed on lots of devices where in >>>>>> fact there would be no issues. >>>>>> >>>>>> If this is an issue for a device driver because it cannot operate without >>>>>> I/O ports, this will be raised by the driver inside the guest. >>>>> On what basis would the driver complain? The kernel might know of >>>>> the MMIO equivalent for ports, and hence might allow the driver >>>>> to properly obtain whatever is needed to later access the ports. >>>>> Just that the port accesses then wouldn't work (possibly crashing >>>>> the guest, or making it otherwise misbehave). >>>> As ECAM and Arm does not support I/O ports, a driver requesting access >>>> to them would get an error back. >>>> So in practice it is not possible to try to access the ioports as there is no >>>> way on arm to use them (no instructions). >>>> >>>> A driver could misbehave by ignoring the fact that ioports are not there but >>>> I am not quite sure how we could solve that as it would be a bug in the driver. >>> The minimal thing I'd suggest (or maybe you're doing this already) >>> would be to expose such BARs to the guest as r/o zero, rather than >>> letting their port nature "shine through". >> If we have the same, but baremetal then which entity disallows >> those BARs to shine? > I'm sorry, but I don't understand what you're trying to say. > >> I mean that if guest wants to crash... why >> should we stop it and try emulating something special for it? > This isn't about a guest "wanting to crash", but a driver potentially > getting mislead into thinking that it can driver a device a certain > way. Well, for the guest, as we do not advertise IO in the emulated host bridge, the driver won't be able to allocate any IO at all. Thus, even if we have a BAR with PCI_BASE_ADDRESS_SPACE_IO bit set, the driver won't get anything. So, I think this is equivalent to a baremetal use-case where we have no IO supported by the host bridge and a device with IO BAR. > > Jan >
On 12.10.2021 12:06, Oleksandr Andrushchenko wrote: > On 12.10.21 13:01, Jan Beulich wrote: >> On 12.10.2021 11:38, Oleksandr Andrushchenko wrote: >>> On 12.10.21 12:32, Jan Beulich wrote: >>>> The minimal thing I'd suggest (or maybe you're doing this already) >>>> would be to expose such BARs to the guest as r/o zero, rather than >>>> letting their port nature "shine through". >>> If we have the same, but baremetal then which entity disallows >>> those BARs to shine? >> I'm sorry, but I don't understand what you're trying to say. >> >>> I mean that if guest wants to crash... why >>> should we stop it and try emulating something special for it? >> This isn't about a guest "wanting to crash", but a driver potentially >> getting mislead into thinking that it can driver a device a certain >> way. > Well, for the guest, as we do not advertise IO in the emulated host > bridge, the driver won't be able to allocate any IO at all. Thus, even > if we have a BAR with PCI_BASE_ADDRESS_SPACE_IO bit set, the > driver won't get anything. So, I think this is equivalent to a baremetal > use-case where we have no IO supported by the host bridge and > a device with IO BAR. Oh, now I follow. Fair enough. Jan
Hi Jan, > On 12 Oct 2021, at 11:20, Jan Beulich <jbeulich@suse.com> wrote: > > On 12.10.2021 12:06, Oleksandr Andrushchenko wrote: >> On 12.10.21 13:01, Jan Beulich wrote: >>> On 12.10.2021 11:38, Oleksandr Andrushchenko wrote: >>>> On 12.10.21 12:32, Jan Beulich wrote: >>>>> The minimal thing I'd suggest (or maybe you're doing this already) >>>>> would be to expose such BARs to the guest as r/o zero, rather than >>>>> letting their port nature "shine through". >>>> If we have the same, but baremetal then which entity disallows >>>> those BARs to shine? >>> I'm sorry, but I don't understand what you're trying to say. >>> >>>> I mean that if guest wants to crash... why >>>> should we stop it and try emulating something special for it? >>> This isn't about a guest "wanting to crash", but a driver potentially >>> getting mislead into thinking that it can driver a device a certain >>> way. >> Well, for the guest, as we do not advertise IO in the emulated host >> bridge, the driver won't be able to allocate any IO at all. Thus, even >> if we have a BAR with PCI_BASE_ADDRESS_SPACE_IO bit set, the >> driver won't get anything. So, I think this is equivalent to a baremetal >> use-case where we have no IO supported by the host bridge and >> a device with IO BAR. > > Oh, now I follow. Fair enough. So there is no comment remaining on this patch ? Would it possible to get an ack on it ? Thanks Bertrand > > Jan
On 12.10.2021 12:41, Bertrand Marquis wrote: > So there is no comment remaining on this patch ? No, but the comments weren't on the patch anyway, but on a question raised in the course of the discussion. > Would it possible to get an ack on it ? No, as I can't ack this patch: I'm not a maintainer of the file being changed. And I don't feel qualified to give an R-b. Jan
Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > So there is no comment remaining on this patch ? I have been following this thread. I think I have a notion of why this is needed but I'm not 100% clear on it. The commit message says this: > ARM architecture does not implement I/O ports. Ignore this call on ARM > to avoid the overhead of making a hypercall just for Xen to return > -ENOSYS. which implies it's a performance improvement. But the change also suppresses an error return, so this commit message is false. I think that the thread has concluded something different, but it should be explained in the commit message. The purpose of a commit message is precisely to capture the kind of considerations and discussion that occurred in this thread. If the overall outcome implied by this patch is correct (as I *think* the thread has concluded) then I don't think the #ifdefery is appropriate. This should be done with a new arch-specific function in libxl_x86.c and libxl_arm.c. I'm not sure precisely what that function should be called, but maybe something like libxl_ioports_supported ? I see that the fact that we avoid #ifdefs wasn't documented in CODING_STYLE, so I have sent a patch to add that. Sorry about that. Thanks, Ian.
Hi Ian, > On 12 Oct 2021, at 15:53, Ian Jackson <iwj@xenproject.org> wrote: > > Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): >> So there is no comment remaining on this patch ? > > I have been following this thread. I think I have a notion of why > this is needed but I'm not 100% clear on it. The commit message says > this: > >> ARM architecture does not implement I/O ports. Ignore this call on ARM >> to avoid the overhead of making a hypercall just for Xen to return >> -ENOSYS. > > which implies it's a performance improvement. But the change also > suppresses an error return, so this commit message is false. I think > that the thread has concluded something different, but it should be > explained in the commit message. The purpose of a commit message is > precisely to capture the kind of considerations and discussion that > occurred in this thread. I can add something in the commit message about the fact that we improve performance and prevent to do a call that is and will not be supported in Xen. > > If the overall outcome implied by this patch is correct (as I *think* > the thread has concluded) then I don't think the #ifdefery is > appropriate. This should be done with a new arch-specific function in > libxl_x86.c and libxl_arm.c. I'm not sure precisely what that > function should be called, but maybe something like > libxl_ioports_supported > ? > > I see that the fact that we avoid #ifdefs wasn't documented in > CODING_STYLE, so I have sent a patch to add that. Sorry about that. I saw your change in CODING_STYLE and I understand the request. I will try to see if we can handle this change before the feature freeze. Regards Bertrand > > Thanks, > Ian.
Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > I can add something in the commit message about the fact that we improve > performance and prevent to do a call that is and will not be supported in Xen. Thanks but I'm afraid I don't think that is a correct summary of the thread. Nor would it be an adequate justification for the change. At least, not unless you plan to write something considerably longer (and more precise). Firstly, I'm not convinced this change would be justified by the performance impact. This is a small number of hypercalls during domain startup. Usually none, I think ? If someone wants to optimise domain startup speed then I am very open to that but I think this change will make negligible change in practice. Unless someone wants to tell me I'm wrong about that ? And if I am wrong about that then an explanation of why my suppositions are wrong ought to go in the commit message. Secondly, there is no justification there for the change in error status. Why is this change needed ? (What goes wrong if it is omitted ?) That is what the commit message ought to answer. Plus, given that it stubs out a function to make it into a no-op, that itself requires an explanation. Why is it OK for this function which is supposed to do a thing, to in fact not do anything at all and return successfully saying "yes I did that" ? I think (having read the thread) that I know the answers to these questions but it needs to be clearly and explicitly written down. > I saw your change in CODING_STYLE and I understand the request. > I will try to see if we can handle this change before the feature freeze. Thanks. I doubt that this will be hard. I am more worried about the commit message. Indeed, since we haven't had the rationale for this change explicitly written down, there is a risk that when we do so, we will discover some problem with the approach that we had previously overlooked. Discovering that kind of thing is one reason to explicitly write down why we are doing what we are doing, but this situation does mean we shouldn't feel we've yet achieved confidence that this patch is right. Sorry, Ian.
On Tue, 12 Oct 2021, Ian Jackson wrote: > Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > > I can add something in the commit message about the fact that we improve > > performance and prevent to do a call that is and will not be supported in Xen. > > Thanks but I'm afraid I don't think that is a correct summary of the > thread. Nor would it be an adequate justification for the change. At > least, not unless you plan to write something considerably longer (and > more precise). > > Firstly, I'm not convinced this change would be justified by the > performance impact. This is a small number of hypercalls during > domain startup. Usually none, I think ? If someone wants to optimise > domain startup speed then I am very open to that but I think this > change will make negligible change in practice. Unless someone wants > to tell me I'm wrong about that ? And if I am wrong about that then > an explanation of why my suppositions are wrong ought to go in the > commit message. > > Secondly, there is no justification there for the change in error > status. > > Why is this change needed ? (What goes wrong if it is omitted ?) > That is what the commit message ought to answer. > > Plus, given that it stubs out a function to make it into a no-op, that > itself requires an explanation. Why is it OK for this function which > is supposed to do a thing, to in fact not do anything at all and > return successfully saying "yes I did that" ? > > I think (having read the thread) that I know the answers to these > questions but it needs to be clearly and explicitly written down. > > > I saw your change in CODING_STYLE and I understand the request. > > I will try to see if we can handle this change before the feature freeze. > > Thanks. I doubt that this will be hard. I am more worried about the > commit message. > > Indeed, since we haven't had the rationale for this change explicitly > written down, there is a risk that when we do so, we will discover > some problem with the approach that we had previously overlooked. > > Discovering that kind of thing is one reason to explicitly write down > why we are doing what we are doing, but this situation does mean we > shouldn't feel we've yet achieved confidence that this patch is right. I don't think it is about performance. From a performance point of view, we could make as many (unneeded) hypercalls as required. It is mostly about minimizing unwanted changes to common libxl code. Let me explain. IO ports on ARM don't exist so all IO ports related hypercalls are going to fail. This is expected. Today, a failure of xc_domain_ioport_permission would turn into a critical failure at domain creation. We need to avoid this outcome; instead we want to continue with domain creation as normal even if xc_domain_ioport_permission fails. (FYI the underlying hypercall XEN_DOMCTL_ioport_permission is not implemented on ARM so it would return -ENOSYS.) We have a few options to achieve this goal: 1) No xc_domain_ioport_permission calls on ARM Use #ifdefs or similar checks in libxl_pci.c to avoid calling xc_domain_ioport_permission on ARM. This could be best but it would cause some churn in arch-neutral libxl code. 2) Handle xc_domain_ioport_permission errors in libxl Introduce checks on the return value of xc_domain_ioport_permission and ignore specific errors on ARM in libxl_pci.c. For instance: if (ARM && rc == -ENOSYS) continue. This might cause less churn than 1) but still requires a few changes in arch-neutral libxl code. 3) Force XEN_DOMCTL_ioport_permission to return zero on ARM Force the hypercall to return success even if it did nothing. Currently it returns -ENOSYS. This is possible but it wasn't chosen for the implementation as we felt that the hypercall should reflect what was actually done (nothing) and it should be userspace to handle the error. I guess this could be argued either way. 4) Force xc_domain_ioport_permission to return zero on ARM Force xc_domain_ioport_permission to return success even if the hypercall would return -ENOSYS. This way there are no changes to libxl. This is what the patch currently implements by using #ifdef in xc_domain_ioport_permission. It could also have achieved the same goal by making the implementation of xc_domain_ioport_permission arch-specific, and in the ARM implementation returning 0. All options above achieve the goal of a successful domain creation with PCI device assigned on ARM. You might be able to think of other options as well. I think noone here is really set on using one option over the other -- as long as xc_domain_ioport_permission failures don't turn into domain creation failures on ARM we are good. Let us know what you think.
On Tue, Oct 12, 2021 at 04:15:20PM +0000, Bertrand Marquis wrote: > Hi Ian, > > > On 12 Oct 2021, at 15:53, Ian Jackson <iwj@xenproject.org> wrote: > > > > Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > >> So there is no comment remaining on this patch ? > > > > I have been following this thread. I think I have a notion of why > > this is needed but I'm not 100% clear on it. The commit message says > > this: > > > >> ARM architecture does not implement I/O ports. Ignore this call on ARM > >> to avoid the overhead of making a hypercall just for Xen to return > >> -ENOSYS. > > > > which implies it's a performance improvement. But the change also > > suppresses an error return, so this commit message is false. I think > > that the thread has concluded something different, but it should be > > explained in the commit message. The purpose of a commit message is > > precisely to capture the kind of considerations and discussion that > > occurred in this thread. > > I can add something in the commit message about the fact that we improve > performance and prevent to do a call that is and will not be supported in Xen. IMO it would be good to modify the commit message so it covers the fact that the emulated host bridge on Arm does not advertise IO port support, so the guest is capable of realizing IO BARs are not supported. Otherwise it seems like the toolstack is ignoring a failure which could cause a device to malfunction when passed though (which is still the case, but the guest will be able to notice). Thanks, Roger.
On Tue, Oct 12, 2021 at 01:42:22PM -0700, Stefano Stabellini wrote: > On Tue, 12 Oct 2021, Ian Jackson wrote: > > Bertrand Marquis writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > > > I can add something in the commit message about the fact that we improve > > > performance and prevent to do a call that is and will not be supported in Xen. > > > > Thanks but I'm afraid I don't think that is a correct summary of the > > thread. Nor would it be an adequate justification for the change. At > > least, not unless you plan to write something considerably longer (and > > more precise). > > > > Firstly, I'm not convinced this change would be justified by the > > performance impact. This is a small number of hypercalls during > > domain startup. Usually none, I think ? If someone wants to optimise > > domain startup speed then I am very open to that but I think this > > change will make negligible change in practice. Unless someone wants > > to tell me I'm wrong about that ? And if I am wrong about that then > > an explanation of why my suppositions are wrong ought to go in the > > commit message. > > > > Secondly, there is no justification there for the change in error > > status. > > > > Why is this change needed ? (What goes wrong if it is omitted ?) > > That is what the commit message ought to answer. > > > > Plus, given that it stubs out a function to make it into a no-op, that > > itself requires an explanation. Why is it OK for this function which > > is supposed to do a thing, to in fact not do anything at all and > > return successfully saying "yes I did that" ? > > > > I think (having read the thread) that I know the answers to these > > questions but it needs to be clearly and explicitly written down. > > > > > I saw your change in CODING_STYLE and I understand the request. > > > I will try to see if we can handle this change before the feature freeze. > > > > Thanks. I doubt that this will be hard. I am more worried about the > > commit message. > > > > Indeed, since we haven't had the rationale for this change explicitly > > written down, there is a risk that when we do so, we will discover > > some problem with the approach that we had previously overlooked. > > > > Discovering that kind of thing is one reason to explicitly write down > > why we are doing what we are doing, but this situation does mean we > > shouldn't feel we've yet achieved confidence that this patch is right. > > > I don't think it is about performance. From a performance point of view, > we could make as many (unneeded) hypercalls as required. It is mostly > about minimizing unwanted changes to common libxl code. Let me explain. > > > IO ports on ARM don't exist so all IO ports related hypercalls are going > to fail. This is expected. Today, a failure of > xc_domain_ioport_permission would turn into a critical failure at domain > creation. We need to avoid this outcome; instead we want to continue > with domain creation as normal even if xc_domain_ioport_permission > fails. (FYI the underlying hypercall XEN_DOMCTL_ioport_permission is not > implemented on ARM so it would return -ENOSYS.) > > > We have a few options to achieve this goal: > > > 1) No xc_domain_ioport_permission calls on ARM > > Use #ifdefs or similar checks in libxl_pci.c to avoid calling > xc_domain_ioport_permission on ARM. This could be best but it would > cause some churn in arch-neutral libxl code. > > > 2) Handle xc_domain_ioport_permission errors in libxl > > Introduce checks on the return value of xc_domain_ioport_permission > and ignore specific errors on ARM in libxl_pci.c. > For instance: if (ARM && rc == -ENOSYS) continue. > > This might cause less churn than 1) but still requires a few changes > in arch-neutral libxl code. > > > 3) Force XEN_DOMCTL_ioport_permission to return zero on ARM > > Force the hypercall to return success even if it did nothing. > Currently it returns -ENOSYS. > > This is possible but it wasn't chosen for the implementation as we > felt that the hypercall should reflect what was actually done > (nothing) and it should be userspace to handle the error. I guess > this could be argued either way. > > > 4) Force xc_domain_ioport_permission to return zero on ARM > > Force xc_domain_ioport_permission to return success even if the > hypercall would return -ENOSYS. This way there are no changes to > libxl. > > This is what the patch currently implements by using #ifdef in > xc_domain_ioport_permission. It could also have achieved the same > goal by making the implementation of xc_domain_ioport_permission > arch-specific, and in the ARM implementation returning 0. > > > All options above achieve the goal of a successful domain creation with > PCI device assigned on ARM. You might be able to think of other options > as well. I think noone here is really set on using one option over the > other -- as long as xc_domain_ioport_permission failures don't turn into > domain creation failures on ARM we are good. > I think having a libxl_arch_io_ports_supported helper could be the cleaner way to do this. For x86 it will unconditionally return true, while for Arm you could consider poking at XEN_DOMCTL_ioport_permission and see if it returns ENOSYS or otherwise. I guess it's possible that in the future we allow IO ports access on Arm guests using some kind of emulated mechanism if the need arises, at which point the hypercall will be implemented. Thanks, Roger.
Roger Pau Monné writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > On Tue, Oct 12, 2021 at 01:42:22PM -0700, Stefano Stabellini wrote: > > I don't think it is about performance. From a performance point of view, > > we could make as many (unneeded) hypercalls as required. It is mostly > > about minimizing unwanted changes to common libxl code. Let me explain. Thanks. This summary is helpful And, pleasingly, it matches what I had thought I had gleaned from the thread. > > All options above achieve the goal of a successful domain creation with > > PCI device assigned on ARM. You might be able to think of other options > > as well. I think noone here is really set on using one option over the > > other -- as long as xc_domain_ioport_permission failures don't turn into > > domain creation failures on ARM we are good. > > I think having a libxl_arch_io_ports_supported helper could be the > cleaner way to do this. For x86 it will unconditionally return true, > while for Arm you could consider poking at > XEN_DOMCTL_ioport_permission and see if it returns ENOSYS or > otherwise. > I guess it's possible that in the future we allow IO ports access on > Arm guests using some kind of emulated mechanism if the need arises, > at which point the hypercall will be implemented. I agree with Roger. So I think I would like to see a version of this patch which * Introduces libxl_arch_io_ports_supported. (I am fine with it just returning false, unconditionally on Arm, ie in libxl_arm.c.) * Has a commit message explaining what is actually going on. Cutting and pasting liberally from your email seems like it would be a very good starting point. Even discussion of rejected alternatives is fine, if it seems like it fits. I'm quite unlikely to object to a commit message on grounds that it's too long. Thanks, Ian.
Roger Pau Monné writes ("Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM."): > IMO it would be good to modify the commit message so it covers the > fact that the emulated host bridge on Arm does not advertise IO port > support, so the guest is capable of realizing IO BARs are not > supported. Yes. > Otherwise it seems like the toolstack is ignoring a failure which > could cause a device to malfunction when passed though (which is still > the case, but the guest will be able to notice). Quite. Thanks, Ian.
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c index 23322b70b5..25c95f6596 100644 --- a/tools/libs/ctrl/xc_domain.c +++ b/tools/libs/ctrl/xc_domain.c @@ -1348,6 +1348,14 @@ int xc_domain_ioport_permission(xc_interface *xch, uint32_t nr_ports, uint32_t allow_access) { +#if defined(__arm__) || defined(__aarch64__) + /* + * The ARM architecture does not implement I/O ports. + * Avoid the overhead of making a hypercall just for Xen to return -ENOSYS. + * It is safe to ignore this call on ARM so we just return 0. + */ + return 0; +#else DECLARE_DOMCTL; domctl.cmd = XEN_DOMCTL_ioport_permission; @@ -1357,6 +1365,7 @@ int xc_domain_ioport_permission(xc_interface *xch, domctl.u.ioport_permission.allow_access = allow_access; return do_domctl(xch, &domctl); +#endif } int xc_availheap(xc_interface *xch,