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

[Gonglei (Arei)]

> -----Original Message-----
> From: Markus Armbruster [mailto:address@hidden
> Sent: Tuesday, November 15, 2016 5:05 PM
> To: Zhuangyanying
> Cc: address@hidden; Gonglei (Arei); Huangweidong (C);
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH] hw/misc/ivshmem:fix misconfig of
> not_legacy_32bit
> 
> Zhuangyanying <address@hidden> writes:
> 
> > From: ZhuangYanying <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.
> 
> Please limit commit message line length to about 70 characters.
> 
> >
> > Signed-off-by: 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;
> >      }
> >
> 
> This hunk fixes a regression in commit 5400c02, v2.6.0.  It should go
> into 2.7.0.
> 
> > @@ -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(),
> >  };
> 
> These two hunks add legacy property "use64" to to non-legacy devices.
> These devices not having the property is intentional.  Are you sure
> adding them makes sense?  If yes, make it a separate patch, and give
> your reasons in the commit message.
> 
It's reasonable. :)

> Please repost just the regression fix as soon as possible.

Yanying, pls respins your patch, and CC qemu-stable@
adds my R-b.

Regards,
-Gonglei