| Message ID | 20210414091832.5132-1-lingshan.zhu@intel.com |
|---|---|
| Headers | show |
| Series | vDPA/ifcvf: enables Intel C5000X-PL virtio-blk | expand |
在 2021/4/14 下午5:18, Zhu Lingshan 写道: > This commit deduces VIRTIO device ID as device type when probe, > then ifcvf_vdpa_get_device_id() can simply return the ID. > ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size() > can work properly based on the device ID. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/ifcvf/ifcvf_base.h | 1 + > drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------ > 2 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h > index b2eeb16b9c2c..1c04cd256fa7 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.h > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > @@ -84,6 +84,7 @@ struct ifcvf_hw { > u32 notify_off_multiplier; > u64 req_features; > u64 hw_features; > + u32 dev_type; > struct virtio_pci_common_cfg __iomem *common_cfg; > void __iomem *net_cfg; > struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2]; > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 44d7586019da..99b0a6b4c227 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct vdpa_device *vdpa_dev) > > static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev) > { > - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev); > - struct pci_dev *pdev = adapter->pdev; > - u32 ret = -ENODEV; > - > - if (pdev->device < 0x1000 || pdev->device > 0x107f) > - return ret; > - > - if (pdev->device < 0x1040) > - ret = pdev->subsystem_device; > - else > - ret = pdev->device - 0x1040; > + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); > > - return ret; > + return vf->dev_type; > } > > static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev) > @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id) > pci_set_drvdata(pdev, adapter); > > vf = &adapter->vf; > + if (pdev->device < 0x1000 || pdev->device > 0x107f) > + return -EOPNOTSUPP; > + > + if (pdev->device < 0x1040) > + vf->dev_type = pdev->subsystem_device; > + else > + vf->dev_type = pdev->device - 0x1040; So a question here, is the device a transtional device or modern one? If it's a transitonal one, can it swtich endianess automatically or not? Thanks > + > vf->base = pcim_iomap_table(pdev); > > adapter->pdev = pdev;
在 2021/4/14 下午5:18, Zhu Lingshan 写道: > get_config_size() should return the size based on the decected > device type. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 9b6a38b798fa..b48b9789b69e 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) > > static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) > { > - return sizeof(struct virtio_net_config); > + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); > + size_t size; > + > + if (vf->dev_type == VIRTIO_ID_NET) > + size = sizeof(struct virtio_net_config); > + > + if (vf->dev_type == VIRTIO_ID_BLOCK) > + size = sizeof(struct virtio_blk_config); > + > + return size; > } > > static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
On 4/15/2021 11:30 AM, Jason Wang wrote: > > 在 2021/4/14 下午5:18, Zhu Lingshan 写道: >> This commit deduces VIRTIO device ID as device type when probe, >> then ifcvf_vdpa_get_device_id() can simply return the ID. >> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size() >> can work properly based on the device ID. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------ >> 2 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >> b/drivers/vdpa/ifcvf/ifcvf_base.h >> index b2eeb16b9c2c..1c04cd256fa7 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >> @@ -84,6 +84,7 @@ struct ifcvf_hw { >> u32 notify_off_multiplier; >> u64 req_features; >> u64 hw_features; >> + u32 dev_type; >> struct virtio_pci_common_cfg __iomem *common_cfg; >> void __iomem *net_cfg; >> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2]; >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >> b/drivers/vdpa/ifcvf/ifcvf_main.c >> index 44d7586019da..99b0a6b4c227 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct >> vdpa_device *vdpa_dev) >> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev) >> { >> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev); >> - struct pci_dev *pdev = adapter->pdev; >> - u32 ret = -ENODEV; >> - >> - if (pdev->device < 0x1000 || pdev->device > 0x107f) >> - return ret; >> - >> - if (pdev->device < 0x1040) >> - ret = pdev->subsystem_device; >> - else >> - ret = pdev->device - 0x1040; >> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >> - return ret; >> + return vf->dev_type; >> } >> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev) >> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, >> const struct pci_device_id *id) >> pci_set_drvdata(pdev, adapter); >> vf = &adapter->vf; >> + if (pdev->device < 0x1000 || pdev->device > 0x107f) >> + return -EOPNOTSUPP; >> + >> + if (pdev->device < 0x1040) >> + vf->dev_type = pdev->subsystem_device; >> + else >> + vf->dev_type = pdev->device - 0x1040; > > > So a question here, is the device a transtional device or modern one? > > If it's a transitonal one, can it swtich endianess automatically or not? > > Thanks Hi Jason, This driver should drive both modern and transitional devices as we discussed before. If it's a transitional one, it will act as a modern device by default, legacy mode is a fail-over path. For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must in modern mode. I think we don't need to worry about endianess for legacy mode. Thanks Zhu Lingshan > > >> + >> vf->base = pcim_iomap_table(pdev); >> adapter->pdev = pdev; >
在 2021/4/15 下午1:52, Zhu Lingshan 写道: > > > On 4/15/2021 11:30 AM, Jason Wang wrote: >> >> 在 2021/4/14 下午5:18, Zhu Lingshan 写道: >>> This commit deduces VIRTIO device ID as device type when probe, >>> then ifcvf_vdpa_get_device_id() can simply return the ID. >>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size() >>> can work properly based on the device ID. >>> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>> --- >>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------ >>> 2 files changed, 11 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>> index b2eeb16b9c2c..1c04cd256fa7 100644 >>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>> @@ -84,6 +84,7 @@ struct ifcvf_hw { >>> u32 notify_off_multiplier; >>> u64 req_features; >>> u64 hw_features; >>> + u32 dev_type; >>> struct virtio_pci_common_cfg __iomem *common_cfg; >>> void __iomem *net_cfg; >>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2]; >>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>> index 44d7586019da..99b0a6b4c227 100644 >>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct >>> vdpa_device *vdpa_dev) >>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev) >>> { >>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev); >>> - struct pci_dev *pdev = adapter->pdev; >>> - u32 ret = -ENODEV; >>> - >>> - if (pdev->device < 0x1000 || pdev->device > 0x107f) >>> - return ret; >>> - >>> - if (pdev->device < 0x1040) >>> - ret = pdev->subsystem_device; >>> - else >>> - ret = pdev->device -0x1040; >>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>> - return ret; >>> + return vf->dev_type; >>> } >>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev) >>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, >>> const struct pci_device_id *id) >>> pci_set_drvdata(pdev, adapter); >>> vf = &adapter->vf; >>> + if (pdev->device < 0x1000 || pdev->device > 0x107f) >>> + return -EOPNOTSUPP; >>> + >>> + if (pdev->device < 0x1040) >>> + vf->dev_type = pdev->subsystem_device; >>> + else >>> + vf->dev_type = pdev->device - 0x1040; >> >> >> So a question here, is the device a transtional device or modern one? >> >> If it's a transitonal one, can it swtich endianess automatically or not? >> >> Thanks > Hi Jason, > > This driver should drive both modern and transitional devices as we > discussed before. > If it's a transitional one, it will act as a modern device by default, > legacy mode is a fail-over path. Note that legacy driver use native endian, support legacy driver requires the device to know native endian which I'm not sure your device can do that. Thanks > For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must > in modern mode. > I think we don't need to worry about endianess for legacy mode. > > Thanks > Zhu Lingshan >> >> >>> + >>> vf->base = pcim_iomap_table(pdev); >>> adapter->pdev = pdev; >> >
On 4/15/2021 2:30 PM, Jason Wang wrote: > > 在 2021/4/15 下午1:52, Zhu Lingshan 写道: >> >> >> On 4/15/2021 11:30 AM, Jason Wang wrote: >>> >>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道: >>>> This commit deduces VIRTIO device ID as device type when probe, >>>> then ifcvf_vdpa_get_device_id() can simply return the ID. >>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size() >>>> can work properly based on the device ID. >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>> --- >>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------ >>>> 2 files changed, 11 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> index b2eeb16b9c2c..1c04cd256fa7 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> @@ -84,6 +84,7 @@ struct ifcvf_hw { >>>> u32 notify_off_multiplier; >>>> u64 req_features; >>>> u64 hw_features; >>>> + u32 dev_type; >>>> struct virtio_pci_common_cfg __iomem *common_cfg; >>>> void __iomem *net_cfg; >>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2]; >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> index 44d7586019da..99b0a6b4c227 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct >>>> vdpa_device *vdpa_dev) >>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev) >>>> { >>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev); >>>> - struct pci_dev *pdev = adapter->pdev; >>>> - u32 ret = -ENODEV; >>>> - >>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f) >>>> - return ret; >>>> - >>>> - if (pdev->device < 0x1040) >>>> - ret = pdev->subsystem_device; >>>> - else >>>> - ret = pdev->device -0x1040; >>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>> - return ret; >>>> + return vf->dev_type; >>>> } >>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev) >>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, >>>> const struct pci_device_id *id) >>>> pci_set_drvdata(pdev, adapter); >>>> vf = &adapter->vf; >>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f) >>>> + return -EOPNOTSUPP; >>>> + >>>> + if (pdev->device < 0x1040) >>>> + vf->dev_type = pdev->subsystem_device; >>>> + else >>>> + vf->dev_type = pdev->device - 0x1040; >>> >>> >>> So a question here, is the device a transtional device or modern one? >>> >>> If it's a transitonal one, can it swtich endianess automatically or >>> not? >>> >>> Thanks >> Hi Jason, >> >> This driver should drive both modern and transitional devices as we >> discussed before. >> If it's a transitional one, it will act as a modern device by >> default, legacy mode is a fail-over path. > > > Note that legacy driver use native endian, support legacy driver > requires the device to know native endian which I'm not sure your > device can do that. > > Thanks Yes, legacy requires guest native endianess, I think we don't need to worry about this because our transitional device should work in modern mode by default(legacy mode is the failover path we will never reach, get_features will fail if no ACCESS_PLATFORM), we don't support legacy device in vDPA. Thanks > > >> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must >> in modern mode. >> I think we don't need to worry about endianess for legacy mode. >> >> Thanks >> Zhu Lingshan >>> >>> >>>> + >>>> vf->base = pcim_iomap_table(pdev); >>>> adapter->pdev = pdev; >>> >> >
在 2021/4/15 下午2:36, Zhu Lingshan 写道: > > > On 4/15/2021 2:30 PM, Jason Wang wrote: >> >> 在 2021/4/15 下午1:52, Zhu Lingshan 写道: >>> >>> >>> On 4/15/2021 11:30 AM, Jason Wang wrote: >>>> >>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道: >>>>> This commit deduces VIRTIO device ID as device type when probe, >>>>> then ifcvf_vdpa_get_device_id() can simply return the ID. >>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size() >>>>> can work properly based on the device ID. >>>>> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>> --- >>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------ >>>>> 2 files changed, 11 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> index b2eeb16b9c2c..1c04cd256fa7 100644 >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw { >>>>> u32 notify_off_multiplier; >>>>> u64 req_features; >>>>> u64 hw_features; >>>>> + u32 dev_type; >>>>> struct virtio_pci_common_cfg __iomem *common_cfg; >>>>> void __iomem *net_cfg; >>>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2]; >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> index 44d7586019da..99b0a6b4c227 100644 >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct >>>>> vdpa_device *vdpa_dev) >>>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev) >>>>> { >>>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev); >>>>> - struct pci_dev *pdev = adapter->pdev; >>>>> - u32 ret = -ENODEV; >>>>> - >>>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f) >>>>> - return ret; >>>>> - >>>>> - if (pdev->device < 0x1040) >>>>> - ret = pdev->subsystem_device; >>>>> - else >>>>> - ret = pdev->device-0x1040; >>>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>>> - return ret; >>>>> + return vf->dev_type; >>>>> } >>>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev) >>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, >>>>> const struct pci_device_id *id) >>>>> pci_set_drvdata(pdev, adapter); >>>>> vf = &adapter->vf; >>>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f) >>>>> + return -EOPNOTSUPP; >>>>> + >>>>> + if (pdev->device < 0x1040) >>>>> + vf->dev_type = pdev->subsystem_device; >>>>> + else >>>>> + vf->dev_type = pdev->device - 0x1040; >>>> >>>> >>>> So a question here, is the device a transtional device or modern one? >>>> >>>> If it's a transitonal one, can it swtich endianess automatically or >>>> not? >>>> >>>> Thanks >>> Hi Jason, >>> >>> This driver should drive both modern and transitional devices as we >>> discussed before. >>> If it's a transitional one, it will act as a modern device by >>> default, legacy mode is a fail-over path. >> >> >> Note that legacy driver use native endian, support legacy driver >> requires the device to know native endian which I'm not sure your >> device can do that. >> >> Thanks > Yes, legacy requires guest native endianess, I think we don't need to > worry about this because our transitional device should work in modern > mode by > default(legacy mode is the failover path we will never reach, > get_features will fail if no ACCESS_PLATFORM), we don't support legacy > device in vDPA. > > Thanks Ok, so I think it's better to add a comment here. Thanks >> >> >>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must >>> in modern mode. >>> I think we don't need to worry about endianess for legacy mode. >>> >>> Thanks >>> Zhu Lingshan >>>> >>>> >>>>> + >>>>> vf->base = pcim_iomap_table(pdev); >>>>> adapter->pdev = pdev; >>>> >>> >> >
On 4/15/2021 3:16 PM, Jason Wang wrote: > > 在 2021/4/15 下午2:36, Zhu Lingshan 写道: >> >> >> On 4/15/2021 2:30 PM, Jason Wang wrote: >>> >>> 在 2021/4/15 下午1:52, Zhu Lingshan 写道: >>>> >>>> >>>> On 4/15/2021 11:30 AM, Jason Wang wrote: >>>>> >>>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道: >>>>>> This commit deduces VIRTIO device ID as device type when probe, >>>>>> then ifcvf_vdpa_get_device_id() can simply return the ID. >>>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size() >>>>>> can work properly based on the device ID. >>>>>> >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> --- >>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------ >>>>>> 2 files changed, 11 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> index b2eeb16b9c2c..1c04cd256fa7 100644 >>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw { >>>>>> u32 notify_off_multiplier; >>>>>> u64 req_features; >>>>>> u64 hw_features; >>>>>> + u32 dev_type; >>>>>> struct virtio_pci_common_cfg __iomem *common_cfg; >>>>>> void __iomem *net_cfg; >>>>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2]; >>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> index 44d7586019da..99b0a6b4c227 100644 >>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct >>>>>> vdpa_device *vdpa_dev) >>>>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device >>>>>> *vdpa_dev) >>>>>> { >>>>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev); >>>>>> - struct pci_dev *pdev = adapter->pdev; >>>>>> - u32 ret = -ENODEV; >>>>>> - >>>>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f) >>>>>> - return ret; >>>>>> - >>>>>> - if (pdev->device < 0x1040) >>>>>> - ret = pdev->subsystem_device; >>>>>> - else >>>>>> - ret = pdev->device-0x1040; >>>>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>>>> - return ret; >>>>>> + return vf->dev_type; >>>>>> } >>>>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device >>>>>> *vdpa_dev) >>>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, >>>>>> const struct pci_device_id *id) >>>>>> pci_set_drvdata(pdev, adapter); >>>>>> vf = &adapter->vf; >>>>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f) >>>>>> + return -EOPNOTSUPP; >>>>>> + >>>>>> + if (pdev->device < 0x1040) >>>>>> + vf->dev_type = pdev->subsystem_device; >>>>>> + else >>>>>> + vf->dev_type = pdev->device - 0x1040; >>>>> >>>>> >>>>> So a question here, is the device a transtional device or modern one? >>>>> >>>>> If it's a transitonal one, can it swtich endianess automatically >>>>> or not? >>>>> >>>>> Thanks >>>> Hi Jason, >>>> >>>> This driver should drive both modern and transitional devices as we >>>> discussed before. >>>> If it's a transitional one, it will act as a modern device by >>>> default, legacy mode is a fail-over path. >>> >>> >>> Note that legacy driver use native endian, support legacy driver >>> requires the device to know native endian which I'm not sure your >>> device can do that. >>> >>> Thanks >> Yes, legacy requires guest native endianess, I think we don't need to >> worry about this because our transitional device should work in >> modern mode by >> default(legacy mode is the failover path we will never reach, >> get_features will fail if no ACCESS_PLATFORM), we don't support >> legacy device in vDPA. >> >> Thanks > > > Ok, so I think it's better to add a comment here. sure, will add a comment in V2 Thanks > > Thanks > > >>> >>> >>>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it >>>> must in modern mode. >>>> I think we don't need to worry about endianess for legacy mode. >>>> >>>> Thanks >>>> Zhu Lingshan >>>>> >>>>> >>>>>> + >>>>>> vf->base = pcim_iomap_table(pdev); >>>>>> adapter->pdev = pdev; >>>>> >>>> >>> >> >
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote: >get_config_size() should return the size based on the decected >device type. > >Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >--- > drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > >diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >index 9b6a38b798fa..b48b9789b69e 100644 >--- a/drivers/vdpa/ifcvf/ifcvf_main.c >+++ b/drivers/vdpa/ifcvf/ifcvf_main.c >@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) > > static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) > { >- return sizeof(struct virtio_net_config); >+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >+ size_t size; >+ >+ if (vf->dev_type == VIRTIO_ID_NET) >+ size = sizeof(struct virtio_net_config); >+ >+ if (vf->dev_type == VIRTIO_ID_BLOCK) >+ size = sizeof(struct virtio_blk_config); >+ >+ return size; I'm not familiar with the ifcvf details, but can it happen that the device is not block or net? Should we set `size` to 0 by default to handle this case or are we sure it's one of the two? Maybe we should add a comment or a warning message in this case, to prevent some analysis tool or compiler from worrying that `size` might be uninitialized. I was thinking something like this: switch(vf->dev_type) { case VIRTIO_ID_NET: size = sizeof(struct virtio_net_config); break; case VIRTIO_ID_BLOCK: size = sizeof(struct virtio_blk_config); break; default: /* or WARN(1, "") if dev_warn() not apply */ dev_warn(... , "virtio ID [0x%x] not supported\n") size = 0; } Thanks, Stefano
在 2021/4/15 下午4:12, Stefano Garzarella 写道: > On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote: >> get_config_size() should return the size based on the decected >> device type. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >> b/drivers/vdpa/ifcvf/ifcvf_main.c >> index 9b6a38b798fa..b48b9789b69e 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct >> vdpa_device *vdpa_dev) >> >> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) >> { >> - return sizeof(struct virtio_net_config); >> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >> + size_t size; >> + >> + if (vf->dev_type == VIRTIO_ID_NET) >> + size = sizeof(struct virtio_net_config); >> + >> + if (vf->dev_type == VIRTIO_ID_BLOCK) >> + size = sizeof(struct virtio_blk_config); >> + >> + return size; > > I'm not familiar with the ifcvf details, but can it happen that the > device is not block or net? > > Should we set `size` to 0 by default to handle this case or are we > sure it's one of the two? > > Maybe we should add a comment or a warning message in this case, to > prevent some analysis tool or compiler from worrying that `size` might > be uninitialized. > > I was thinking something like this: > > switch(vf->dev_type) { > case VIRTIO_ID_NET: > size = sizeof(struct virtio_net_config); > break; > case VIRTIO_ID_BLOCK: > size = sizeof(struct virtio_blk_config); > break; > default: > /* or WARN(1, "") if dev_warn() not apply */ > dev_warn(... , "virtio ID [0x%x] not supported\n") > size = 0; > > } > Yes, I agree. Thanks > Thanks, > Stefano >