[v5,01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.

Commit Message

Rahul Singh Oct. 6, 2021, 5:40 p.m. UTC

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.

Signed-off-by: Rahul Singh <[e-mail]>
Reviewed-by: Stefano Stabellini <[e-mail]>
Reviewed-by: Bertrand Marquis <[e-mail]>
---
Change in v5: none
Change in v4:
- Added Reviewed-by: Bertrand Marquis <[e-mail]>
Change in v3: Added Reviewed-by: Stefano Stabellini <[e-mail]>
Change in v2:
- Instead of returning success in XEN, ignored the call in xl.
---
---
 tools/libs/ctrl/xc_domain.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Roger Pau Monné Oct. 11, 2021, 11:47 a.m. UTC | #1

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.

Bertrand Marquis Oct. 11, 2021, 12:11 p.m. UTC | #2

Hi Roger,

> On 11 Oct 2021, at 12:47, Roger Pau Monné <[e-mail]> 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.

Roger Pau Monné Oct. 11, 2021, 1:20 p.m. UTC | #3

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é <[e-mail]> 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.

Bertrand Marquis Oct. 11, 2021, 1:40 p.m. UTC | #4

Hi Roger,

+ Oleksandr to have a better PCI expert then me.

> On 11 Oct 2021, at 14:20, Roger Pau Monné <[e-mail]> 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é <[e-mail]> 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.

Roger Pau Monné Oct. 11, 2021, 1:57 p.m. UTC | #5

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é <[e-mail]> 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é <[e-mail]> 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.

Oleksandr Andrushchenko Oct. 11, 2021, 2:16 p.m. UTC | #6

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é <[e-mail]> 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é <[e-mail]> 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.

Bertrand Marquis Oct. 11, 2021, 2:16 p.m. UTC | #7

Hi Roger,

> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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é <[e-mail]> 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é <[e-mail]> 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.

Roger Pau Monné Oct. 11, 2021, 4:32 p.m. UTC | #8

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é <[e-mail]> 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é <[e-mail]> 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é <[e-mail]> 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.

Bertrand Marquis Oct. 11, 2021, 5:11 p.m. UTC | #9

Hi Roger,

> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> 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é <[e-mail]> 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é <[e-mail]> 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é <[e-mail]> 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.

Jan Beulich Oct. 12, 2021, 8:29 a.m. UTC | #10

On 11.10.2021 19:11, Bertrand Marquis wrote:
>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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

Bertrand Marquis Oct. 12, 2021, 8:41 a.m. UTC | #11

Hi Jan,

> On 12 Oct 2021, at 09:29, Jan Beulich <[e-mail]> wrote:
> 
> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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
>

Jan Beulich Oct. 12, 2021, 9:32 a.m. UTC | #12

On 12.10.2021 10:41, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 12 Oct 2021, at 09:29, Jan Beulich <[e-mail]> wrote:
>>
>> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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

Oleksandr Andrushchenko Oct. 12, 2021, 9:38 a.m. UTC | #13

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 <[e-mail]> wrote:
>>>
>>> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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
>

Bertrand Marquis Oct. 12, 2021, 9:40 a.m. UTC | #14

Hi Jan,

> On 12 Oct 2021, at 10:32, Jan Beulich <[e-mail]> wrote:
> 
> On 12.10.2021 10:41, Bertrand Marquis wrote:
>> Hi Jan,
>> 
>>> On 12 Oct 2021, at 09:29, Jan Beulich <[e-mail]> wrote:
>>> 
>>> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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

Jan Beulich Oct. 12, 2021, 10:01 a.m. UTC | #15

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 <[e-mail]> wrote:
>>>> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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

Jan Beulich Oct. 12, 2021, 10:03 a.m. UTC | #16

On 12.10.2021 11:40, Bertrand Marquis wrote:
> Hi Jan,
> 
>> On 12 Oct 2021, at 10:32, Jan Beulich <[e-mail]> wrote:
>>
>> On 12.10.2021 10:41, Bertrand Marquis wrote:
>>> Hi Jan,
>>>
>>>> On 12 Oct 2021, at 09:29, Jan Beulich <[e-mail]> wrote:
>>>>
>>>> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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

Oleksandr Andrushchenko Oct. 12, 2021, 10:06 a.m. UTC | #17

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 <[e-mail]> wrote:
>>>>> On 11.10.2021 19:11, Bertrand Marquis wrote:
>>>>>>> On 11 Oct 2021, at 17:32, Roger Pau Monné <[e-mail]> wrote:
>>>>>>> On Mon, Oct 11, 2021 at 02:16:19PM +0000, Bertrand Marquis wrote:
>>>>>>>>> On 11 Oct 2021, at 14:57, Roger Pau Monné <[e-mail]> 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
>

Jan Beulich Oct. 12, 2021, 10:20 a.m. UTC | #18

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

Bertrand Marquis Oct. 12, 2021, 10:41 a.m. UTC | #19

Hi Jan,

> On 12 Oct 2021, at 11:20, Jan Beulich <[e-mail]> 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

Jan Beulich Oct. 12, 2021, 10:44 a.m. UTC | #20

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

Ian Jackson Oct. 12, 2021, 2:53 p.m. UTC | #21

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.

Bertrand Marquis Oct. 12, 2021, 4:15 p.m. UTC | #22

Hi Ian,

> On 12 Oct 2021, at 15:53, Ian Jackson <[e-mail]> 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.

Ian Jackson Oct. 12, 2021, 4:29 p.m. UTC | #23

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.

Stefano Stabellini Oct. 12, 2021, 8:42 p.m. UTC | #24

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.

Roger Pau Monné Oct. 13, 2021, 8:02 a.m. UTC | #25

On Tue, Oct 12, 2021 at 04:15:20PM +0000, Bertrand Marquis wrote:
> Hi Ian,
> 
> > On 12 Oct 2021, at 15:53, Ian Jackson <[e-mail]> 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.

Roger Pau Monné Oct. 13, 2021, 8:07 a.m. UTC | #26

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.

Ian Jackson Oct. 13, 2021, 11:52 a.m. UTC | #27

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.

Ian Jackson Oct. 13, 2021, 12:02 p.m. UTC | #28

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.

Patch

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,