From: marc.zyngier@arm•com (Marc Zyngier)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 06/10] arm: simplify MMIO dispatching
Date: Wed, 17 Jun 2015 15:06:34 +0100 [thread overview]
Message-ID: <55817EEA.9060402@arm.com> (raw)
In-Reply-To: <55817B07.8010502@arm.com>
On 17/06/15 14:49, Andre Przywara wrote:
> 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.
No, that's fine.
I just wondered what was the rational behind having the
arm_addr_in_pci_region() call there. It might have guarded something,
but if you're absolutely positive that this doesn't cause a regression,
that's OK with me.
Reviewed-by: Marc Zyngier <marc.zyngier@arm•com>
M.
--
Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2015-06-17 14:06 UTC|newest]
Thread overview: 21+ 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 ` [PATCH v3 01/10] AArch64: Reserve two 64k pages for GIC CPU interface 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 ` [PATCH v3 03/10] irq: add irq__get_nr_allocated_lines 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 ` [PATCH v3 05/10] arm: finish VGIC initialisation explicitly Andre Przywara
2015-06-17 11:21 ` [PATCH v3 06/10] arm: simplify MMIO dispatching Andre Przywara
2015-06-17 12:48 ` Marc Zyngier
2015-06-17 13:49 ` Andre Przywara
2015-06-17 14:06 ` Marc Zyngier [this message]
2015-06-24 13:30 ` Andre Przywara
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 12:53 ` Marc Zyngier
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 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 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 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=55817EEA.9060402@arm.com \
--to=marc.zyngier@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox