Re: [PATCH 1/6] vDPA/ifcvf: get_config_size should return a value no greater than dev implementation

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 6/2/2022 3:11 PM, Jason Wang wrote:
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> ifcvf_get_config_size() should return a virtio device type specific value,
>> however the ret_value should not be greater than the onboard size of
>> the device implementation. E.g., for virtio_net, config_size should be
>> the minimum value of sizeof(struct virtio_net_config) and the onboard
>> cap size.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++--
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 48c4dadb0c7c..6bccc8291c26 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
>>                          break;
>>                  case VIRTIO_PCI_CAP_DEVICE_CFG:
>>                          hw->dev_cfg = get_cap_addr(hw, &cap);
>> +                       hw->cap_dev_config_size = le32_to_cpu(cap.length);
> One possible issue here is that, if the hardware have more size than
> spec, we may end up with a migration compatibility issue.
>
> It looks to me we'd better build the config size based on the
> features, e.g it looks to me for net, we should probably use
>
> offset_of(struct virtio_net_config, mtu)?
Hi Jason,

our ifcvf devices have nothing out of the spec in the device config 
space, so I think we can trust the cap size
>
>>                          IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
>>                          break;
>>                  }
>> @@ -233,15 +234,18 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features)
>>   u32 ifcvf_get_config_size(struct ifcvf_hw *hw)
>>   {
>>          struct ifcvf_adapter *adapter;
>> +       u32 net_config_size = sizeof(struct virtio_net_config);
>> +       u32 blk_config_size = sizeof(struct virtio_blk_config);
>> +       u32 cap_size = hw->cap_dev_config_size;
>>          u32 config_size;
>>
>>          adapter = vf_to_adapter(hw);
>>          switch (hw->dev_type) {
>>          case VIRTIO_ID_NET:
>> -               config_size = sizeof(struct virtio_net_config);
>> +               config_size = cap_size >= net_config_size ? net_config_size : cap_size;
> I don't get the code here, any chance that net_config_size could be zero?
This means, if the capability size is more than the size of structure 
virtio-net-cofnig,
there may be something out of the spec, then we only migrate the spec 
contents, which is
a defensive coding. If the capability size is smaller than the size in 
spec, means
the capability is a sub-set of the spec contents, we only migrate the 
onboard contents.

Sorry for the confusing, I will add a comment here.

Thanks
Zhu Lingshan
>
> Thanks
>
>>                  break;
>>          case VIRTIO_ID_BLOCK:
>> -               config_size = sizeof(struct virtio_blk_config);
>> +               config_size = cap_size >= blk_config_size ? blk_config_size : cap_size;
>>                  break;
>>          default:
>>                  config_size = 0;
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 115b61f4924b..f5563f665cc6 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -87,6 +87,8 @@ struct ifcvf_hw {
>>          int config_irq;
>>          int vqs_reused_irq;
>>          u16 nr_vring;
>> +       /* VIRTIO_PCI_CAP_DEVICE_CFG size */
>> +       u32 cap_dev_config_size;
>>   };
>>
>>   struct ifcvf_adapter {
>> --
>> 2.31.1
>>