[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] ivshmem: fix misconfig of not_legacy_32b
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] ivshmem: fix misconfig of not_legacy_32bit |
Date: |
Thu, 17 Nov 2016 11:55:35 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Zhuangyanying <address@hidden> writes:
> From: ZhuangYanying <address@hidden>
>
> After commit 5400c02, ivshmem_64bit renamed to not_legacy_32bit,
> and changed the implementation of this property.
> Then use64 = 1, ~PCI_BASE_ADDRESS_MEM_TYPE_64 (default for ivshmem),
> the actual use is the legacy model,
> can not support greater than or equal 1G mapping,
> which is the opposite of configuration requirements.
>
> Cc: address@hidden
> Signed-off-by: Zhuang Yanying <address@hidden>
> Reviewed-by: Gonglei <address@hidden>
I find the commit message hard to understand. To improve it, let's
figure out what exactly is broken.
The sense of device ivshmem's property use64 is reverted: use64=0
enables the 64 bit memory bar, use64=1 disables it. Default is 32 bit
unless pc-1.2 or older. This regressed in commit 5400c02. Before,
use64 had the correct sense, and the default was 64 bit unless pc-1.2 or
older.
Note that devices ivshmem-plain and ivshmem-doorbell always have a 64
bit memory bar, as intended.
> ---
> hw/misc/ivshmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 230e51b..b897685 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -858,7 +858,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error
> **errp)
> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
> &s->ivshmem_mmio);
>
> - if (!s->not_legacy_32bit) {
> + if (s->not_legacy_32bit) {
> attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> }
Thanks a lot for catching my mistake!
However, this breaks ivshmem-plain and ivshmem-doorbell. You fix that
in the next patch, but that's no good, the two need to be squashed into
a single commit.
Suggested commit message for the squashed commit:
ivshmem: Fix 64 bit memory bar configuration
Device ivshmem property use64=0 is designed to make the device
expose a 32 bit shared memory BAR instead of 64 bit one. The
default is a 64 bit BAR, except pc-1.2 and older retain a 32 bit
BAR. A 32 bit BAR can support only up to 1 GiB of shared memory.
This worked as designed until commit 5400c02 accidentally flipped
its sense: since then, we misinterpret use64=0 as use64=1 and vice
versa. Worse, the default got flipped as well. Devices
ivshmem-plain and ivshmem-doorbell are not affected.
Fix by restoring the test of IVShmemState member not_legacy_32bit
that got messed up in commit 5400c02. Also update its
initialization for devices ivhsmem-plain and ivshmem-doorbell.
Without that, they'd regress to 32 bit BARs.
Cc: address@hidden
Would that work for you?