Re: [dpdk-dev] [RFC PATCH] bus/pci: avoid depending on private value in kernel source

[Tiwei Bie <tiwei.bie@intel.com>]

On Wed, Jul 03, 2019 at 10:01:44AM +0200, David Marchand wrote:
> On Wed, Jul 3, 2019 at 9:58 AM Tiwei Bie <tiwei.bie@intel.com> wrote:
> 
>     On Wed, Jul 03, 2019 at 09:36:26AM +0200, David Marchand wrote:
>     > On Wed, Jul 3, 2019 at 9:35 AM Tiwei Bie <tiwei.bie@intel.com> wrote:
>     >
>     >     Hi David,
>     >
>     >     On Wed, Jul 03, 2019 at 09:02:59AM +0200, David Marchand wrote:
>     >     > Hello,
>     >     >
>     >     > On Wed, Jul 3, 2019 at 7:47 AM Tiwei Bie <tiwei.bie@intel.com>
>     wrote:
>     >     >
>     >     >     The value 40 used in VFIO_GET_REGION_ADDR() is a private value
>     >     >     (VFIO_PCI_OFFSET_SHIFT) defined in Linux kernel source [1]. It
>     >     >     is not part of VFIO API, and we should not depend on it.
>     >     >
>     >     >     [1] https://github.com/torvalds/linux/blob/6fbc7275c7a9/drivers
>     /vfio/
>     >     pci/
>     >     >     vfio_pci_private.h#L19
>     >     >
>     >     >
>     >     >
>     >     > I did not follow linux kernel changes, is there something that
>     would
>     >     change
>     >     > this offset?
>     >     > It looks like a cleanup (did not look into the details yet), do we
>     need
>     >     this
>     >     > now?
>     >
>     >     In VFIO/mdev [1], the offset can be something different. It depends
>     >     on the parent device. It's not just a cleanup. It's a preparation
>     >     for the mdev support in DPDK.
>     >
>     >     [1] https://github.com/torvalds/linux/blob/master/Documentation/
>     >     vfio-mediated-device.txt
>     >
>     >
>     >
>     > Ok, thanks.
>     > So we can wait for mdev to be ready before working on this.
> 
>     What do you mean by "mdev to be ready"? RFC ready? I don't see
>     anything blocking the discussion on this now.
> 
>     PS. I already sent a RFC series of the mdev support in DPDK
>         to the mailing list 3 month ago.
> 
> 
> If you need it and the mdev support has been posted already, why not send a n+1
> patchset with this patch in it?
> 
> This patch alone looked odd to me.

That series was using the old API which assumes the shift
is 40 which may not work in some cases. And this patch is
to fix the API. I think this patch is actually trying to
fix a relatively independent issue -- i.e. switching to using
the proper VFIO API to get the region offsets instead of
depending on kernel code's internal value.

> I want to understand if this is a fix we need now or something that can wait.
> We have a really large backlog.

I guess that's why I didn't get a lot of responses in that
RFC series. :)

Thanks,
Tiwei

> 
> 
> --
> David Marchand