Restrict CPU I/O instructions to architectures providing
I/O bus.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
tests/qtest/libqos/fw_cfg.h | 3 +++
tests/qtest/libqos/fw_cfg.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
index c6a7cf8cf05..3bfb6d6d55b 100644
--- a/tests/qtest/libqos/fw_cfg.h
+++ b/tests/qtest/libqos/fw_cfg.h
@@ -36,6 +36,8 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
+
+#ifdef TARGET_HAS_IOPORT
QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
void io_fw_cfg_uninit(QFWCFG *fw_cfg);
@@ -48,6 +50,7 @@ static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
{
io_fw_cfg_uninit(fw_cfg);
}
+#endif /* TARGET_HAS_IOPORT */
G_DEFINE_AUTOPTR_CLEANUP_FUNC(QFWCFG, mm_fw_cfg_uninit)
diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
index 6b8e1babe51..db2b83f5212 100644
--- a/tests/qtest/libqos/fw_cfg.c
+++ b/tests/qtest/libqos/fw_cfg.c
@@ -131,6 +131,7 @@ void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
g_free(fw_cfg);
}
+#ifdef TARGET_HAS_IOPORT
static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
{
qtest_outw(fw_cfg->qts, fw_cfg->base, key);
@@ -162,3 +163,4 @@ void io_fw_cfg_uninit(QFWCFG *fw_cfg)
{
g_free(fw_cfg);
}
+#endif /* TARGET_HAS_IOPORT */
--
2.26.2
(+Peter, comment below)
On 03/15/21 00:29, Philippe Mathieu-Daudé wrote:
> Restrict CPU I/O instructions to architectures providing
> I/O bus.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> tests/qtest/libqos/fw_cfg.h | 3 +++
> tests/qtest/libqos/fw_cfg.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
> index c6a7cf8cf05..3bfb6d6d55b 100644
> --- a/tests/qtest/libqos/fw_cfg.h
> +++ b/tests/qtest/libqos/fw_cfg.h
> @@ -36,6 +36,8 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
>
> QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
> void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
> +
> +#ifdef TARGET_HAS_IOPORT
> QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
> void io_fw_cfg_uninit(QFWCFG *fw_cfg);
>
> @@ -48,6 +50,7 @@ static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
> {
> io_fw_cfg_uninit(fw_cfg);
> }
> +#endif /* TARGET_HAS_IOPORT */
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(QFWCFG, mm_fw_cfg_uninit)
>
> diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
> index 6b8e1babe51..db2b83f5212 100644
> --- a/tests/qtest/libqos/fw_cfg.c
> +++ b/tests/qtest/libqos/fw_cfg.c
> @@ -131,6 +131,7 @@ void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
> g_free(fw_cfg);
> }
>
> +#ifdef TARGET_HAS_IOPORT
> static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
> {
> qtest_outw(fw_cfg->qts, fw_cfg->base, key);
> @@ -162,3 +163,4 @@ void io_fw_cfg_uninit(QFWCFG *fw_cfg)
> {
> g_free(fw_cfg);
> }
> +#endif /* TARGET_HAS_IOPORT */
>
I'm not sure the macro name is ideal; the PCI host on aarch64/"virt"
emulates IO Ports (it's possible to allocate PCI IO resources on
"virt"). From patch#3, TARGET_HAS_IOPORT does not seem to extend to arm64.
I guess the intent is OK in both patches #3 and #5.
Thanks
Laszlo
On 3/16/21 9:37 AM, Laszlo Ersek wrote:
> (+Peter, comment below)
>
> On 03/15/21 00:29, Philippe Mathieu-Daudé wrote:
>> Restrict CPU I/O instructions to architectures providing
>> I/O bus.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> tests/qtest/libqos/fw_cfg.h | 3 +++
>> tests/qtest/libqos/fw_cfg.c | 2 ++
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
>> index c6a7cf8cf05..3bfb6d6d55b 100644
>> --- a/tests/qtest/libqos/fw_cfg.h
>> +++ b/tests/qtest/libqos/fw_cfg.h
>> @@ -36,6 +36,8 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
>>
>> QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
>> void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
>> +
>> +#ifdef TARGET_HAS_IOPORT
>> QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
>> void io_fw_cfg_uninit(QFWCFG *fw_cfg);
>>
>> @@ -48,6 +50,7 @@ static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
>> {
>> io_fw_cfg_uninit(fw_cfg);
>> }
>> +#endif /* TARGET_HAS_IOPORT */
>>
>> G_DEFINE_AUTOPTR_CLEANUP_FUNC(QFWCFG, mm_fw_cfg_uninit)
>>
>> diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
>> index 6b8e1babe51..db2b83f5212 100644
>> --- a/tests/qtest/libqos/fw_cfg.c
>> +++ b/tests/qtest/libqos/fw_cfg.c
>> @@ -131,6 +131,7 @@ void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
>> g_free(fw_cfg);
>> }
>>
>> +#ifdef TARGET_HAS_IOPORT
>> static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>> {
>> qtest_outw(fw_cfg->qts, fw_cfg->base, key);
>> @@ -162,3 +163,4 @@ void io_fw_cfg_uninit(QFWCFG *fw_cfg)
>> {
>> g_free(fw_cfg);
>> }
>> +#endif /* TARGET_HAS_IOPORT */
>>
>
> I'm not sure the macro name is ideal; the PCI host on aarch64/"virt"
> emulates IO Ports (it's possible to allocate PCI IO resources on
> "virt"). From patch#3, TARGET_HAS_IOPORT does not seem to extend to arm64.
Correct, aarch64 has memory-mapped pci io resources, they are not on a separate
ioport address space as for x86 and avr.
r~
Hi Richard and Laszlo,
On 3/16/21 4:43 PM, Richard Henderson wrote:
> On 3/16/21 9:37 AM, Laszlo Ersek wrote:
>> (+Peter, comment below)
>>
>> On 03/15/21 00:29, Philippe Mathieu-Daudé wrote:
>>> Restrict CPU I/O instructions to architectures providing
>>> I/O bus.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> tests/qtest/libqos/fw_cfg.h | 3 +++
>>> tests/qtest/libqos/fw_cfg.c | 2 ++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
>>> index c6a7cf8cf05..3bfb6d6d55b 100644
>>> --- a/tests/qtest/libqos/fw_cfg.h
>>> +++ b/tests/qtest/libqos/fw_cfg.h
>>> @@ -36,6 +36,8 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
>>> *filename,
>>> QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
>>> void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
>>> +
>>> +#ifdef TARGET_HAS_IOPORT
>>> QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
>>> void io_fw_cfg_uninit(QFWCFG *fw_cfg);
>>> @@ -48,6 +50,7 @@ static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
>>> {
>>> io_fw_cfg_uninit(fw_cfg);
>>> }
>>> +#endif /* TARGET_HAS_IOPORT */
>>> G_DEFINE_AUTOPTR_CLEANUP_FUNC(QFWCFG, mm_fw_cfg_uninit)
>>> diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
>>> index 6b8e1babe51..db2b83f5212 100644
>>> --- a/tests/qtest/libqos/fw_cfg.c
>>> +++ b/tests/qtest/libqos/fw_cfg.c
>>> @@ -131,6 +131,7 @@ void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
>>> g_free(fw_cfg);
>>> }
>>> +#ifdef TARGET_HAS_IOPORT
>>> static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>>> {
>>> qtest_outw(fw_cfg->qts, fw_cfg->base, key);
>>> @@ -162,3 +163,4 @@ void io_fw_cfg_uninit(QFWCFG *fw_cfg)
>>> {
>>> g_free(fw_cfg);
>>> }
>>> +#endif /* TARGET_HAS_IOPORT */
>>>
>>
>> I'm not sure the macro name is ideal; the PCI host on aarch64/"virt"
>> emulates IO Ports (it's possible to allocate PCI IO resources on
>> "virt"). From patch#3, TARGET_HAS_IOPORT does not seem to extend to
>> arm64.
>
> Correct, aarch64 has memory-mapped pci io resources, they are not on a
> separate ioport address space as for x86 and avr.
I first wrote TARGET_CPU_HAS_IOPORT but realized architecture
and CPU are linked, so I elided _CPU_.
What I'd like to clear from the QTest API is the idea that the CPU has
direct access to the I/O bus via I/O specific instructions.
Any machine able to use a host <-> PCI bus chipset is able to access
the I/O function from the PCI bus.
The fact that on X86 the first PCI function is wired to the same I/O
bus than the CPU is a machine implementation detail.
When accessing PCI I/O ressources on Aarch64, you don't have to use
dedicated I/O instructions.
Anyway for now Thomas discarded this series, as QTest is a generic API,
and we never had to worry about mixing address spaces so far, so not in
a hurry to clean this (although it would be useful to change address
space to access DMA or secure-CPU-view from QTest).
Regards,
Phil.
On 03/16/21 16:55, Philippe Mathieu-Daudé wrote:
> Hi Richard and Laszlo,
>
> On 3/16/21 4:43 PM, Richard Henderson wrote:
>> On 3/16/21 9:37 AM, Laszlo Ersek wrote:
>>> (+Peter, comment below)
>>>
>>> On 03/15/21 00:29, Philippe Mathieu-Daudé wrote:
>>>> Restrict CPU I/O instructions to architectures providing
>>>> I/O bus.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>> tests/qtest/libqos/fw_cfg.h | 3 +++
>>>> tests/qtest/libqos/fw_cfg.c | 2 ++
>>>> 2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
>>>> index c6a7cf8cf05..3bfb6d6d55b 100644
>>>> --- a/tests/qtest/libqos/fw_cfg.h
>>>> +++ b/tests/qtest/libqos/fw_cfg.h
>>>> @@ -36,6 +36,8 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
>>>> *filename,
>>>> QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
>>>> void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
>>>> +
>>>> +#ifdef TARGET_HAS_IOPORT
>>>> QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
>>>> void io_fw_cfg_uninit(QFWCFG *fw_cfg);
>>>> @@ -48,6 +50,7 @@ static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
>>>> {
>>>> io_fw_cfg_uninit(fw_cfg);
>>>> }
>>>> +#endif /* TARGET_HAS_IOPORT */
>>>> G_DEFINE_AUTOPTR_CLEANUP_FUNC(QFWCFG, mm_fw_cfg_uninit)
>>>> diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
>>>> index 6b8e1babe51..db2b83f5212 100644
>>>> --- a/tests/qtest/libqos/fw_cfg.c
>>>> +++ b/tests/qtest/libqos/fw_cfg.c
>>>> @@ -131,6 +131,7 @@ void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
>>>> g_free(fw_cfg);
>>>> }
>>>> +#ifdef TARGET_HAS_IOPORT
>>>> static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>>>> {
>>>> qtest_outw(fw_cfg->qts, fw_cfg->base, key);
>>>> @@ -162,3 +163,4 @@ void io_fw_cfg_uninit(QFWCFG *fw_cfg)
>>>> {
>>>> g_free(fw_cfg);
>>>> }
>>>> +#endif /* TARGET_HAS_IOPORT */
>>>>
>>>
>>> I'm not sure the macro name is ideal; the PCI host on aarch64/"virt"
>>> emulates IO Ports (it's possible to allocate PCI IO resources on
>>> "virt"). From patch#3, TARGET_HAS_IOPORT does not seem to extend to
>>> arm64.
>>
>> Correct, aarch64 has memory-mapped pci io resources, they are not on a
>> separate ioport address space as for x86 and avr.
>
> I first wrote TARGET_CPU_HAS_IOPORT but realized architecture
> and CPU are linked, so I elided _CPU_.
>
> What I'd like to clear from the QTest API is the idea that the CPU has
> direct access to the I/O bus via I/O specific instructions.
>
> Any machine able to use a host <-> PCI bus chipset is able to access
> the I/O function from the PCI bus.
>
> The fact that on X86 the first PCI function is wired to the same I/O
> bus than the CPU is a machine implementation detail.
>
> When accessing PCI I/O ressources on Aarch64, you don't have to use
> dedicated I/O instructions.
>
> Anyway for now Thomas discarded this series, as QTest is a generic API,
> and we never had to worry about mixing address spaces so far, so not in
> a hurry to clean this (although it would be useful to change address
> space to access DMA or secure-CPU-view from QTest).
If this is about an "IO Bus" or "IO instructions", then we should call
the macro TARGET_HAS_IO_BUS or "TARGET_ISA_HAS_IO" (or
"TARGET_HAS_IO_INSNS"), or something like those. My only confusion was
about the "IO Port" expression in the macro name; the idea is OK from my
perspective otherwise.
Thanks
Laszlo
On 3/17/21 4:59 PM, Laszlo Ersek wrote:
> On 03/16/21 16:55, Philippe Mathieu-Daudé wrote:
>> Hi Richard and Laszlo,
>>
>> On 3/16/21 4:43 PM, Richard Henderson wrote:
>>> On 3/16/21 9:37 AM, Laszlo Ersek wrote:
>>>> (+Peter, comment below)
>>>>
>>>> On 03/15/21 00:29, Philippe Mathieu-Daudé wrote:
>>>>> Restrict CPU I/O instructions to architectures providing
>>>>> I/O bus.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>> tests/qtest/libqos/fw_cfg.h | 3 +++
>>>>> tests/qtest/libqos/fw_cfg.c | 2 ++
>>>>> 2 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/tests/qtest/libqos/fw_cfg.h b/tests/qtest/libqos/fw_cfg.h
>>>>> index c6a7cf8cf05..3bfb6d6d55b 100644
>>>>> --- a/tests/qtest/libqos/fw_cfg.h
>>>>> +++ b/tests/qtest/libqos/fw_cfg.h
>>>>> @@ -36,6 +36,8 @@ size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char
>>>>> *filename,
>>>>> QFWCFG *mm_fw_cfg_init(QTestState *qts, uint64_t base);
>>>>> void mm_fw_cfg_uninit(QFWCFG *fw_cfg);
>>>>> +
>>>>> +#ifdef TARGET_HAS_IOPORT
>>>>> QFWCFG *io_fw_cfg_init(QTestState *qts, uint16_t base);
>>>>> void io_fw_cfg_uninit(QFWCFG *fw_cfg);
>>>>> @@ -48,6 +50,7 @@ static inline void pc_fw_cfg_uninit(QFWCFG *fw_cfg)
>>>>> {
>>>>> io_fw_cfg_uninit(fw_cfg);
>>>>> }
>>>>> +#endif /* TARGET_HAS_IOPORT */
>>>>> G_DEFINE_AUTOPTR_CLEANUP_FUNC(QFWCFG, mm_fw_cfg_uninit)
>>>>> diff --git a/tests/qtest/libqos/fw_cfg.c b/tests/qtest/libqos/fw_cfg.c
>>>>> index 6b8e1babe51..db2b83f5212 100644
>>>>> --- a/tests/qtest/libqos/fw_cfg.c
>>>>> +++ b/tests/qtest/libqos/fw_cfg.c
>>>>> @@ -131,6 +131,7 @@ void mm_fw_cfg_uninit(QFWCFG *fw_cfg)
>>>>> g_free(fw_cfg);
>>>>> }
>>>>> +#ifdef TARGET_HAS_IOPORT
>>>>> static void io_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>>>>> {
>>>>> qtest_outw(fw_cfg->qts, fw_cfg->base, key);
>>>>> @@ -162,3 +163,4 @@ void io_fw_cfg_uninit(QFWCFG *fw_cfg)
>>>>> {
>>>>> g_free(fw_cfg);
>>>>> }
>>>>> +#endif /* TARGET_HAS_IOPORT */
>>>>>
>>>>
>>>> I'm not sure the macro name is ideal; the PCI host on aarch64/"virt"
>>>> emulates IO Ports (it's possible to allocate PCI IO resources on
>>>> "virt"). From patch#3, TARGET_HAS_IOPORT does not seem to extend to
>>>> arm64.
>>>
>>> Correct, aarch64 has memory-mapped pci io resources, they are not on a
>>> separate ioport address space as for x86 and avr.
>>
>> I first wrote TARGET_CPU_HAS_IOPORT but realized architecture
>> and CPU are linked, so I elided _CPU_.
>>
>> What I'd like to clear from the QTest API is the idea that the CPU has
>> direct access to the I/O bus via I/O specific instructions.
>>
>> Any machine able to use a host <-> PCI bus chipset is able to access
>> the I/O function from the PCI bus.
>>
>> The fact that on X86 the first PCI function is wired to the same I/O
>> bus than the CPU is a machine implementation detail.
>>
>> When accessing PCI I/O ressources on Aarch64, you don't have to use
>> dedicated I/O instructions.
>>
>> Anyway for now Thomas discarded this series, as QTest is a generic API,
>> and we never had to worry about mixing address spaces so far, so not in
>> a hurry to clean this (although it would be useful to change address
>> space to access DMA or secure-CPU-view from QTest).
>
> If this is about an "IO Bus" or "IO instructions", then we should call
> the macro TARGET_HAS_IO_BUS or "TARGET_ISA_HAS_IO" (or
> "TARGET_HAS_IO_INSNS"), or something like those. My only confusion was
> about the "IO Port" expression in the macro name; the idea is OK from my
> perspective otherwise.
TARGET_HAS_IO_BUS / TARGET_HAS_IO_INSNS LGTM
(ISA bus is not particularly relevant for the AVR target).
Thanks for the feedback :)
On 03/17/21 17:24, Philippe Mathieu-Daudé wrote: > On 3/17/21 4:59 PM, Laszlo Ersek wrote: >> If this is about an "IO Bus" or "IO instructions", then we should call >> the macro TARGET_HAS_IO_BUS or "TARGET_ISA_HAS_IO" (or >> "TARGET_HAS_IO_INSNS"), or something like those. My only confusion was >> about the "IO Port" expression in the macro name; the idea is OK from my >> perspective otherwise. > > TARGET_HAS_IO_BUS / TARGET_HAS_IO_INSNS LGTM > (ISA bus is not particularly relevant for the AVR target). Apologies for being unclear -- by "ISA", I meant "instruction set architecture". So "TARGET_ISA_HAS_IO" was a synonym for "TARGET_HAS_IO_INSNS" -- i.e., no IO-specific machine code instructions. > Thanks for the feedback :) My pleasure :) Laszlo
© 2016 - 2026 Red Hat, Inc.