Re: [Qemu-devel] [PATCH] hw/misc/ivshmem:fix misconfig of not_legacy_32bit

[Gonglei]

On 2016/11/15 16:47, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Nov 14, 2016 at 4:44 PM Zhuangyanying <address@hidden 
> <mailto:address@hidden>> wrote:
> 
>     From: ZhuangYanying <address@hidden <mailto:address@hidden>>
> 
>     After "ivshmem: Split ivshmem-plain, ivshmem-doorbell off ivshmem", 
> ivshmem_64bit renamed to not_legacy_32bit, and changed the implementation of 
> this property.
>     Then use64 = not_legacy_32bit = 1, then PCI attribute configuration ~ 
> 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.
> 
>     Signed-off-by: address@hidden <mailto:address@hidden>
>     ---
>     Recently, I tested ivshmem, found that use64, that is not_legacy_32bit 
> implementation is odd, or even the opposite.
>     Previous use64 = ivshmem_64bit = 1, then attr |= 
> PCI_BASE_ADDRESS_MEM_TYPE_64, then ivshmem support 1G and above packaged into 
> bar2, presented to the virtual machine.
>     But after "ivshmem: Split ivshmem-plain, ivshmem-doorbell off ivshmem", 
> PCI_BASE_ADDRESS_MEM_TYPE_64 is configured while not_legacy_32bit = 0, that 
> is the legacy model.
>     ---
>      hw/misc/ivshmem.c | 4 +++-
>      1 file changed, 3 insertions(+), 1 deletion(-)
> 
>     diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>     index 230e51b..b71acf6 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;
>          }
> 
> 
> ouch, yes
>  
> 
>     @@ -1033,6 +1033,7 @@ static const VMStateDescription ivshmem_plain_vmsd 
> = {
> 
>      static Property ivshmem_plain_properties[] = {
>          DEFINE_PROP_ON_OFF_AUTO("master", IVShmemState, master, 
> ON_OFF_AUTO_OFF),
>     +    DEFINE_PROP_UINT32("use64", IVShmemState, not_legacy_32bit, 1),
>          DEFINE_PROP_END_OF_LIST(),
>      };
> 
>     @@ -1107,6 +1108,7 @@ static Property ivshmem_doorbell_properties[] = {
>          DEFINE_PROP_BIT("ioeventfd", IVShmemState, features, 
> IVSHMEM_IOEVENTFD,
>                          true),
>          DEFINE_PROP_ON_OFF_AUTO("master", IVShmemState, master, 
> ON_OFF_AUTO_OFF),
>     +    DEFINE_PROP_UINT32("use64", IVShmemState, not_legacy_32bit, 1),
>          DEFINE_PROP_END_OF_LIST(),
>      };
> 
> 
> I think we shouldn't reintroduce the use64 property, instead, initialize 
> not_legacy_32bit in realize() ?
> 
It means we can't manually configure 32bit mode if we do that, 
and if that's what we want, we just need drop the property.
I'd like to keep consistence with the previous QEMU.

Regards,
-Gonglei
> 
> -- 
> Marc-André Lureau