From: andre.przywara@arm.com (Andre Przywara)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3 06/10] arm: simplify MMIO dispatching
Date: Wed, 17 Jun 2015 14:49:59 +0100 [thread overview]
Message-ID: <55817B07.8010502@arm.com> (raw)
In-Reply-To: <55816C9F.1090500@arm.com>
Hi Marc,
On 06/17/2015 01:48 PM, Marc Zyngier wrote:
> On 17/06/15 12:21, Andre Przywara wrote:
>> Currently we separate any incoming MMIO request into one of the ARM
>> memory map regions and take care to spare the GIC.
>> It turns out that this is unnecessary, as we only have one special
>> region (the IO port area in the first 64 KByte). The MMIO rbtree
>> takes care about unhandled MMIO ranges, so we can simply drop all the
>> special range checking (except that for the IO range) in
>> kvm_cpu__emulate_mmio().
>> As the GIC is handled in the kernel, a GIC MMIO access should never
>> reach userland (and we don't know what to do with it anyway).
>> This lets us delete some more code and simplifies future extensions
>> (like expanding the GIC regions).
>> To be in line with the other architectures, move the now simpler
>> code into a header file.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> arm/include/arm-common/kvm-arch.h | 12 ------------
>> arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>> arm/kvm-cpu.c | 16 ----------------
>> 3 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index 082131d..90d6733 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>> return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>> }
>>
>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>> -{
>> - u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>> - return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>> -}
>> -
>> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
>> -{
>> - u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
>> - return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
>> -}
>> -
>> struct kvm_arch {
>> /*
>> * We may have to align the guest memory for virtio, so keep the
>> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
>> index 36c7872..329979a 100644
>> --- a/arm/include/arm-common/kvm-cpu-arch.h
>> +++ b/arm/include/arm-common/kvm-cpu-arch.h
>> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>> return false;
>> }
>>
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> - u32 len, u8 is_write);
>> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
>> + u8 *data, u32 len, u8 is_write)
>> +{
>> + if (arm_addr_in_ioport_region(phys_addr)) {
>> + int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> + u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> +
>> + return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> + }
>> +
>> + return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> +}
>>
>> unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>>
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index ab08815..7780251 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>> return false;
>> }
>>
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> - u32 len, u8 is_write)
>> -{
>> - if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>> - return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> - } else if (arm_addr_in_ioport_region(phys_addr)) {
>> - int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> - u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> - return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> - } else if (arm_addr_in_pci_region(phys_addr)) {
>> - return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> - }
>
> Can you explain why this arm_addr_in_pci_region(phys_addr) check has
> disappeared in your updated version on this function? It may be a non
> issue, but I'd very much like to understand.
If you look above the calls to kvm__emulate_mmio() are exactly the same
for the PCI and the virtio_mmio region, also as the areas are
non-overlapping the if branches can be reordered.
arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
while arm_addr_in_pci_region() gives true between 1GB and 2GB.
So this translates into: do kvm__emulate_io() for anything below 64K and
kvm__emulate_mmio() for everything else except for the GIC area,
admittedly in a quite convoluted way.
So my patch just removes the check for the GIC region and rewrites it to
match that description in the last sentence, with the rationale given in
the commit message.
Does that make sense?
If you desperately want some extra barfing for misguided GIC requests,
I'd rather introduce that to the "no match" code path in
kvm__emulate_mmio or register some dummy MMIO regions for the GIC with
panic() handlers.
Cheers,
Andre.
WARNING: multiple messages have this Message-ID (diff)
From: Andre Przywara <andre.przywara@arm.com>
To: Marc Zyngier <marc.zyngier@arm.com>, Will Deacon <Will.Deacon@arm.com>
Cc: "penberg@kernel.org" <penberg@kernel.org>,
"kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 06/10] arm: simplify MMIO dispatching
Date: Wed, 17 Jun 2015 14:49:59 +0100 [thread overview]
Message-ID: <55817B07.8010502@arm.com> (raw)
In-Reply-To: <55816C9F.1090500@arm.com>
Hi Marc,
On 06/17/2015 01:48 PM, Marc Zyngier wrote:
> On 17/06/15 12:21, Andre Przywara wrote:
>> Currently we separate any incoming MMIO request into one of the ARM
>> memory map regions and take care to spare the GIC.
>> It turns out that this is unnecessary, as we only have one special
>> region (the IO port area in the first 64 KByte). The MMIO rbtree
>> takes care about unhandled MMIO ranges, so we can simply drop all the
>> special range checking (except that for the IO range) in
>> kvm_cpu__emulate_mmio().
>> As the GIC is handled in the kernel, a GIC MMIO access should never
>> reach userland (and we don't know what to do with it anyway).
>> This lets us delete some more code and simplifies future extensions
>> (like expanding the GIC regions).
>> To be in line with the other architectures, move the now simpler
>> code into a header file.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> arm/include/arm-common/kvm-arch.h | 12 ------------
>> arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>> arm/kvm-cpu.c | 16 ----------------
>> 3 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index 082131d..90d6733 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>> return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>> }
>>
>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>> -{
>> - u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>> - return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>> -}
>> -
>> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
>> -{
>> - u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
>> - return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
>> -}
>> -
>> struct kvm_arch {
>> /*
>> * We may have to align the guest memory for virtio, so keep the
>> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
>> index 36c7872..329979a 100644
>> --- a/arm/include/arm-common/kvm-cpu-arch.h
>> +++ b/arm/include/arm-common/kvm-cpu-arch.h
>> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>> return false;
>> }
>>
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> - u32 len, u8 is_write);
>> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
>> + u8 *data, u32 len, u8 is_write)
>> +{
>> + if (arm_addr_in_ioport_region(phys_addr)) {
>> + int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> + u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> +
>> + return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> + }
>> +
>> + return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> +}
>>
>> unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>>
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index ab08815..7780251 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>> return false;
>> }
>>
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> - u32 len, u8 is_write)
>> -{
>> - if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>> - return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> - } else if (arm_addr_in_ioport_region(phys_addr)) {
>> - int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> - u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> - return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> - } else if (arm_addr_in_pci_region(phys_addr)) {
>> - return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> - }
>
> Can you explain why this arm_addr_in_pci_region(phys_addr) check has
> disappeared in your updated version on this function? It may be a non
> issue, but I'd very much like to understand.
If you look above the calls to kvm__emulate_mmio() are exactly the same
for the PCI and the virtio_mmio region, also as the areas are
non-overlapping the if branches can be reordered.
arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
while arm_addr_in_pci_region() gives true between 1GB and 2GB.
So this translates into: do kvm__emulate_io() for anything below 64K and
kvm__emulate_mmio() for everything else except for the GIC area,
admittedly in a quite convoluted way.
So my patch just removes the check for the GIC region and rewrites it to
match that description in the last sentence, with the rationale given in
the commit message.
Does that make sense?
If you desperately want some extra barfing for misguided GIC requests,
I'd rather introduce that to the "no match" code path in
kvm__emulate_mmio or register some dummy MMIO regions for the GIC with
panic() handlers.
Cheers,
Andre.
next prev parent reply other threads:[~2015-06-17 13:49 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-17 11:21 [PATCH v3 00/10] kvmtool: arm64: GICv3 guest support Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 01/10] AArch64: Reserve two 64k pages for GIC CPU interface Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 02/10] AArch{32, 64}: use KVM_CREATE_DEVICE & co to instanciate the GIC Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 03/10] irq: add irq__get_nr_allocated_lines Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 04/10] AArch{32, 64}: dynamically configure the number of GIC interrupts Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 05/10] arm: finish VGIC initialisation explicitly Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 06/10] arm: simplify MMIO dispatching Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 12:48 ` Marc Zyngier
2015-06-17 12:48 ` Marc Zyngier
2015-06-17 13:49 ` Andre Przywara [this message]
2015-06-17 13:49 ` Andre Przywara
2015-06-17 14:06 ` Marc Zyngier
2015-06-17 14:06 ` Marc Zyngier
2015-06-24 13:30 ` Andre Przywara
2015-06-24 13:30 ` Andre Przywara
2015-06-24 17:56 ` Will Deacon
2015-06-24 17:56 ` Will Deacon
2015-06-17 11:21 ` [PATCH v3 07/10] limit number of VCPUs on demand Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 12:53 ` Marc Zyngier
2015-06-17 12:53 ` Marc Zyngier
2015-06-17 13:14 ` Andre Przywara
2015-06-17 13:14 ` Andre Przywara
2015-06-17 11:21 ` [PATCH v3 08/10] arm: prepare for instantiating different IRQ chip devices Andre Przywara
2015-06-17 11:21 ` Andre Przywara
2015-06-17 13:06 ` Marc Zyngier
2015-06-17 13:06 ` Marc Zyngier
2015-06-17 11:22 ` [PATCH v3 09/10] arm: add support for supplying GICv3 redistributor addresses Andre Przywara
2015-06-17 11:22 ` Andre Przywara
2015-06-17 13:09 ` Marc Zyngier
2015-06-17 13:09 ` Marc Zyngier
2015-06-17 11:22 ` [PATCH v3 10/10] arm: use new irqchip parameter to create different vGIC types Andre Przywara
2015-06-17 11:22 ` Andre Przywara
2015-06-17 13:16 ` Marc Zyngier
2015-06-17 13:16 ` Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55817B07.8010502@arm.com \
--to=andre.przywara@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.